Repository navigation
[fix](scan) Disable unsafe scan prelimits for row filters - #65916
Open
Hastyshell wants to merge 1 commit into
Open
Hastyshell wants to merge 1 commit into
Hastyshell wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
run buildall |
Hastyshell
force-pushed
the
codex/ann-topn-prefilter-rf
branch
from
July 22, 2026 12:10
4243a6f to
8a501f3
Compare
Collaborator
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Disables ANN TopN pushdown when residual scan predicates remain to prevent under-returning rows, and adds a regression test covering the residual-predicate + ANN TopN scenario.
Changes:
- Reset/disable ANN TopN runtime when residual scan conjuncts are present after conjunct processing.
- Add a new regression test suite and golden output verifying correct results with residual predicates.
- Configure session variables in the test to exercise the ANN path deterministically (as intended).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| be/src/exec/operator/olap_scan_operator.cpp | Disables ANN TopN runtime when residual scan conjuncts remain to ensure correctness. |
| regression-test/suites/ann_index_p0/ann_topn_residual_predicate.groovy | Adds regression coverage for ANN TopN + residual predicate correctness. |
| regression-test/data/ann_index_p0/ann_topn_residual_predicate.out | Adds expected output for the new regression query. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29179 ms |
Contributor
TPC-H: Total hot run time: 29528 ms |
Contributor
TPC-DS: Total hot run time: 177750 ms |
Contributor
ClickBench: Total hot run time: 25.03 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Hastyshell
marked this pull request as ready for review
July 22, 2026 14:56
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Collaborator
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.
Request changes: the ANN reset is sound for genuine scanner residuals, but the functionally parallel ScoreRuntime/BM25 path can still pre-limit rows before those same residuals (inline comment).
Critical checkpoint conclusions:
- Goal and scope: clearing
_ann_topn_runtimeafter normalization correctly prevents ANN per-segment TopN from running before residual_conjuncts; the retained upper TopN and independent virtual-distance expression preserve final ordering and limit semantics. The change is small and focused for ANN. - Parallel paths: BM25
score()TopN remains unsafe when an indexed MATCH/SEARCH is consumed but a scanner-only residual remains. Both score pre-limit stages need the same residual gate. - Concurrency and lifecycle:
_process_conjuncts()runs before scanner construction, so serial and parallel scanner/tablet/segment copies all observe the ANN reset without a new lock or ownership hazard. A late join runtime filter cannot target this direct ANN scan because the retained physical TopN is a runtime-filter pushdown barrier; ready filters are normalized before the guard. - Error, memory, compatibility, and persistence: no ignored status, ownership cycle, configuration, thrift/storage-format, transaction, persisted-state, or mixed-version change was found.
- Tests and results: the new output is deterministic and follows suite conventions, but
abs(id) > 2is moved to common-expression pushdown on this DUP_KEYS table, whose existing fallback already disables ANN TopN. Thus the test can pass before this patch and does not exercise the new guard; existing discussion r3630166258 already covers that path-proof gap, so I did not duplicate it. Current PR checks report P0 Regression, BE UT, compile, formatting/checkstyle, and coverage passing. - Performance and observability: disabling ANN only for true residuals is a correctness-first fallback; existing ANN profile counters are sufficient, while the score fix should add a causal path assertion or counter.
- User focus: no additional focus was supplied.
Validation was static only, as required by the review runner; no local build or test command was run.
Hastyshell
force-pushed
the
codex/ann-topn-prefilter-rf
branch
from
July 23, 2026 02:11
8a501f3 to
c202c0f
Compare
Hastyshell
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
July 23, 2026 02:11
Hastyshell
force-pushed
the
codex/ann-topn-prefilter-rf
branch
from
July 23, 2026 02:26
c202c0f to
92c5df1
Compare
Collaborator
Author
|
/review |
Hastyshell
force-pushed
the
codex/ann-topn-prefilter-rf
branch
from
July 23, 2026 02:29
92c5df1 to
ea6b46b
Compare
Collaborator
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Request changes: the live head fixes ANN and score pre-limits when true scanner residuals remain, but score pre-limit can still rank rows before storage visibility and predicate restrictions; see the inline P1.
Critical checkpoint conclusions:
- Goal and results: resetting ANN and zeroing the ScoreRuntime pre-limit for residual
_conjunctsare correct. The goal remains incomplete because restrictions outside_conjunctscan still remove selected score leaders. - Scope and metadata: the implementation is localized, but the PR title/body remain ANN-only despite the live score-runtime and score-test additions.
- Concurrency and lifecycle:
_process_conjuncts()runs before scanner construction and pointer fan-out, so no race, lock, ownership, or lifetime defect was found. Ready runtime filters are acquired before this guard, and the retained PhysicalTopN blocks a semantically required late join runtime filter. - Parallel paths and conditions: a zero limit disables both SEARCH
query_limitand BM25 top-k while preserving score materialization and the upper TopN. Scan keys, remaining common expressions, persisted rowset delete predicates, and MOW delete bitmaps are nevertheless invisible to this guard; the inline comment covers their shared invariant. - Error and memory handling: no ignored Status, nullability issue, allocation/accounting problem, or ownership cycle was introduced.
- Configuration, compatibility, persistence, and writes: there is no production configuration, wire/storage-format, EditLog, transaction, write-path, or mixed-version change. The MOW case is a read-time visibility boundary.
- Tests and results: the two score cases genuinely exercise both residual score consumers and have deterministic output. The ANN result is correct but does not prove the new branch; existing discussion
r3630166258already covers that gap. Key/common/delete cases remain missing. - Performance and observability: full scoring is required whenever a collector lacks the final eligible candidate set; the normal no-residual fast path remains, and no separate metric or profile-counter gap was found.
- CI immediately before submission: 11 checks passed; macOS BE UT, BE UT, compile, performance, and code-review were pending; none failed.
- User focus: no additional focus was supplied.
Validation was static only, as required by the review runner; no local build or test command was run.
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29758 ms |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Storage TopN prelimit is unsafe when a lower-level collector ranks rows before all row restrictions have been applied. ANN TopN can keep only the nearest topK segment rows before scanner residual conjuncts run. BM25 score() has two similar prelimit stages: SEARCH DSL can set IndexQueryContext::query_limit before storage key/common/delete restrictions are applied, and score materialization can shrink the row bitmap before remaining batch predicates or rowset delete predicates run. These paths may under-return when the selected topK rows are later filtered out even though more candidates could satisfy the final LIMIT.
Disable ANN TopN runtime whenever residual scan conjuncts remain. For score(), preserve ScoreRuntime so score() is still materialized, but set its limit to zero when residual scan conjuncts remain. In SegmentIterator, keep SEARCH query_limit disabled by default and enable it only for a pure SEARCH expression over the full segment with no other row restrictions. Also use BM25 topK score materialization only after the row bitmap represents the final visible predicate-qualified candidate set; otherwise materialize full scores and let the upper TopN choose the final rows.
### Release note
Fix ANN and BM25 score TopN queries to avoid under-returning rows when scan filters, storage predicates, or delete predicates remove preselected candidates.
### Check List (For Author)
- Test: Manual test
- git diff --check
- Added regression test ann_topn_residual_predicate with expected result file, not run locally per request
- Added regression test test_search_score_residual_predicate with expected result file covering residual, key range, common expression, and delete predicate cases, not run locally per request
- build-support/clang-format.sh was attempted but could not run because llvm@16 is not installed locally
- Behavior changed: Yes. Unsafe ANN and BM25 score prelimit paths are disabled when they cannot rank the final visible candidate set; score() materialization and final upper TopN semantics are preserved.
- Does this need documentation: No
Hastyshell
force-pushed
the
codex/ann-topn-prefilter-rf
branch
from
July 23, 2026 03:34
ea6b46b to
d160bbb
Compare
Hastyshell
changed the title
Contributor
TPC-DS: Total hot run time: 177568 ms |
Contributor
ClickBench: Total hot run time: 25.29 s |
yiguolei
reviewed
Jul 23, 2026
| !_col_predicates.empty()) { | ||
| return false; | ||
| } | ||
| if (_common_expr_ctxs_push_down.size() != 1) { |
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: close None
Related PR: None
Problem Summary: Storage TopN prelimit is unsafe when a lower-level collector ranks rows before all row restrictions have been applied. ANN TopN can keep only the nearest topK segment rows before scanner residual conjuncts run. BM25 score() has two similar prelimit stages: SEARCH DSL can set IndexQueryContext::query_limit before storage key/common/delete restrictions are applied, and score materialization can shrink the row bitmap before remaining batch predicates or rowset delete predicates run. These paths may under-return when the selected topK rows are later filtered out even though more candidates could satisfy the final LIMIT.
This PR disables ANN TopN runtime whenever residual scan conjuncts remain. For score(), it preserves ScoreRuntime so score() is still materialized, but disables score prelimit when residual scan conjuncts remain. SegmentIterator now keeps SEARCH query_limit disabled by default and enables it only for a pure SEARCH expression over the full segment with no other row restrictions. BM25 topK score materialization is used only after the row bitmap represents the final visible predicate-qualified candidate set; otherwise Doris materializes full scores and lets the upper TopN choose the final rows.
The regression coverage is result-based and covers ANN residual predicates plus BM25 score residual, key range, common expression, and delete predicate cases.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)