[opt](ditributed-plan) bucket shuffle down grade only check not shuffle side #59506
Conversation
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
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 35730 ms |
TPC-DS: Total hot run time: 174649 ms |
ClickBench: Total hot run time: 27.1 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
morrySnow
changed the title
morrySnow
changed the title
morrySnow
force-pushed
the
test_bcg
branch
2 times, most recently
from
2e31cce to
eb05d6b
Compare
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 31194 ms |
TPC-DS: Total hot run time: 172753 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
ClickBench: Total hot run time: 26.97 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run NonConcurrent |
englefly
approved these changes
Jan 12, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
bot
added
approved
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
Author
|
run buildall |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the bucket shuffle join strategy logic in the Nereids query planner to improve how bucket shuffle downgrades are detected and handled. The changes introduce explicit control flags to determine when to check for bucket shuffle downgrades, replacing the previous scattered downgrade checks with a more structured approach.
Changes:
- Introduced
shouldCheckLeftBucketDownGradeandshouldCheckRightBucketDownGradeflags to control when downgrade checks occur - Refactored conditional logic to consolidate bucket shuffle downgrade checks at the end of the method instead of inline
- Updated test output files to reflect improved bucket shuffle join selection in TPC-DS queries
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ChildrenPropertiesRegulator.java | Refactors the visitPhysicalHashJoin method to use control flags for bucket shuffle downgrade checks, improving code organization and correctness |
| query72.out | Updated query plan output showing improved join strategy selection with bucketShuffle joins |
| query54.out | Updated query plan output showing improved join strategy selection with shuffleBucket joins |
| query82.out (multiple variants) | Updated query plan output showing broadcast to bucketShuffle join strategy improvement |
| query78.out (multiple variants) | Updated query plan output showing shuffle to bucketShuffle join strategy improvement |
| query5.out (multiple variants) | Updated query plan output showing shuffle to bucketShuffle join strategy improvement |
| query39.out (multiple variants) | Updated query plan output showing broadcast to bucketShuffle join strategy improvement |
| query37.out (multiple variants) | Updated query plan output showing broadcast to bucketShuffle join strategy improvement |
| query22.out (multiple variants) | Updated query plan output showing broadcast to bucketShuffle join strategy improvement |
| query21.out (multiple variants) | Updated query plan output showing broadcast to bucketShuffle join strategy improvement |
| query80.out (multiple variants) | Updated query plan output showing shuffle to bucketShuffle join strategy improvement |
| query75.out (multiple variants) | Updated query plan output showing shuffle to bucketShuffle join strategy improvement |
| query49.out (multiple variants) | Updated query plan output showing shuffle to bucketShuffle join strategy improvement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if ((leftHashSpec.getShuffleType() == ShuffleType.STORAGE_BUCKETED | ||
| && rightHashSpec.getShuffleType() == ShuffleType.EXECUTION_BUCKETED)) { | ||
| if (children.get(0).getPlan() instanceof PhysicalDistribute) { | ||
| shouldCheckrightBucketDownGrade = true; |
There was a problem hiding this comment.
There is a typo in the variable name. The variable should be named "shouldCheckRightBucketDownGrade" (capital 'R'), not "shouldCheckrightBucketDownGrade" (lowercase 'r'). This inconsistency with the naming of "shouldCheckLeftBucketDownGrade" makes the code harder to read and maintain.
| (DistributionSpecHash) requiredProperties.get(0).getDistributionSpec(), | ||
| (DistributionSpecHash) requiredProperties.get(1).getDistributionSpec())); | ||
| if (children.get(0).getPlan() instanceof PhysicalDistribute) { | ||
| shouldCheckrightBucketDownGrade = true; |
There was a problem hiding this comment.
There is a typo in the variable name. The variable should be named "shouldCheckRightBucketDownGrade" (capital 'R'), not "shouldCheckrightBucketDownGrade" (lowercase 'r'). This inconsistency with the naming of "shouldCheckLeftBucketDownGrade" makes the code harder to read and maintain.
| (DistributionSpecHash) requiredProperties.get(0).getDistributionSpec(), | ||
| (DistributionSpecHash) requiredProperties.get(1).getDistributionSpec())); | ||
| } | ||
| if (shouldCheckrightBucketDownGrade && isBucketShuffleDownGrade(rightChild)) { |
There was a problem hiding this comment.
There is a typo in the variable name. The variable should be named "shouldCheckRightBucketDownGrade" (capital 'R'), not "shouldCheckrightBucketDownGrade" (lowercase 'r'). This inconsistency with the naming of "shouldCheckLeftBucketDownGrade" makes the code harder to read and maintain.
| Optional<PhysicalProperties> updatedForRight = Optional.empty(); | ||
|
|
||
| boolean shouldCheckLeftBucketDownGrade = false; | ||
| boolean shouldCheckrightBucketDownGrade = false; |
There was a problem hiding this comment.
There is a typo in the variable name. The variable should be named "shouldCheckRightBucketDownGrade" (capital 'R'), not "shouldCheckrightBucketDownGrade" (lowercase 'r'). This inconsistency with the naming of "shouldCheckLeftBucketDownGrade" makes the code harder to read and maintain.
Suggested change
| boolean shouldCheckrightBucketDownGrade = false; | |
| boolean shouldCheckRightBucketDownGrade = false; |
| } else if ((leftHashSpec.getShuffleType() == ShuffleType.EXECUTION_BUCKETED | ||
| && rightHashSpec.getShuffleType() == ShuffleType.STORAGE_BUCKETED)) { | ||
| if (children.get(0).getPlan() instanceof PhysicalDistribute) { | ||
| shouldCheckrightBucketDownGrade = true; |
There was a problem hiding this comment.
There is a typo in the variable name. The variable should be named "shouldCheckRightBucketDownGrade" (capital 'R'), not "shouldCheckrightBucketDownGrade" (lowercase 'r'). This inconsistency with the naming of "shouldCheckLeftBucketDownGrade" makes the code harder to read and maintain.
TPC-H: Total hot run time: 31248 ms |
TPC-DS: Total hot run time: 174030 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
2 similar comments
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
feiniaofeiafei
approved these changes
Jan 12, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
zzzxl1993
pushed a commit
to zzzxl1993/doris
that referenced
this pull request
Jan 13, 2026
…le side (apache#59506) Related PR: apache#36784 This pull request refactors the logic in the `visitPhysicalHashJoin` method to improve the handling and enforcement of bucket shuffle join strategies in the Nereids planner. It introduces new checks and flags to more accurately determine when bucket shuffle downgrades are needed, and updates the plan output to reflect the use of `bucketShuffle` joins in various TPC-DS queries. Key changes include: ### Planner logic improvements * Added `shouldCheckLeftBucketDownGrade` and `shouldCheckrightBucketDownGrade` flags to control when to check for bucket shuffle downgrades, leading to more precise enforcement of shuffle strategies. * Refactored conditional logic to use these new flags, replacing previous direct downgrade checks, and improved the handling of cases where shuffle key orders do not match. These changes enhance the accuracy and efficiency of join planning in distributed query execution.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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?
Related PR: #36784
Problem Summary:
This pull request refactors the logic in the
visitPhysicalHashJoinmethod to improve the handling and enforcement of bucket shuffle join strategies in the Nereids planner. It introduces new checks and flags to more accurately determine when bucket shuffle downgrades are needed, and updates the plan output to reflect the use ofbucketShufflejoins in various TPC-DS queries.Key changes include:
Planner logic improvements
shouldCheckLeftBucketDownGradeandshouldCheckrightBucketDownGradeflags to control when to check for bucket shuffle downgrades, leading to more precise enforcement of shuffle strategies.These changes enhance the accuracy and efficiency of join planning in distributed query execution.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)