Repository navigation
[fix](be) Pass uint64_t offsets to the ANN index writer in unit tests - #69584
Merged
csun5285 merged 1 commit intoOct 25, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69070 Problem Summary: apache#69070 changed the offsets parameter of the ANN index writers' add_array_values() to const uint64_t*, but three unit tests still build their offsets as std::vector<size_t> and pass offsets.data(): ann_index_writer_test.cpp (15 places), ann_index_edge_case_test.cpp and snii_ann_container_test.cpp. On Linux size_t and uint64_t are both unsigned long, so the tests compile. On macOS size_t is unsigned long and uint64_t is unsigned long long, and ann_index_edge_case_test.cpp fails with "cannot initialize a parameter of type 'const uint64_t *' (aka 'const unsigned long long *') with an rvalue of type 'value_type *' (aka 'unsigned long *')". The vector_search_test library is force-loaded into doris_be_test, so no BE unit test builds on macOS. The macOS CI pipeline builds the BE without tests and does not see it. These tests now declare the offsets as std::vector<uint64_t>, the type the writer takes. Production code is unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test - macOS: the BE UT build compiles the three files and links doris_be_test again; AnnIndexWriterTest, VectorSearchTest and SniiAnnContainerTest pass. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
morningman
requested review from
airborne12,
csun5285 and
eldenmoon
as code owners
October 25, 2026 08:00
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all 17 offset-vector type replacements across the three changed BE test files at head 29debc8. No actionable findings. The declarations now exactly match the existing const uint64_t* parameter in both IndexColumnWriter and AnnIndexColumnWriter; offset values, lengths, row counts, and assertions are preserved. Existing inline findings: none. Additional user-provided focus: none.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | The source fixes the reported macOS pointer-type mismatch. Existing ANN/SNII tests exercise the calls and CMake includes them in doris_be_test; build success is not independently verified. |
| Scope and clarity | Minimal: 17 declaration-only changes (15 writer tests, one large-dimension test, one SNII helper); production code is unchanged. |
| Concurrency and locks | No thread, shared-state access, lock, or synchronization changes. |
| Lifecycle and static initialization | Local vectors remain alive during synchronous offset consumption; the callee retains no offset pointer. No static initialization changes. |
| Configuration | No configuration additions or dynamic-setting behavior changes. Existing test configuration handling is unchanged. |
| Compatibility | No production ABI, storage-format, rolling-upgrade, or glibc dependency changes. |
| Thrift/protobuf IDs | No gensrc changes. |
| Parallel paths | Direct, virtual-base, HNSW/IVF, buffered-build, and SNII callers checked. The unchanged smoke test already explicitly casts its pointer, so it has no equivalent direct-pointer compilation mismatch. |
| Conditional checks | No conditions or failure branches changed. |
| Test coverage | Existing tests cover success, wrong dimensions, zero rows, batching, build thresholds, large dimensions, and SNII round trips/failure propagation. Tests were inspected, not executed. |
| Test results | No expected-result files or assertions changed; initialized offset values are preserved and representable as uint64_t. |
| Observability | No new production operation requires logs or metrics. |
| Transactions and persistence | No transaction, EditLog, replay, failover, version-visibility, or delete-bitmap changes. Existing index persistence tests are preserved. |
| Writes and atomicity | Only test input declarations change; index write flow, failure propagation, and atomicity are unchanged. |
| FE/BE variables | No new transmitted variables or protocol paths. |
| Performance and memory | No algorithmic change, additional allocation, redundant computation, or new memory-accounting path. Buffer bounds and ownership are preserved. |
| Other issues and conventions | No new error-handling, nullable-column, include-layering, PCH, explicit-instantiation, unity-build, or test-order dependency issue identified. |
Review converged in round 1 after full changed-file coverage, independent review, and the final sweep. No unresolved candidates or uncovered changed paths remain. Validation limit: static review only, as required; no builds or tests were run, and the author's macOS execution results were not independently reproduced.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28558 ms |
Contributor
TPC-DS: Total hot run time: 150901 ms |
Contributor
ClickBench: Total hot run time: 24.43 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
approved these changes
Oct 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #69070
Problem Summary:
In short. Since #69070, the BE unit tests no longer compile on macOS: three ANN tests pass
std::vector<size_t>::data()where the writer now takesconst uint64_t*. The tests now usestd::vector<uint64_t>.Background. #69070 changed the offsets parameter of the ANN index writers'
add_array_values()fromconst uint8_t*toconst uint64_t*. Thevector_search_testlibrary, which holds the ANN tests, is force-loaded intodoris_be_test.The problem, and what it cost.
ann_index_writer_test.cpp(15 places),ann_index_edge_case_test.cppandsnii_ann_container_test.cppstill build their offsets asstd::vector<size_t>. On Linuxsize_tanduint64_tare bothunsigned long, so they compile. On macOSsize_tisunsigned longanduint64_tisunsigned long long, and the build stops at:Because
vector_search_testis linked intodoris_be_test, no BE unit test can be built on macOS. The macOS CI pipeline builds the BE without tests, so CI does not report it.How this PR fixes it. The 17 offsets vectors in those tests are declared as
std::vector<uint64_t>, the type the writer takes. Production code is unchanged.Results.
ann_index_edge_case_test.cppdoris_be_test; the ANN tests passRelease note
None
Check List (For Author)
AnnIndexWriterTest(27),VectorSearchTest(66) andSniiAnnContainerTest(20) pass.Check List (For Reviewer who merge this PR)