[fix](bm25) Collect collection statistics over the whole collection, not one scanner's slice#66047
Open
airborne12 wants to merge 2 commits into
Open
[fix](bm25) Collect collection statistics over the whole collection, not one scanner's slice#66047airborne12 wants to merge 2 commits into
airborne12 wants to merge 2 commits into
Conversation
…not one scanner's slice BM25 idf and avg_dl are collection-level quantities. OlapScanner built them from `_tablet_reader_params.rs_splits`, which is the slice of the read source that scanner was handed -- and ParallelScannerBuilder divides one tablet's read source among several scanners. So as soon as parallel scan produced more than one scanner per tablet, each scanner derived idf from a different subset of rowsets, and score() depended on how the tablet happened to be split. Rows served by a scanner that owned few rowsets scored differently from identical rows served by a scanner that owned many. The read source is now captured before it is sliced, and the statistics are collected once per tablet over all of its rowsets. CollectionStatisticsBuildState does the sharing: the first scanner to arrive builds while the rest wait on a condition variable, then every scanner receives its own clone. The collected counts live in an immutable block the clones share; only the lazily memoized idf/avg_dl maps are per-scanner, so no locking is needed on the read path. A failed or throwing build is recorded and handed to the waiters instead of being retried once per scanner. Without the sharing this would be a straight regression: every scanner would repeat the full-collection walk that previously covered only its own slice.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Jul 25, 2026
…n change The full-collection scope and CollectionStatisticsBuildState were bundled into the SNII branch, but neither is SNII-specific: OlapScanner collected BM25 statistics from the scanner's own rs_splits while ParallelScannerBuilder divides a tablet's read source among scanners, so idf depended on how the tablet was split for every inverted index format, not just SNII. Fixing that here made the SNII change carry an unrelated correctness fix and made both harder to review. The scope fix and the single-flight sharing now live in apache#66047 against master, together with the three concurrency tests that covered them. This branch drops its copy: the exec/scan files go back to master byte for byte, and collection_statistics loses clone_for_scanner(), freeze_for_scanners(), the shared base block and the build state. Nothing SNII-specific is lost. collect() remains a thin adapter over collect_full_collection(), so every SNII segment path still runs; SNII simply inherits master's per-scanner behaviour until apache#66047 lands, at which point this branch picks the fix up from master on the next rebase.
…rency Three tests for what the sharing has to guarantee when several scanners of one tablet reach it at once, each driving the real threads through a barrier rather than simulating the interleaving: - eight scanners arrive together and exactly one builder runs, and all eight get usable statistics; - a build that fails is reported to every waiter, and the failure is not rebuilt per scanner; - clones share the collected counts but keep their own lazily memoized avg_dl, so one scanner seeding its cache cannot change what another scanner computes.
Contributor
TPC-H: Total hot run time: 29503 ms |
Contributor
TPC-DS: Total hot run time: 178318 ms |
Contributor
TPC-H: Total hot run time: 29239 ms |
Contributor
TPC-DS: Total hot run time: 177908 ms |
Contributor
ClickBench: Total hot run time: 25.12 s |
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
2 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.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
BM25
idfandavg_dlare properties of the whole collection, butOlapScannerbuilt them from_tablet_reader_params.rs_splits— the slice of the read source that this scanner was handed:ParallelScannerBuilderdivides a tablet'sentire_read_source.rs_splitsinto severalpartitial_read_source.rs_splits, one per scanner. So as soon as parallel scan produced more thanone scanner for a tablet, each scanner computed
idfover a different subset of rowsets, andscore()depended on how the tablet happened to be split — rows served by a scanner owning fewrowsets scored differently from identical rows served by a scanner owning many.
This affects any inverted index format that supports BM25 scoring.
Fix. The read source is captured before it is sliced, and statistics are collected once per
tablet over all of its rowsets.
CollectionStatisticsBuildStateperforms the sharing: the firstscanner to arrive builds while the others wait on a condition variable, then each receives its own
clone. The collected counts live in an immutable block that clones share; only the lazily memoized
idf/avg_dlmaps are per-scanner, so the read path needs no locking. A failed or throwing buildis recorded and handed to the waiters rather than retried once per scanner.
The sharing is not incidental — without it this would be a straight regression, since every scanner
would repeat the full-collection walk that previously covered only its own slice.
Release note
Fix
score()returning different BM25 scores for the same row depending on how many scanners thetablet's read source was split into. Collection statistics are now computed over the entire
collection and shared by all scanners of a tablet.
Check List (For Author)
Test
Regression test
regression-test/suites/inverted_index_p0/test_bm25_score_parallel_scan.groovyloads a V3inverted index table as several rowsets, then compares
ORDER BY score() LIMIT 20withenable_parallel_scan = falseagainst the same query forced onto many scanners(
parallel_scan_min_rows_per_scanner = 16,parallel_scan_max_scanners_count = 16).The two result sets must be identical.
Unit Test
Manual test (add detailed scripts or steps below)
No need to test or manual test. Explain why:
Behavior changed:
No.
Yes.
score()values change for queries that were previously served by more than one scanner pertablet. The new values are the correct collection-wide ones; the old values varied with the
scanner split. Relative ranking within a single scanner was already consistent, so this mainly
affects absolute scores and cross-scanner ordering.
Does this need documentation?
Check List (For Reviewer who merge this PR)