[opt](bucket pruning) Avoid materializing tablet IDs during pruning - #67150
Open
zyp-V wants to merge 1 commit into
Open
[opt](bucket pruning) Avoid materializing tablet IDs during pruning#67150zyp-V wants to merge 1 commit into
zyp-V wants to merge 1 commit into
Conversation
zyp-V
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
August 26, 2026 03:48
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review conclusion: request changes
The production optimization is otherwise sound: it preserves bucket ordering and planner-path behavior, and selective HASH pruning avoids the eager O(N) boxed-ID list. One blocking test migration remains.
Critical checkpoints:
- Goal and scope: the focused allocation reduction works, but the PR is not merge-ready because an existing Nereids rewrite test still mocks the old accessor and now fails.
- Data correctness and parallel paths: selected-tablet membership and order remain equivalent across Nereids, legacy, short-circuit point queries, rollup/MV, row-binlog, manual/sample, HASH/RANDOM, and null/empty/all outcomes.
- Concurrency and lifecycle: the pruner captures one copy-on-write MaterializedIndex tablet snapshot; normal/cloud creation, restore, Gson reconstruction, and downstream bucket-sequence mapping preserve bucket order. No new lock, deadlock, static-initialization, or ownership issue was found.
- Compatibility, configuration, persistence, and writes: no configuration, FE-BE/thrift/storage-format, persistence, transaction, data-write, or rolling-upgrade surface is changed.
- Error handling and observability: no new Status/RPC boundary or runtime instrumentation need is introduced.
- Performance: selective paths materialize only chosen IDs; fallback paths retain the required ordered O(N) result without the former extra eager ID list.
- Tests and results: the migrated direct unit fixtures and selected/all-bucket assertions are structurally sound, and CheckStyle passed. No FE unit-test job ran, and RewriteRuleSuiteTest.testPruneOlapScanTablet will fail until its MaterializedIndex mock supplies getTablets().
- User focus: no additional focus was provided.
Review completion: complete after two rounds; every candidate was verified and deduplicated, with MAIN-001 as the sole accepted issue.
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Hash tablet pruning eagerly materialized every tablet ID before evaluating selective predicates. Backport the MaterializedIndex-backed pruning path so tablet IDs are resolved by bucket only when needed, while preserving the current branch's materialized-view column mapping. Migrate all affected unit tests, including the Nereids rewrite fixture, to the index-backed API and remove the obsolete list-backed constructor.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.planner.HashDistributionPrunerTest,org.apache.doris.planner.OlapScanNodeTest,org.apache.doris.nereids.rules.rewrite.RewriteRuleSuiteTest
- Behavior changed: No
- Does this need documentation: No
Contributor
|
run buildall |
morrySnow
changed the title
Contributor
TPC-H: Total hot run time: 17019 ms |
Contributor
TPC-DS: Total hot run time: 82008 ms |
Contributor
ClickBench: Total hot run time: 14.76 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review conclusion: request changes
One new blocking performance issue remains at the current head: non-point all-bucket scans lose the old boxed-tablet-ID reuse and can retain more allocation on the common full-scan path. The earlier Nereids Mockito failure is fixed at this head, its live thread is resolved, and it is not duplicated here.
Critical checkpoints:
- Goal and proof: the index-backed pruner achieves the intended O(K) ID materialization for selective point pruning, but MAIN-001 shows that non-point all-bucket planning can retain N extra
Longobjects, so the optimization is not yet safe across its full scope. - Scope and clarity: the six-file internal FE API/test migration is otherwise small and focused; all seven constructor call sites are migrated.
- Data correctness and parallel paths: bucket order and selected-tablet semantics remain equivalent across Nereids/legacy, HASH/RANDOM, point/non-point, base/rollup, manual/specified/sample, null/empty/all-result, missing-tablet, and bucket-sequence paths.
- Concurrency and lifecycle:
MaterializedIndex.getTablets()supplies a stable copy-on-write bucket-ordered snapshot; creation, shadow/rollup publication, and restore preserve count/order. No new lock, deadlock, static-initialization, ownership, or cleanup issue was found. - Configuration, compatibility, persistence, and writes: no configuration, FE-BE/thrift/storage-format, rolling-upgrade, EditLog, transaction, or data-write surface changes.
- Error handling and observability: no new error boundary is weakened, and no additional runtime logging or metrics are needed for this internal planner allocation change.
- Performance: selective point paths improve, but MAIN-001 is a concrete retained-allocation regression for non-point all-bucket/HASH/Nereids paths and needs correction or evidence from allocation benchmarks.
- Tests and results: the current tests cover sparse ordered IDs, selective and all-tablet fallback, Nereids integration, and isolated index fixtures. Current-head compile, FE UT, CheckStyle, and performance checks pass.
check_coverage_fe (Coverage)currently fails withTeamCity build failed; no local build or test was run because the review prompt forbids it. - User focus: no additional focus was provided.
Review completion: complete after two convergence rounds; every risk and candidate was independently verified, deduplicated, accepted, or dismissed. MAIN-001 is the sole new accepted inline issue.
| tabletId2BucketSeq.put(allTabletIds.get(i), i); | ||
| List<Tablet> allTablets = selectedTable.getTablets(); | ||
| for (int i = 0; i < allTablets.size(); i++) { | ||
| tabletId2BucketSeq.put(allTablets.get(i).getId(), i); |
Contributor
There was a problem hiding this comment.
[P1] Reuse the boxed tablet IDs when building bucket sequences
Non-point scans still need one ID per tablet as a tabletId2BucketSeq key, but this loop now boxes each ID again after the selected/result path has already boxed it. Previously allTabletIds created N Longs once and both scanTabletIds and this map retained those same objects. On a no-predicate/all-bucket scan, the new code therefore retains two distinct Longs per tablet; production IDs are outside the small Long cache, so removing the N-reference list can actually increase allocation on this common path. Please build the ordered boxed IDs once—or otherwise share each boxed ID between selection and bucket bookkeeping—so the optimization does not regress non-point/full-scan planning memory.
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
4 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: None
Related PR: None
Problem Summary: Hash tablet pruning eagerly materialized every tablet ID before evaluating selective predicates. Backport the MaterializedIndex-backed pruning path so tablet IDs are resolved by bucket only when needed, while preserving the current branch's materialized-view column mapping. Migrate the remaining unit tests to the index-backed API and remove the obsolete list-backed constructor.
Release note
None
Check List (For Author)
Test: No test run (per explicit request to skip compilation and directly review the code)
Behavior changed: No
Does this need documentation: No
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)