Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
csun5285
force-pushed
the
scan-segment-aligned-split-for-match
branch
from
September 13, 2026 14:22
f57184f to
87407c5
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16844 ms |
Contributor
TPC-DS: Total hot run time: 81971 ms |
Contributor
ClickBench: Total hot run time: 14.63 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
force-pushed
the
scan-segment-aligned-split-for-match
branch
2 times, most recently
from
September 14, 2026 02:57
7651394 to
74f34f4
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16901 ms |
Contributor
TPC-DS: Total hot run time: 82586 ms |
Contributor
ClickBench: Total hot run time: 14.73 s |
csun5285
force-pushed
the
scan-segment-aligned-split-for-match
branch
from
September 14, 2026 06:58
74f34f4 to
f4ab2d5
Compare
`SegmentIterator::_lazy_init` runs the inverted index query over the whole
segment first and only then intersects the result with `_opts.row_ranges`. The
cost of a MATCH predicate is therefore priced per segment, while
`ParallelScannerBuilder` splits by rows. When a segment ends up shared by two
scanners, its posting lists are walked once per scanner.
An ann topn is priced per segment for the same reason and already asks for one
scanner per segment through `optimize_index_scan_parallelism`. Reuse that path
for MATCH: `_use_scan_parallelism_by_per_segment()` now decides both cases, and
`_build_scanners_by_per_segment` gives every segment a scanner of its own, so a
split boundary never falls inside a segment.
The MATCH branch applies in cloud mode only. A query without MATCH and without
an ann topn keeps taking `_build_scanners_by_rowid` and behaves exactly as
before. Note that the predicate also matches `search()` and `multi_match()`,
which are priced per segment in the same way.
Measured on a cloud cluster, cold query (file cache cleared and BE restarted
between runs). `NumSegmentTotal` counts how many times a segment is opened, so
it is the direct measure of the duplicated work this removes:
8 segments over 4 rowsets, 20M rows
before after
NumScanners 10 8
NumSegmentTotal 16 8 duplicate reads removed
InvertedIndexQueryTime 5.01s 2.55s -49%
Total 801ms 627ms -22%
40 segments over 40 rowsets, 2M rows
before after
NumScanners 1 40
NumSegmentTotal 40 40 nothing to remove
InvertedIndexQueryTime 323ms 411ms +27%
Total 403ms 69ms -83%
The gain comes from dropping duplicate segment reads, so it depends on the
geometry. When the row split cuts segments apart the index work roughly halves.
When it does not - many small rowsets, one segment each - there is no duplicate
work to remove and the index cost grows by the extra per-scanner setup, while
the wall clock drops because the scan is no longer serialized behind a single
scanner.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VgV4Zy5NaEYpUP4Yrh1yWn
csun5285
force-pushed
the
scan-segment-aligned-split-for-match
branch
from
September 14, 2026 08:25
f4ab2d5 to
5a5f430
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![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 cloud MATCH route reaches the intended segment-aligned builder, but the current implementation introduces one result-correctness regression and three substantial performance/coverage defects. The inline comments cover: incompatible rowset-local BM25 statistics for score TopN; unbounded scanner/delegate/task creation that ignores the configured cap; false-positive routing when MATCH will execute only via row fallback; and omission of supported MATCH virtual-column projections.
Critical checkpoint conclusions:
- Goal and test proof: ordinary cloud filter MATCH does enter the segment-aligned route, but the four issues prevent the change from safely accomplishing its goal. The PR adds no unit/regression test and checks no test box.
- Scope and focus: the two-file change is mechanically small, but the selector is not aligned with the full set of index-executable paths and the reused builder has semantics the new automatic caller cannot safely inherit unchanged.
- Concurrency: scanner construction precedes worker scheduling; no new data race, lock-order issue, or deadlock was found. The concurrency-related defect is eager cardinality before admission, reported inline.
- Lifecycle: no ownership cycle or static-initialization issue was found. Scanner/delegate/task objects are eventually released, but one of each is retained per segment until cancellation/close.
- Configuration: no new option is added. The route disregards existing
parallel_scan_max_scanners_count/minimum-row behavior, and it does not honorenable_inverted_index_querywhen deciding whether the optimization has any index work to reuse. - Compatibility: no protocol, function-symbol, storage-format, or rolling-upgrade compatibility change was found.
- Parallel paths: non-cloud, serial, binlog/TSO, aggregate/merge, and ANN-only gating remain unchanged. The supported virtual-column projection path is omitted and is reported inline.
- Conditions and errors: cloud/common-expression timing is valid, but expression-tree presence is not sufficient evidence that SegmentIterator will dispatch inverted-index evaluation. No separate error-propagation defect was found.
- Tests: existing score, no-index, projection, and ANN/fulltext tests do not cover cloud multi-rowset BM25 comparability, scanner-cap adherence, or positive/negative selector cases. No builds or tests were run because this review runner explicitly prohibits them; visible CI currently proves formatting/light checks only, with build/test jobs skipped.
- Observability: existing scanner/index counters are adequate; no separate observability blocker was found.
- Persistence and writes: transactions, committed-version visibility, delete bitmap/version propagation, persistence, and write paths are not changed; captured query read-source restrictions were traced and preserved.
- FE/BE variables: no new cross-layer variable or serialization field is introduced.
- Performance: besides the unbounded object population and lost slow-path row parallelism, score-enabled N-segment rowsets can repeat full-rowset collection-statistics work N times.
- Other: no additional user-provided focus was supplied. Two full rounds converged with every candidate accepted, merged, or dismissed and all round-two agents returning
NO_NEW_VALUABLE_FINDINGS.
| // Find MATCH recursively; ones nested in AND / OR / NOT count too. | ||
| static bool is_match_expr(const VExprSPtr& expr) { | ||
| DORIS_CHECK(expr != nullptr); | ||
| if (expr->node_type() == TExprNodeType::MATCH_PRED || |
Contributor
There was a problem hiding this comment.
[P1] Gate this on index execution, not only on expression shape. With enable_inverted_index_query=false (or a MATCH column without an iterator), this still returns true even though SegmentIterator skips index evaluation and runs the supported row fallback. On a single large segment, the old row-count builder can split that expensive fallback across scanners, while this route creates exactly one scanner and serializes it; on many segments it also triggers the unbounded population noted separately. Row-only wrappers have the same false positive because their roots never dispatch evaluate_inverted_index() to the MATCH child. Require index execution to be enabled and a usable/index-evaluable root before selecting this strategy.
Contributor
Author
There was a problem hiding this comment.
在cloud 上可以认为 match 的执行一定有倒排索引的
| state()->query_options().optimize_index_scan_parallelism && _ann_topn_runtime != nullptr) { | ||
| return true; | ||
| } | ||
| return config::is_cloud_mode() && |
Contributor
There was a problem hiding this comment.
[P1] Preserve global BM25 statistics before splitting by segment. This branch also covers score-enabled MATCH/SEARCH scans. Each generated scanner gets one segment from one rowset, and OlapScanner::_prepare_impl() builds a fresh CollectionStatistics from only that scanner's rs_splits; those rowset-local document-frequency and length values feed BM25. Rows from different rowsets are therefore ranked with incompatible IDF/avgdl values, so a final ORDER BY score() LIMIT ... can return the wrong row. Also, collection walks every segment of the supplied rowset, so an N-segment rowset now repeats that work N times. Compute/share statistics from the original complete read source, or keep score queries on the prior strategy, and add a cloud multi-rowset TopN regression.
Contributor
Author
There was a problem hiding this comment.
CollectionStatistics 一直是按scanner 的粒度构建的
| return true; | ||
| } | ||
| return config::is_cloud_mode() && | ||
| std::ranges::any_of(_common_expr_ctxs_push_down, |
Contributor
There was a problem hiding this comment.
[P2] Include pushed-down MATCH virtual-column projections in this decision. Projection-only MATCH is stored in _slot_id_to_virtual_column_expr, not _common_expr_ctxs_push_down, yet every scanner clones that context and SegmentIterator evaluates it over num_rows() for the whole segment before materializing the selected row range. A large segment split among N scanners therefore still repeats the same index evaluation N times, so this misses a supported path with the exact cost the PR targets. Inspect the virtual-column roots here as well (including any VirtualSlotRef unwrapping) and add a cloud projection regression.
Contributor
Author
There was a problem hiding this comment.
不考虑 projection 先，仅针对 where 后面的倒排索引计算
| scanner_builder.set_scan_parallelism_by_per_segment(true); | ||
| } | ||
| } | ||
| scanner_builder.set_scan_parallelism_by_per_segment(_use_scan_parallelism_by_per_segment()); |
Contributor
There was a problem hiding this comment.
[P1] Keep segment-aligned scans within the configured scanner cap. _build_scanners_by_per_segment() creates one scanner for every segment and never consults parallel_scan_max_scanners_count or the minimum-rows setting; initialization then creates a delegate and pending task for every scanner before execution concurrency is applied. A cloud table with many historical segments can therefore allocate an unbounded scanner/task population even with the cap set to 1, and LIMIT/cancellation only clean it up afterward. Group whole segments into at most the configured number of scanner read sources so no segment is split while scanner cardinality remains bounded.
Contributor
TPC-H: Total hot run time: 16930 ms |
Contributor
TPC-DS: Total hot run time: 82554 ms |
Contributor
ClickBench: Total hot run time: 14.83 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
3 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?
An inverted index query is run over the whole segment and only then intersected with the scanner's
row range, so its cost is priced per segment while
ParallelScannerBuildersplits by rows. Asegment shared by two scanners gets its posting lists walked twice.
An ann topn has the same property and already asks for one scanner per segment. This PR reuses that
path for MATCH, so both of these now build one scanner per segment: