[opt](Ann) Cancel index building if input rows is less than the min_train_rows - #60358
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 32707 ms |
ClickBench: Total hot run time: 28.28 s |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 31776 ms |
ClickBench: Total hot run time: 28.48 s |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 33229 ms |
ClickBench: Total hot run time: 28.19 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 31816 ms |
ClickBench: Total hot run time: 28.4 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 31888 ms |
ClickBench: Total hot run time: 28.26 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
RoanHeNaN
marked this pull request as ready for review
January 31, 2026 14:55
RoanHeNaN
marked this pull request as draft
February 1, 2026 01:13
RoanHeNaN
marked this pull request as ready for review
February 3, 2026 08:19
Member
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
PR: WIP Cancel index building if input rows is less than the min_train_rows
This PR adds logic to skip ANN index building when training data is insufficient, with fallback to brute-force search at query time. The core idea is sound but several issues should be addressed before merging.
Critical Issues
-
Excessive
[DEBUG]LOG(INFO) inindex_file_reader.cpp: ~15 verbose debug log statements at INFO level will flood production logs on every index file read operation. These must be removed or downgraded to VLOG_DEBUG before merge. This file is in the hot path for ALL inverted index reads, not just ANN. -
Lost load timing metrics: When load_index was moved from
query()/range_search()totry_load_index(), theSCOPED_TIMER(&(stats->load_index_costs_ns))andDorisMetrics::instance()->ann_index_load_costs_mstracking were dropped entirely. Load cost is no longer being measured. -
Stale LOG_INFO in
ann_index_iterator.cpp:52:LOG_INFO("_context of ann index iterator is {}", ...)fires on every query call and should be removed.
Minor Issues
-
SQ comment/value mismatch in
faiss_ann_index.cpp: Comment says "use a minimum of 20 training vectors" but code setsquantizer_min = 1. -
Unreachable code in
ann_index_writer.cpp:198:return Status::OK()after the if/else block is dead code since all branches already return. -
_float_arraynot cleared infinish(): Unlike the original code, the new code doesn't call_float_array.clear()after processing, keeping memory allocated unnecessarily.
Observations
- The
_need_save_indexflag +min_train_rowslogic is well-structured for handling the edge cases (empty segment, small remainder after chunks, fully small segment). - Adding
try_load_index()to gracefully handle missing index files (when building was skipped) is the right approach. - Good test coverage for the writer logic with multiple edge case scenarios.
- The PR title says [WIP] which may indicate these debug logs are intentionally temporary, but they should definitely not be merged to master.
|
|
||
| LOG(INFO) << "[DEBUG] IndexFileReader::_init_from start, index_path_prefix: " | ||
| << _index_path_prefix << ", index_file_full_path: " << index_file_full_path | ||
| << ", read_buffer_size: " << read_buffer_size; |
Contributor
There was a problem hiding this comment.
Critical: Remove debug logging before merge. This file has ~15 [DEBUG] LOG(INFO) statements that will fire on every index file read (not just ANN index). This is the hot path for all inverted index operations and will flood production logs. These should be removed entirely or downgraded to VLOG_DEBUG.
|
|
||
| // _context may be unset in some test scenarios; pass nullptr IOContext in that case. | ||
| io::IOContext* io_ctx = (_context != nullptr) ? _context->io_ctx : nullptr; | ||
| LOG_INFO("_context of ann index iterator is {}", (_context != nullptr) ? "not null" : "null"); |
Contributor
There was a problem hiding this comment.
Remove this LOG_INFO. This fires on every ANN query call and provides no diagnostic value in production. It was likely added for debugging.
| double load_costs_ms = static_cast<double>(stats->load_index_costs_ns.value()) / 1000.0; | ||
| DorisMetrics::instance()->ann_index_load_costs_ms->increment( | ||
| static_cast<int64_t>(load_costs_ms)); | ||
| Status st = load_index(io_ctx); |
Contributor
There was a problem hiding this comment.
Lost metrics tracking. The original code had:
SCOPED_TIMER(&(stats->load_index_costs_ns));
RETURN_IF_ERROR(load_index(io_ctx));
double load_costs_ms = ...
DorisMetrics::instance()->ann_index_load_costs_ms->increment(...);By moving load to try_load_index() (which has no stats parameter), both load_index_costs_ns timing and ann_index_load_costs_ms metrics are completely lost. You should either pass the stats object to try_load_index() or record the timing at the call site in segment_iterator.cpp.
Member
There was a problem hiding this comment.
@zhiqiang-hhhh need check this
Contributor
Author
There was a problem hiding this comment.
| } else if (_params.quantizer == FaissBuildParameter::Quantizer::SQ4 || | ||
| _params.quantizer == FaissBuildParameter::Quantizer::SQ8) { | ||
| // For SQ, use a minimum of 20 training vectors, similar to IVF's nlist * 2 with nlist=10 | ||
| quantizer_min = 1; |
Contributor
There was a problem hiding this comment.
Comment/code mismatch. Comment says "use a minimum of 20 training vectors, similar to IVF's nlist * 2 with nlist=10" but the code sets quantizer_min = 1. Should this be quantizer_min = 20? Or if 1 is correct, the comment should be updated.
| } | ||
|
|
||
| return _vector_index->save(_dir.get()); | ||
| return Status::OK(); |
Contributor
There was a problem hiding this comment.
Dead code. This return Status::OK() is unreachable because every branch in the if (_float_array.empty()) ... else ... block above already returns. Consider removing it to avoid confusion.
| // It happens to have not enough data to train. | ||
| // If we have data to add before, we still need to save the index. | ||
| if (_need_save_index) { | ||
| RETURN_IF_ERROR(_vector_index->add(num_rows, _float_array.data())); |
Contributor
There was a problem hiding this comment.
Potential correctness concern: When _need_save_index == true (previous chunks were trained+added) but the remaining rows are fewer than min_train_rows, you call add() without train(). For IVF indexes, this means the remaining vectors are added to an index whose quantizer was trained on previous batches. This is generally fine for FAISS since the quantizer is already trained, but it means these vectors won't benefit from the last batch's clustering. Worth a comment explaining this is intentional.
Member
There was a problem hiding this comment.
@zhiqiang-hhhh need check this
Contributor
Author
There was a problem hiding this comment.
@zhiqiang-hhhh need check this
A comment has been added.
RoanHeNaN
added a commit
to RoanHeNaN/doris
that referenced
this pull request
Mar 3, 2026
1. Remove excessive [DEBUG] LOG(INFO) statements from index_file_reader.cpp that would flood production logs 2. Restore load timing metrics tracking by adding SCOPED_TIMER and DorisMetrics in segment_iterator.cpp and vectorized_fn_call.cpp 3. Remove stale LOG_INFO in ann_index_iterator.cpp that fires on every query 4. Fix SQ quantizer comment/value mismatch in faiss_ann_index.cpp 5. Remove unreachable return Status::OK() in ann_index_writer.cpp 6. Add _float_array.clear() calls to avoid keeping memory allocated unnecessarily 7. Add comment explaining IVF correctness when adding vectors without training
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 28677 ms |
TPC-DS: Total hot run time: 184982 ms |
RoanHeNaN
force-pushed
the
opt-index-train
branch
from
March 20, 2026 03:30
fe7291e to
7d4eebc
Compare
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 27019 ms |
TPC-DS: Total hot run time: 168267 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
yiguolei
approved these changes
Mar 25, 2026
HappenLee
pushed a commit
to HappenLee/incubator-doris
that referenced
this pull request
Mar 31, 2026
…rain_rows (apache#60358) Before this change, when the amount of data used to train the index was less than the required amount, import or compaction might fail, which severely impacted user experience. Now, in such cases, it automatically determines whether training and index generation are needed. When the amount is completely insufficient, index construction is skipped, and during queries, it falls back to brute-force computation. For the calculation of min_train_rows: 1. IVF requires no less than nlist rows. 2. PQ requires no less than 2^pq_nbits * 100 rows. Take the max of the two as the required minimum number of rows. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
RoanHeNaN
added a commit
to RoanHeNaN/doris
that referenced
this pull request
Apr 8, 2026
…rain_rows (apache#60358) Before this change, when the amount of data used to train the index was less than the required amount, import or compaction might fail, which severely impacted user experience. Now, in such cases, it automatically determines whether training and index generation are needed. When the amount is completely insufficient, index construction is skipped, and during queries, it falls back to brute-force computation. For the calculation of min_train_rows: 1. IVF requires no less than nlist rows. 2. PQ requires no less than 2^pq_nbits * 100 rows. Take the max of the two as the required minimum number of rows. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
RoanHeNaN
added a commit
to RoanHeNaN/doris
that referenced
this pull request
Apr 8, 2026
yiguolei
pushed a commit
that referenced
this pull request
Apr 9, 2026
…es (#62215) ## Summary - backport PR #60358, #61160 and #62178 into branch-4.1 as a single commit - add IVF on-disk ANN index support, related cache/runtime changes, and FE session/property updates - bring over ANN regression coverage updates for IVF, IVF on-disk, small-segment and min-train-rows scenarios
7 tasks
morningman
added a commit
to morningman/doris
that referenced
this pull request
Aug 5, 2026
…lect compaction profile BE by tablet replica apache#62178 apache#65552 Backport two upstream regression-test fixes that never reached branch-4.0. Both cause recurring failures in the daily branch-4.0 P0 pipeline. 1. ann_index_basic vs ivf_index_test table-name collision (apache#62178) ann_index_basic and ivf_index_test run in the same regression database (ann_index_p0) and both used tbl_ann_l2 / tbl_ann_ip. With suiteParallel=10 they can run concurrently: FE logs of the failing run show ivf_index_test dropping and recreating tbl_ann_ip 230ms after ann_index_basic created it, then inserting 6 rows. ann_index_basic then reads the neighbor's table by name and fails. This also explains the historical intermittent empty result of sql_ip_asc (query landing between the neighbor's create and publish); the insert itself publishes in ~80ms, so the visibility-window theory behind the waitRowsVisible gate (apache#65942) was wrong, and the 30s gate now times out against the neighbor's 6-row table instead. Rename the shared tables with basic_/ivf_ prefixes as upstream did in 9c226f5 (apache#62178) and drop the gate. ann_index_basic.groovy becomes byte-identical to the upstream post-fix file. ivf_index_test takes only the renames because the upstream file also carries the apache#60358 behavior change (insufficient train rows no longer throws) which branch-4.0 BE does not have. No .out changes needed. 2. test_compaction_profile_action queries an arbitrary BE (apache#65552) The suite built the /api/compaction/profile URL from backendId_to_backendIP.keySet()[0]. On a multi-BE pipeline (4 BEs, replication forced to 3) the chosen BE has ~1/4 chance of not hosting the tablet replica, so the tablet_id filter returns an empty list (observed 5/26 failures; the case is currently muted as DORIS-26131). Derive the endpoint from the BackendId of the selected tablet row as upstream did in fd16ebd (apache#65552); the file becomes byte-identical to the upstream post-fix version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
6 participants
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.
Before this change, when the amount of data used to train the index was less than the required amount, import or compaction might fail, which severely impacted user experience. Now, in such cases, it automatically determines whether training and index generation are needed. When the amount is completely insufficient, index construction is skipped, and during queries, it falls back to brute-force computation.
For the calculation of min_train_rows:
Take the max of the two as the required minimum number of rows.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)