Repository navigation
[fix](inverted index) Handle deleted documents in score TopN - #66979
Merged
Merged
Conversation
hoshinojyunn
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
August 20, 2026 07:00
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17405 ms |
Contributor
TPC-DS: Total hot run time: 83599 ms |
Contributor
ClickBench: Total hot run time: 14.67 s |
hoshinojyunn
force-pushed
the
fix/topN_delete_predicate
branch
from
August 20, 2026 09:10
871b036 to
6aad708
Compare
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hoshinojyunn
force-pushed
the
fix/topN_delete_predicate
branch
from
August 20, 2026 10:49
6aad708 to
75f1e55
Compare
hoshinojyunn
changed the title
hoshinojyunn
changed the title
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17610 ms |
Contributor
TPC-DS: Total hot run time: 85093 ms |
Contributor
ClickBench: Total hot run time: 14.73 s |
hoshinojyunn
force-pushed
the
fix/topN_delete_predicate
branch
from
August 20, 2026 11:55
75f1e55 to
ad8158a
Compare
Issue Number: None
Related PR: None
Problem Summary: A score-sorted query could truncate inverted-index hits or BM25 score materialization to its LIMIT before evaluating a delete predicate. A deleted high-score document then occupied the only TopN slot, so a lower-score visible document was never returned. When a rowset has delete predicates, directly disable both score TopN early-truncation points while retaining the existing fast path when no delete predicate is present. The regression verifies score order before deletion, visible results after deletion, and that a later data rowset is not affected by an earlier delete predicate.
Fixes incorrect results for score-sorted searches with delete predicates.
- Test: Regression test / Build
- ./build.sh --be -j 192
- ./run-regression-test.sh --conf /tmp/score-topn-delete-predicate.mz4MAv.groovy --run -g p0 -d search -s test_search_score_topn_delete_predicate -forceGenOut
- Behavior changed: Yes (score TopN is not truncated before delete predicates are evaluated)
- Does this need documentation: No
hoshinojyunn
force-pushed
the
fix/topN_delete_predicate
branch
from
August 20, 2026 12:05
ad8158a to
5b4c4e1
Compare
Contributor
Author
|
run buildall |
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.
Requesting changes for two substantive issues in the score-TopN eligibility/resource behavior and two test gaps.
Critical checkpoint conclusions:
- Goal and proof: the delete-predicate and MOW delete-bitmap mechanisms fix the named deleted-winner cases, but the early TopK candidate domain is still incomplete when ordinary filters or UNIQUE-MOR merge/delete-sign visibility reject winners later (MAIN-001). The added tests cover the three advertised deletion modes, but MAIN-003 and MAIN-004 leave the offset/block-WAND behavior and normal golden output unproved.
- Scope and clarity: the production change is localized and generally clear; the incomplete eligibility condition and unbounded fallback are the two material design issues.
- Concurrency and lifecycle: no new concurrent mutation or locking path is introduced. The delete bitmap is an immutable visible-version snapshot whose shared cache handle remains pinned through the iterator/query context; no ownership cycle or cross-TU static-initialization issue was found.
- Configuration and compatibility: no configuration, wire protocol, storage format, public symbol, or rolling-upgrade behavior changes. The collector API addition is internal and has a trailing default argument.
- Parallel paths and conditions: I traced SEARCH versus legacy MATCH, CLucene/SNII reader shapes, WAND/non-WAND, delete predicates, MOW bitmaps, ordinary/key/residual filters, and UNIQUE-MOR merge visibility. Runtime bitmap offsets and deleted-hit threshold feedback are sound, but the new delete-predicate-only eligibility fence does not cover every later visibility/filtering path.
- Tests and results: the author reports the targeted regression suite and BE unit test; I did not rerun them because this automated-review environment forbids builds. Header-hygiene checks and
git diff --checkpass. Deterministic regression results need the repository golden-output mechanism, and the unit fixture needs nonzero-base plus genuine block-WAND coverage. - Error, memory, nullability, and observability: status/error and nullable-column handling are unchanged. Existing index/row-filter statistics are adequate for correctness investigation, but MAIN-002 introduces full-match score-map/vector materialization without a concrete resource bound.
- Transactions, persistence, and writes: no transaction, EditLog, persistence, or data-write semantics are changed.
- FE/BE variables: no new cross-layer variable propagation is introduced.
- Performance: the delete-predicate fallback changes bounded TopK work into O(matches) score storage/materialization (MAIN-002).
- Additional review focus: none was supplied.
Review completion: two full rounds completed; both normal passes and the separate risk-focused pass returned NO_NEW_VALUABLE_FINDINGS in Round 2. All accepted and dismissed candidates have concrete conclusions.
| _index_query_context->collection_statistics = _opts.collection_statistics; | ||
| _index_query_context->collection_similarity = std::make_shared<CollectionSimilarity>(); | ||
| _index_query_context->query_limit = _score_runtime->get_limit(); | ||
| _index_query_context->query_limit = |
Contributor
There was a problem hiding this comment.
[P1] Gate early TopK on the complete visible candidate domain
Absence of a rowset delete predicate is not enough to make segment-local TopK safe. For example, with a higher-scoring id=1 and lower-scoring id=2, WHERE SEARCH('title:alpha') AND id=2 ORDER BY score() DESC LIMIT 1 lets FunctionSearch keep only id=1 from the full index, after which the existing id/key predicate intersects it away and id=2 cannot be recovered. UNIQUE-MOR has the same issue when an explicit __DORIS_DELETE_SIGN__=1 row or version merge removes a high-scoring key above SegmentIterator; neither source appears in this predicate count or the MOW bitmap. Please enable early TopK only when no later filter/merge can reject a hit, or feed the complete admissible-row domain into the collector, and cover both triggers.
Contributor
Author
There was a problem hiding this comment.
This is actually another bug in topN, unrelated to the issue of deleting predicates/bitmaps addressed in this PR. It should be fixed in a separate PR later.
| IColumn::MutablePtr result_column; | ||
| auto result_row_ids = std::make_unique<std::vector<uint64_t>>(); | ||
| if (_score_runtime->get_limit() > 0 && _col_predicates.empty() && | ||
| if (_opts.delete_condition_predicates->num_of_column_predicate() == 0 && |
Contributor
There was a problem hiding this comment.
[P1] Avoid materializing every score for a small TopK query
Together with query_limit = 0, this branch makes any applicable delete predicate turn ORDER BY score() ... LIMIT 1 into full-match collection: collect_multi_segment_doc_set inserts every score into _bm25_scores, then get_bm25_scores allocates row-id and score arrays for the entire bitmap before the delete predicate runs. A broad term on an old rowset with even one delete predicate can therefore consume O(matches) memory/CPU instead of bounded TopK resources. Please make the candidate collection visibility-aware/refillable while remaining bounded, or add a concrete resource guard with scale evidence that prevents this fallback from exhausting a query.
Contributor
Author
There was a problem hiding this comment.
In fact, only rowsets with a version less than the delete predicate will follow the full materialization path, while rowsets with a version greater than the delete predicate will follow the topN path normally. The performance loss of the former is paid for correctness and is within expectations.
|
|
||
| auto index_query_context = std::make_shared<IndexQueryContext>(); | ||
| auto field = StringHelper::to_wstring("title"); | ||
| TermQuery query(index_query_context, field, StringHelper::to_wstring("fleabag")); |
Contributor
There was a problem hiding this comment.
[P2] Exercise the offset and actual block-WAND paths
This case does not fail if + seg_base is removed: bitmap {0} still deletes first-reader local doc 0, while the expected second-reader hit is local doc 1/global doc 3 and remains admitted. Also, TermQuery::weight(false) is held as WeightPtr; TermWeight has only a templated same-name overload, not a virtual PruningCallback override, so the use_wand=true leg takes Weight's generic scorer loop just like false, and these two hits have equal frequency/length rather than a deleted high scorer plus a lower replacement. Please add a nonzero-base deletion that fails on local-id lookup, plus a genuinely scoring Boolean query whose OccurBooleanWeight enters block WAND with a deleted high scorer and a visible replacement.
| } | ||
|
|
||
| def assertScoreTopN = { tableName, predicate, expectedId -> | ||
| assertEquals(expectedId, sql(""" |
Contributor
There was a problem hiding this comment.
[P2] Use the regression golden-output mechanism
These are deterministic query results, but the suite wraps them in raw assertEquals(sql(...)) calls and adds no generated .out. The repository testing rules require qt_/order_qt_ (or explicit ORDER BY) with runner-generated expected output rather than Groovy assertions for determined results. Please convert the cases to named query tests and commit the generated output so the full rows/types are reviewed by the normal regression harness.
Contributor
TPC-H: Total hot run time: 17399 ms |
Contributor
TPC-DS: Total hot run time: 83413 ms |
Contributor
ClickBench: Total hot run time: 14.58 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
approved these changes
Aug 21, 2026
5 of 10 tasks
5 tasks done
airborne12
pushed a commit
that referenced
this pull request
Oct 22, 2026
…of a tablet (#69352) ### What problem does this PR solve? Issue Number: None Related PR: #69313 (BM25 statistics after compaction), #66979 (deleted documents in score TopN) Problem Summary: A parallel scan (`ParallelScannerBuilder`) splits one tablet into several scanners. Each `OlapScanner` then calls `CollectionStatistics::collect()` with only the `rs_splits` of its own split, so the BM25 doc count, avgdl and term doc frequencies come from a subset of the tablet's rowsets. Rows with identical text get different scores depending on which scanner reads them. The split depends on the tablet's total row count, `parallel_scan_max_scanners_count` (defaulting to the core count) and `parallel_scan_min_rows_per_scanner`. A row's score and its rank in `ORDER BY score()` therefore change when unrelated rows are loaded into the tablet, and differ between BEs with different core counts. Pagination over a score-ordered search returns repeated and missing rows. Reproduction (one bucket, one replica, no concurrent writes): ```sql CREATE TABLE t ( id BIGINT, content TEXT, INDEX idx_content (content) USING INVERTED PROPERTIES("parser" = "english") ) UNIQUE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 PROPERTIES ("replication_num" = "1", "enable_unique_key_merge_on_write" = "true", "disable_auto_compaction" = "true"); -- 8 loads (8 rowsets), each with a different share of matching rows INSERT INTO t SELECT 0 * 2000 + number, IF(number % 2 = 0, 'we should try these jobs today', 'call the candidate about salary') FROM numbers("number" = "2000"); INSERT INTO t SELECT 1 * 2000 + number, IF(number % 5 = 0, 'we should try these jobs today', 'call the candidate about salary') FROM numbers("number" = "2000"); -- ... batches 2..7 with modulus 8, 11, 14, 17, 20, 23 SELECT /*+ SET_VAR(enable_parallel_scan=true, parallel_scan_min_rows_per_scanner=1024, parallel_scan_max_scanners_count=16) */ score() AS s, count(*) FROM (SELECT score() FROM t WHERE SEARCH('content:jobs OR content:try') ORDER BY score() DESC LIMIT 100000) x GROUP BY s; ``` On 4.1.4, the 2,280 matching rows all have the same text but get **15 distinct scores (1.34 to 5.81)**. With `enable_parallel_scan=false` they all get 3.647686. On a production-sized table, the default thresholds give the same behavior. Fix: - `TabletCollectionStatistics` holds the rowsets of a tablet's entire read source. The first scanner of the tablet to prepare collects the statistics once with `collect_full_collection()`, and every scanner of that tablet copies them into its own `CollectionStatistics`. Later scanners wait for the first collection and get its result or error. - `CollectionStatistics::copy_collected_from()` copies the collected counters and resets the derived idf/avgdl caches, so no cache is shared between scanner threads. - `ParallelScannerBuilder` creates one holder per tablet, only when the scan has a `score()` runtime. Both the by-rowid and per-segment build paths pass it to their scanners. - The non-parallel path is unchanged. Each of its scanners already receives the tablet's full read source. Statistics remain per tablet. Making them consistent across tablets and BEs (as with Elasticsearch's `dfs_query_then_fetch`) is out of scope.
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: None
Problem Summary:
A score-sorted inverted-index
SEARCHquery can return an empty result when its highest-scoring hit has already been deleted. The scan applies deletion visibility after the score TopN collector has consumed the queryLIMIT; if the collector retains only a deleted document, the lower-scoring visible document has already been discarded.Reproduction:
Before this change, the score collector keeps only deleted
id = 1; the later visibility filtering removes it, so the actual result is:The expected result is:
There are two deletion mechanisms with different handling requirements:
Delete predicate
DUP tables and Unique MOW tables with
enable_mow_light_delete = trueuse delete predicates. A delete predicate is evaluated during scan after the inverted-index score collector and BM25 score materialization. Therefore, for a segment with delete predicates, this change disables both early score TopN truncation points: the index query receives no score limit, and score materialization does not keep only the query TopN. The scan evaluates the delete predicate first and applies the final query limit only to visible rows. Segments without delete predicates retain the existing fast path.Delete bitmap
Unique MOW tables with
enable_mow_light_delete = falsemark obsolete physical rows in a delete bitmap. These rows are also filtered later in scan, so they can occupy a score TopN slot for the same reason. Unlike delete predicates, delete bitmaps are common for key-overwrite imports as well as deletes, so disabling TopN whenever one exists would unnecessarily lose the optimization. This change passes the segment delete bitmap tocollect_multi_segment_top_kand excludes deleted document IDs while collecting TopK candidates. The collector can therefore retain the highest-scoring visible document.MATCH_ANYdoes not callcollect_multi_segment_top_k; it reads the inverted-index bitmap throughFunctionMatchBase::evaluate_inverted_index(). It is not part of the score TopN path fixed here.The regression covers the delete-predicate path for a DUP table and MOW light-delete table, and the delete-bitmap path for a Unique MOW table. The BE unit test verifies that multi-segment TopK collection skips entries present in the delete bitmap.
Release note
Fixes incorrect empty results for score-sorted inverted-index
SEARCHqueries when deleted documents would otherwise occupy the TopN candidates.Check List (For Author)
Test
./run-regression-test.sh --conf /tmp/doris-topn-regression.hNbTTJ/regression-conf.groovy --run -g p0 -d search -s test_search_score_topn_delete_predicate./run-be-ut.sh --run --filter=MultiSegmentCollectorTest.CollectTopKExcludesDeletedDocs -j 192Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)