[Refactor] Move colocation bucket dispatch onto AbstractOlapTableScanNode#76393
Open
banmoy wants to merge 1 commit into
Open
[Refactor] Move colocation bucket dispatch onto AbstractOlapTableScanNode#76393banmoy wants to merge 1 commit into
banmoy wants to merge 1 commit into
Conversation
Contributor
Module-risk briefing for Codex reviewThis is review focus, not a finding list. Validate against the diff. High-confidence risks
|
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…Node The colocation bucket-dispatch contract -- the bucket-sequence-to-locations map, the bucket-count computation, and the tablet-id-to-bucket-sequence fill -- lived inside OlapScanNode, and ColocatedBackendSelector reached into the concrete OlapScanNode (and directly into a public bucketSeq2locations field) instead of depending on an abstraction. Move the contract onto AbstractOlapTableScanNode: getBucketSeqToLocations() as an optional operation that throws by default (a scan that takes part in colocation overrides it), plus the stateless static helpers computeBucketNums(...) and fillTabletId2BucketSeq(...). OlapScanNode's bucketSeq2locations map is now private final, reached only through the getter, and getBucketNums() delegates to computeBucketNums. ColocatedBackendSelector depends on AbstractOlapTableScanNode and reads the map through the getter, so it no longer couples to the concrete OlapScanNode. A scan node that does not take part in colocation (e.g. MetaScanNode) simply does not override the getter. No behavior change: computeBucketNums reproduces the former getBucketNums/getRangeDistributionBucketNums logic verbatim (the instance reads are now explicit parameters); fillTabletId2BucketSeq moves unchanged and still resolves for existing OlapScanNode.fillTabletId2BucketSeq callers via inherited static access; and getBucketSeqToLocations returns the same map the selector previously read directly. Existing colocate tests reach the map through the getter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: PengFei Li <lpengfei2016@gmail.com>
banmoy
force-pushed
the
refactor_colocate_olap_scan_node
branch
from
a240b7a to
70cb258
Compare
banmoy
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Contributor
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Contributor
[FE Incremental Coverage Report]✅ pass : 32 / 34 (94.12%) file detail
|
Contributor
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
xiangguangyxg
approved these changes
Jul 15, 2026
|
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.
Why I'm doing:
The colocation bucket-dispatch contract — the bucket-sequence-to-locations map, the
bucket-count computation, and the tablet-id-to-bucket-sequence fill — lived inside
OlapScanNode, andColocatedBackendSelectorreached into the concreteOlapScanNode(and directly into a public
bucketSeq2locationsfield) instead of depending on anabstraction. Hoisting the contract lets any OLAP scan that takes part in colocation
reuse the same machinery, and lets the selector depend on the base type rather than a
concrete node and its public field.
What I'm doing:
Move the colocation bucket-dispatch contract onto
AbstractOlapTableScanNode:getBucketSeqToLocations()as an optional operation that throwsUnsupportedOperationExceptionby default; a scan that takes part in colocationoverrides it to return its own map. Scans that do not (e.g.
MetaScanNode) simplyinherit the default and never call it.
computeBucketNums(...)andfillTabletId2BucketSeq(...).OlapScanNode'sbucketSeq2locationsmap is nowprivate final, reached only throughthe getter;
getBucketNums()delegates tocomputeBucketNums(...).ColocatedBackendSelectortakesAbstractOlapTableScanNodeand reads the map throughthe getter, instead of the concrete
OlapScanNodeand its public field.No behavior change:
computeBucketNumsreproduces the formergetBucketNums/getRangeDistributionBucketNumslogic verbatim (the instance reads arenow explicit parameters);
fillTabletId2BucketSeqmoves unchanged and still resolves forexisting
OlapScanNode.fillTabletId2BucketSeqcallers via inherited static access;getBucketSeqToLocations()returns the same map the selector previously read directly.Existing colocate tests were updated to reach the map through the getter and otherwise
pass unchanged (ColocateJoinTest, RangeColocateScanDispatchTest,
RangeColocateScanRangeDispatchTest, ColocatedBackendSelectorTest,
DefaultSharedDataWorkerProviderTest).
What type of PR is this:
Does this PR entail a change in behavior?
Checklist:
Bugfix cherry-pick branch check: