Implement BAAT for long queries#3378
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a BlockAtATimeIterator (BAAT) implementation and enum values, integrates BAAT selection into OR query creation with an eligibility heuristic, refactors iterator-selection switches to group BMW and BAAT, and exposes child overrides to force score thresholds. ChangesBlock-At-A-Time Iterator for Long OR Queries
Sequence DiagramsequenceDiagram
participant OrQueryNode
participant BlockAtATimeIterator
participant BlockMaxLeafIterator
participant AccumulatorTopK
OrQueryNode->>BlockAtATimeIterator: construct(sub_doc_iters, topn)
BlockAtATimeIterator->>BlockMaxLeafIterator: Reset / ForceSetScoreThreshold(0.0)
BlockAtATimeIterator->>BlockMaxLeafIterator: Process posting blocks (SIMD or scalar)
BlockMaxLeafIterator->>AccumulatorTopK: accumulate(doc_id, score, match_cnt)
AccumulatorTopK->>BlockAtATimeIterator: update running top-k threshold
BlockAtATimeIterator->>AccumulatorTopK: ExtractTopK() -> topk_results
BlockAtATimeIterator->>OrQueryNode: return next doc >= requested with score >= threshold
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/storage/invertedindex/search/block_at_a_time_iterator_impl.cppsrc/storage/invertedindex/search/block_at_a_time_iterator_impl.cpp:15:1: error: a type specifier is required for all declarations ... [truncated 2200 characters] ... stopping now [-ferror-limit=] Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/storage/invertedindex/search/block_at_a_time_iterator_impl.cpp`:
- Around line 172-176: The current heuristic pruning (if (threshold_ > 0.0f &&
block_max_score <= threshold_ * 0.01f) { ... }) can drop documents that would
reach the threshold after other term accumulations; remove or replace it with a
provably-admissible upper bound check: compute a conservative block upper bound
by summing per-term maximum contributions for that block (use available
per-term/per-block max structures or add them), and only skip the block when
block_upper_bound <= threshold_. Update the code around variables
block_max_score, threshold_, target, block_last and the BlockAtATimeIteratorImpl
logic to use the summed per-term block maxima (or disable this heuristic) and
adjust total_blocks_skipped_ accounting accordingly.
- Around line 129-133: Next(RowID) currently advances using topk_cursor_ over
topk_results_ which are sorted by score, so entries with smaller doc IDs can be
skipped; change the logic in the Next(RowID) method to scan topk_results_ (not
just from topk_cursor_) and pick the smallest candidate_doc >= requested doc_id
with candidate_score > threshold_, update doc_id_ to that value, and mark that
entry as consumed (or remove it) so it won't be returned again; use the symbols
topk_results_, topk_cursor_, threshold_, doc_id_ and the Next(RowID) function to
locate and replace the linear consumption loop with a
full-scan-and-select-min-doc strategy (or alternatively sort a secondary index
by doc_id before seeking) so no valid hits are dropped.
In `@src/storage/invertedindex/search/query_node_impl.cpp`:
- Around line 686-690: Replace the release-only assert with a runtime
validation: in the EarlyTermAlgo::kBlockAtATime branch, check params.topn at
runtime and if it is 0 either throw a clear std::invalid_argument (mentioning
EarlyTermAlgo::kBlockAtATime and params.topn) or return a safe empty/no-op
iterator instead of constructing BlockAtATimeIterator; this prevents later
crashes in BlockAtATimeIterator::ExtractTopK when heap top is accessed with
topn==0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1458e610-c9cc-441e-9e30-28667eeb3a0f
📒 Files selected for processing (5)
src/executor/operator/physical_match_impl.cppsrc/storage/invertedindex/search/block_at_a_time_iterator.cppmsrc/storage/invertedindex/search/block_at_a_time_iterator_impl.cppsrc/storage/invertedindex/search/doc_iterator.cppmsrc/storage/invertedindex/search/query_node_impl.cpp
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/storage/invertedindex/search/block_at_a_time_iterator_impl.cpp (2)
172-179:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReturn the selected top-k in doc-id order before exposing them via
Next(RowID).
ExtractTopK()materializestopk_results_in score order, butNext(RowID)consumes that vector with a monotoniccd >= doc_idscan. Once a higher doc ID is returned first, any remaining lower doc IDs are skipped permanently, so valid hits can disappear from the iterator.Also applies to: 336-340
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/invertedindex/search/block_at_a_time_iterator_impl.cpp` around lines 172 - 179, ExtractTopK() currently fills topk_results_ in score order, but Next(RowID) iterates with a monotonic doc-id scan using topk_cursor_, causing lower docIDs to be permanently skipped; fix by sorting topk_results_ by document id (the cd field) in ascending order after materialization (and before resetting/using topk_cursor_), and apply the same sort in the other ExtractTopK-like code path referenced around the later block (lines 336-340); ensure topk_cursor_ is reset to 0 after sorting so Next(RowID) returns hits in doc-id order.
212-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe 10% block-pruning rule is not a safe upper bound.
block_max_score * 10.0f <= threshold_can still prune documents that already accumulated most ofthreshold_from earlier terms and only need a small contribution from this block. That makes the pruning lossy for exact top-k.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/invertedindex/search/block_at_a_time_iterator_impl.cpp` around lines 212 - 220, The 10% heuristic (block_max_score * 10.0f <= threshold_) can prune valid top-k candidates; remove or disable this unsafe rule and stop skipping blocks based on that multiplication. Replace the condition in the block-level pruning (the code using threshold_, block_max_score computed from leaf->BlockMaxBM25Score(), and incrementing total_blocks_skipped_) with a conservative no-op until a correct per-block accumulator upper-bound is available — either remove the if-branch entirely or only prune when you can provably guarantee no document in the block can reach threshold (i.e., use a correct accumulated-prefix upper bound), and update total_blocks_skipped_ only when pruning is safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/storage/invertedindex/search/block_at_a_time_iterator_impl.cpp`:
- Around line 201-218: The loop currently calls leaf->Next(target) before
checking block-level pruning, which forces per-doc decode/seek even when a whole
block should be skipped; change the logic to advance the block cursor with
BlockMaxLeafIterator::NextShallow() (or call NextShallow() on the same leaf
type) to move to the next block and examine BlockMinPossibleDocID(),
BlockLastDocID(), and BlockMaxBM25Score() before decoding docs with
leaf->Next(target); only call leaf->Next(target) if the block passes the
block-level score/threshold check (use the existing threshold_ logic and
BlockMaxBM25Score() to decide), and preserve the same uses of target,
BlockLastDocID(), and BlockMinPossibleDocID() when entering a block.
- Around line 110-115: ProcessTerm currently attempts to disable child-side
thresholding by calling leaf->UpdateScoreThreshold(0.0f) but
TermDocIterator::UpdateScoreThreshold is monotonic so that won't lower an
already-increased threshold; change the fix by adding a non-monotonic API on the
child iterator (e.g., TermDocIterator::ForceSetScoreThreshold(float) or
TermDocIterator::ResetScoreThreshold()) that unconditionally sets or clears the
child threshold, then call that new API from BlockAtATimeIterator::ProcessTerm
(and the other spots noted around lines 193-195) instead of calling
UpdateScoreThreshold(0.0f); update callers to use the new Force/Reset method so
child-side thresholding can be disabled reliably.
- Around line 129-140: The code currently excludes documents whose score equals
threshold_ by using strict comparisons; change all comparisons that drop ties so
equal scores are kept: replace "entry.score <= threshold_" with "entry.score <
threshold_" and replace "entry.score > running_topk_heap_.top().second" with
"entry.score >= running_topk_heap_.top().second" (apply the same edits in the
other occurrences around the ExtractTopK() / heap rebuild logic where
accumulator_, running_topk_heap_, threshold_, and topn_ are used) so documents
that tie the kth score are not filtered out.
---
Duplicate comments:
In `@src/storage/invertedindex/search/block_at_a_time_iterator_impl.cpp`:
- Around line 172-179: ExtractTopK() currently fills topk_results_ in score
order, but Next(RowID) iterates with a monotonic doc-id scan using topk_cursor_,
causing lower docIDs to be permanently skipped; fix by sorting topk_results_ by
document id (the cd field) in ascending order after materialization (and before
resetting/using topk_cursor_), and apply the same sort in the other
ExtractTopK-like code path referenced around the later block (lines 336-340);
ensure topk_cursor_ is reset to 0 after sorting so Next(RowID) returns hits in
doc-id order.
- Around line 212-220: The 10% heuristic (block_max_score * 10.0f <= threshold_)
can prune valid top-k candidates; remove or disable this unsafe rule and stop
skipping blocks based on that multiplication. Replace the condition in the
block-level pruning (the code using threshold_, block_max_score computed from
leaf->BlockMaxBM25Score(), and incrementing total_blocks_skipped_) with a
conservative no-op until a correct per-block accumulator upper-bound is
available — either remove the if-branch entirely or only prune when you can
provably guarantee no document in the block can reach threshold (i.e., use a
correct accumulated-prefix upper bound), and update total_blocks_skipped_ only
when pruning is safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e545eb5-0fa7-48f4-9dc0-c2c4eb0a0411
📒 Files selected for processing (2)
src/storage/invertedindex/search/block_at_a_time_iterator.cppmsrc/storage/invertedindex/search/block_at_a_time_iterator_impl.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/storage/invertedindex/search/block_at_a_time_iterator.cppm
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@CodeRabbit review |
✅ Action performedReview finished.
|
1 participant
What problem does this PR solve?
Algorithm Overview
Type of change