[fix](score) disable search topn with extra predicates - #65821
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
changed the title
|
run buildall |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
TPC-H: Total hot run time: 29747 ms |
TPC-DS: Total hot run time: 179221 ms |
ClickBench: Total hot run time: 24.85 s |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Request changes. The patch correctly disables early SEARCH Top-K when the logical filter contains explicit residual predicates, and the zero sentinel remains present through FE translation and switches the current/base BE paths to full SEARCH and score collection. However, the preserved lone-SEARCH branch is still unsafe on supported unique-key MOW reads: SEARCH selects K physical documents before the segment delete bitmap removes hidden versions, so a deleted highest scorer can suppress the next visible row. That is a reachable wrong-result path and needs to be addressed before merge.
The new regression suite is broad and its expected rankings/results are deterministic, but it does not cover the MOW visibility case. Two additional test gaps are called out inline conditionally: they matter only if the fix retains a selective positive-limit/recursive-classifier branch.
Review checkpoints
- Goal and correctness: Explicit extra-predicate cases are substantially fixed, but the supported storage-visibility path above leaves the goal incomplete.
- Scope and maintainability: The production change is small and focused; the helper is readable and reuses existing expression traversal.
- Concurrency and lifecycle: No new locking, shared mutable state, thread-lifecycle, or static-initialization behavior is introduced.
- Configuration and compatibility: No configuration, protocol, or storage-format change is introduced. The existing optional score-limit field and zero semantics are compatible with the target-base BE path checked for this review.
- Parallel execution paths: Direct/nested SEARCH, MATCH-only scoring, score-range extraction, LIMIT/OFFSET, multiple SEARCH predicates, row ranges, delete conditions, and MOW delete bitmaps were traced. MATCH-only and the explicit predicate classifier are sound; post-SEARCH visibility is the blocking gap.
- Transactions, persistence, and writes: No write/transaction path changes, but read visibility for committed MOW versions is affected by the blocking issue.
- Error handling and observability: No new exception/nullability issue was found. Existing EXPLAIN output exposes
SCORE SORT LIMIT, which can test the material positive-versus-zero planner choice if that choice remains. - Performance: Zero intentionally uses full document/score collection, so it costs more work but is the necessary correctness fallback until a positive limit can account for all later masks.
- Tests and CI: Changed result files match the suite and exercise indexed/plain predicates, MATCH, ranges, multiple/nested/negated SEARCH, score filtering, and OFFSET. Compile, CheckStyle, FE UT, and the relevant P0 regression checks are green. No local build or test was run because this review environment designates the bundle as review-only.
- User focus: No additional review focus was provided.
| if (!hasSearchPredicate) { | ||
| return false; | ||
| } | ||
| return nonScoreConjuncts.size() > 1 || !(nonScoreConjuncts.get(0) instanceof SearchExpression); |
There was a problem hiding this comment.
[P1] Account for storage visibility before keeping SEARCH Top-K
A lone SEARCH is not sufficient to make early Top-K safe. For example:
TopN(score DESC, LIMIT 1)
Project(id, score() AS score)
Filter(search('title:apple'))
Scan(unique-key MOW table)
This branch keeps score_sort_limit = 1. In BE, SegmentIterator::_lazy_init() evaluates SEARCH in _get_row_ranges_by_column_conditions() before subtracting _opts.delete_bitmap. If a deleted old version has the highest score and a live row is second, SEARCH returns only the deleted row; the later bitmap subtraction removes it, and the upper TopN cannot recover the live runner-up. SEARCH is supported on MOW tables, so this is reachable even with no extra SQL conjunct. Please either disable the early limit whenever post-SEARCH visibility/range filters may exist, or apply those masks before SEARCH selects Top-K, and add a deleted-highest-score regression.
There was a problem hiding this comment.
I checked the MOW case with EXPLAIN, and the actual plan does not keep a positive score_sort_limit for a SQL-level lone SEARCH on a unique-key MOW table.
For example:
SELECT id, score() AS s
FROM test_search_score_mow_topk_visibility
WHERE search('title:alpha')
ORDER BY s DESC
LIMIT 5;
produces:
SCORE SORT LIMIT: 0
PREDICATES: (search('title:alpha') AND (DORIS_DELETE_SIGN = 0))
So although the SQL text contains only one SEARCH predicate, the scan predicate is not a lone SEARCH in a unique-key table. Doris injects the hidden delete-sign predicate, and this PR classifies it as SEARCH plus an extra predicate, so the early SEARCH Top-K limit is disabled.
I agree that if a unique-key/MOW scan kept a positive score_sort_limit, it would be unsafe because SEARCH Top-K is evaluated before post-SEARCH visibility filters such as delete bitmap / delete-sign filtering. But that is not the actual plan produced by this PR for the MOW case. With SCORE SORT LIMIT: 0, BE takes the full doc-set collection path instead of collect_multi_segment_top_k(), and the upper TopN applies the final limit after visibility filtering.
I also tried to reproduce the deleted-highest-score case on a unique-key MOW table, comparing the SQL-level lone SEARCH query with a control query that forces SCORE SORT LIMIT: 0, and both returned the same correct visible rows. Could you provide a concrete MOW SQL/EXPLAIN where a unique-key MOW lone SEARCH still produces a positive SCORE SORT LIMIT? Otherwise, this P1 seems to be based on a plan shape that this PR does not generate.
There was a problem hiding this comment.
The MOW plan is protected by the injected delete-sign predicate, but that does not close the broader visibility concern. DUP_KEYS provides a concrete counterexample because those tables do not receive __DORIS_DELETE_SIGN__.
For example, insert two SEARCH-matching rows where id 1 scores above id 2, then execute DELETE FROM t WHERE id = 1. The later score query still has the reduced plan
TopN(score DESC, LIMIT 1)
Project(id, score() AS score)
Filter(search('title:alpha'))
Scan(DUP_KEYS table)
so this helper keeps SCORE SORT LIMIT: 1. BE attaches the legacy delete predicate to older rowsets in BetaRowsetReader, but SEARCH consumes IndexQueryContext::query_limit first in function_search.cpp; exact delete_condition_predicates evaluation happens later in SegmentIterator::_evaluate_short_circuit_predicate(). The deleted high scorer therefore consumes Top-1 and is then removed, while the live runner-up was never collected.
Please account for storage delete predicates (and any other late semantic mask) before retaining positive SEARCH Top-K, and add a deleted-highest-score DUP_KEYS regression. I am replying here instead of opening another inline thread because this is concrete evidence for the same invariant already raised above.
There was a problem hiding this comment.
This PR is scoped to the case where SEARCH is combined with other planner-visible predicates in the same FE plan, and the Top-K limit may be applied before those remaining predicates are evaluated. The DUP_KEYS + DELETE case is a separate storage-side visibility problem: the delete predicate is applied later than the SEARCH Top-K limit, so the FE rewrite in this PR cannot safely reason about it in the same way.
I’ve filed a separate JIRA to track that broader bug: http://jira.selectdb-in.cc/browse/DORIS-28025.
| sql "set enable_segment_limit_pushdown = true" | ||
| sql "set enable_inverted_index_query_cache = false" | ||
|
|
||
| qt_single_search """ |
There was a problem hiding this comment.
[P2] Cover the positive branch if it remains
These result-only queries cannot distinguish SCORE SORT LIMIT: 1 from the disable sentinel 0, because the upper TopN produces the same rows either way; the complete changed suite passes if the helper always returns zero. After addressing the visibility issue above, if any selective positive-limit branch remains, please add deterministic EXPLAIN assertions for that demonstrably safe case and for an extra-predicate zero case. If the fix removes positive SEARCH Top-K entirely, this assertion is unnecessary.
There was a problem hiding this comment.
The added regression cases are primarily correctness tests. They are constructed so that, for SEARCH plus residual predicates, keeping a positive SCORE SORT LIMIT would prune the high-score row before the residual predicate is applied and produce the wrong result. Therefore the expected results verify the correctness issue that this PR fixes.
| ORDER BY id | ||
| """ | ||
|
|
||
| qt_nested_search_with_other_predicate """ |
There was a problem hiding this comment.
[P2] Make the nested cases depend on recursive SEARCH detection
Neither named case fails with a shallow top-level-only classifier. In the OR case, category MATCH 'special' independently restores id 7 after SEARCH Top-1 keeps id 8; in the NOT case, the separate top-level search('body:beta') already causes the limit to be disabled. If the selective recursive classifier remains after the correctness fix, please make the alternative OR branch nonmatching and add a negated-only witness (or a direct FE rule test) so losing .collect(SearchExpression) changes the expected result.
| } | ||
|
|
||
| long scoreLimit = topN.getLimit() + topN.getOffset(); | ||
| long pushedScoreLimit = shouldDisableSearchTopN(filter.getConjuncts(), extractedScorePredicate) |
There was a problem hiding this comment.
please check overflow and do not push down like #64633
add a test case for it.
There was a problem hiding this comment.
Before that, the overflow has been checked:
if (Utils.addOverflows(topN.getLimit(), topN.getOffset())) { return null; }
If overflow occurs, an error will occur:“score() function requires WHERE clause with MATCH function, ORDER BY and LIMIT for optimization”，instead of “limit + offset overflows the long range”.
Do I need to change return null; to throw new AnalysisException("limit + offset overflows the long range");？
There was a problem hiding this comment.
The key is: return null (skip optimization) if no wrong result, else throw exception
94d0c14 to
a96afba
Compare
github-actions
Bot
removed
the
approved
|
/review |
|
run buildall |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
There was a problem hiding this comment.
Review summary
Request changes. The patch correctly disables early SEARCH Top-K for explicit residual predicates, preserves score materialization with the existing zero sentinel, and now reports limit/offset overflow with the same exception as ordinary TopN. However, the existing P1 visibility thread remains unresolved.
The hidden delete-sign predicate protects the UNIQUE_KEYS/MOW example discussed there, but not DUP_KEYS legacy delete predicates. A lone top-level SEARCH on a DUP_KEYS table still retains a positive score limit; BE evaluates SEARCH with that limit before applying the storage delete predicate. A deleted highest scorer can therefore consume Top-1 and be removed afterward, leaving the live runner-up unavailable. I replied to the existing thread with the concrete reduced plan and FE/BE ordering evidence. No new inline thread is added here because that would duplicate the existing P1.
Review checkpoints
- Goal, correctness, and proof: The explicit extra-predicate cases are fixed, but the retained lone-SEARCH branch still has a reachable wrong-result path on supported DUP_KEYS deletes, so the overall correctness goal is incomplete. The changed result cases are deterministic and exercise explicit residual predicates, but no deleted-highest-score regression covers this remaining path.
- Scope and maintainability: The production change is small and readable. The helper's normalized-expression truth table is conservative for multiple, residual, nested, and negated SEARCH predicates; its unsafe assumption is that the logical conjunct set represents every later semantic mask.
- Concurrency and lifecycle: No new shared mutable state, locking, thread lifecycle, resource lifecycle, or static initialization behavior is introduced.
- Configuration, compatibility, and propagation: No configuration, persisted state, storage format, or protocol field changes. FE reuses the existing score-limit field, and
0already selects BE's full-doc-set path, so mixed-path propagation is intact. - Parallel and special paths: SEARCH versus MATCH-only scoring, extracted min-score predicates, multiple/nested SEARCH, key ranges, parallel scanner row ranges, UNIQUE_KEYS/MOW delete-sign and bitmap handling, and DUP_KEYS legacy delete predicates were traced. Parallel row ranges are exhaustive work partitions; the blocking path is the semantic legacy-delete mask applied after SEARCH collection.
- Error handling and observability: Overflow now matches
LogicalTopNToPhysicalTopNexactly and the new exception regression distinguishes it from the prior unrelated score-usage error. EXPLAIN exposesSCORE SORT LIMIT, but the existing positive-versus-zero and recursive-classifier test-oracle gaps remain covered by the prior P2 threads. - Transactions, persistence, and writes: This PR does not change write or persistence code. It does affect read visibility after a committed DELETE, which is the blocking correctness issue.
- Performance: Full collection for limit
0is more expensive, but it is the required correctness fallback until all later semantic masks are incorporated before Top-K selection. - Tests and CI: The 12 changed ordered result cases and overflow case are internally consistent. Per the review-only instructions, no local build or test was run. At submission time CheckStyle and the lightweight GitHub checks pass; Doris compile and FE UT are still pending.
- User focus: No additional review focus was provided.
TPC-H: Total hot run time: 17492 ms |
TPC-DS: Total hot run time: 83904 ms |
ClickBench: Total hot run time: 14.62 s |
FE Regression Coverage ReportIncrement line coverage |
FE UT Coverage ReportIncrement line coverage |
…5821 (#67327) ### What problem does this PR solve? Issue Number: N/A Related PR: #65821 (master), picked from commit 2b6a45e Problem Summary: Backport of #65821 to branch-4.1. Search score TopN pushdown may return incorrect results when the search predicate is combined with additional predicates, because the pushed TopN limit can be applied before the remaining predicates are evaluated. This change disables the pushed search TopN limit in those cases while preserving the virtual score column pushdown, and adds regression coverage (search + equality / range / match / score range / multiple search predicates, plus limit+offset overflow). **Hunk audit (source diff → this PR):** | Source hunk | Status | |---|---| | `PushDownScoreTopNIntoOlapScan.java` `@@ -194,17 +194,22 @@` (overflow guard rework + pushedScoreLimit) | **Adapted ×2**: ① branch-4.1 never had the #64633 overflow-guard block, so the hunk's removed lines have no counterpart here; ② `Utils.addOverflows` does not exist on 4.1 (#64633 not backported) — inlined the equivalent check `topN.getLimit() > Long.MAX_VALUE - topN.getOffset()` (identical to the master helper's implementation). | | `PushDownScoreTopNIntoOlapScan.java` `@@ -243,6 +248,19 @@` (`shouldDisableSearchTopN` helper) | Ported | | `test_search_score_topn_predicates.out` (new) | Ported (verbatim) | | `test_search_score_topn_predicates.groovy` (new) | **Adapted**: dropped `set enable_segment_limit_pushdown = true` — the variable comes from #62222 which is not on 4.1; it defaults to true on master and only controls a BE-side segment limit optimization, unrelated to this FE-plan-level fix. | **Local verification on this branch:** full ASAN BE+FE build green; `run-regression-test.sh -d inverted_index_p0 -s test_search_score_topn_predicates` → 1 suite, 0 failed against a local 1FE+1BE cluster built from this PR. No FE UT exists for this rule on 4.1 and the source PR added none (its coverage is the regression suite above). ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [x] 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: - [x] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [x] 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 --> Co-authored-by: liangj777 <106017102+LIANG751234313@users.noreply.github.com>
4 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Search score TopN pushdown may return incorrect results when the search predicate is combined with additional predicates, because the pushed TopN limit can be applied before the remaining predicates are evaluated. This change disables the pushed search TopN limit in those cases while preserving the virtual score column pushdown, and adds regression coverage for search predicates combined with equality, range, match, score range and multiple search predicates.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)