Copilot commented on code in PR #59506:
URL: https://github.com/apache/doris/pull/59506#discussion_r2680718148
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java:
##########
@@ -544,24 +533,46 @@ public List<List<PhysicalProperties>>
visitPhysicalHashJoin(
} else if ((leftHashSpec.getShuffleType() ==
ShuffleType.STORAGE_BUCKETED
&& rightHashSpec.getShuffleType() ==
ShuffleType.STORAGE_BUCKETED)) {
- if (bothSideShuffleKeysAreSameOrder(rightHashSpec, leftHashSpec,
+ if (!bothSideShuffleKeysAreSameOrder(rightHashSpec, leftHashSpec,
(DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec(),
(DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec())) {
- return ImmutableList.of(originChildrenProperties);
- }
- if (children.get(0).getPlan() instanceof PhysicalDistribute) {
- updatedForLeft = Optional.of(calAnotherSideRequired(
- ShuffleType.STORAGE_BUCKETED, rightHashSpec,
leftHashSpec,
- (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec(),
- (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec()));
- } else {
- updatedForRight = Optional.of(calAnotherSideRequired(
- ShuffleType.STORAGE_BUCKETED, leftHashSpec,
rightHashSpec,
- (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
- (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
+ if (children.get(0).getPlan() instanceof PhysicalDistribute) {
+ shouldCheckrightBucketDownGrade = true;
+ updatedForLeft = Optional.of(calAnotherSideRequired(
+ ShuffleType.STORAGE_BUCKETED, rightHashSpec,
leftHashSpec,
+ (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec(),
+ (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec()));
+ } else {
+ shouldCheckLeftBucketDownGrade = true;
+ updatedForRight = Optional.of(calAnotherSideRequired(
+ ShuffleType.STORAGE_BUCKETED, leftHashSpec,
rightHashSpec,
+ (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
+ (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
+ }
}
}
+ if (shouldCheckLeftBucketDownGrade &&
isBucketShuffleDownGrade(leftChild)) {
+ updatedForLeft = Optional.of(calAnotherSideRequired(
+ ShuffleType.EXECUTION_BUCKETED, leftHashSpec, leftHashSpec,
+ (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
+ (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec()));
+ updatedForRight = Optional.of(calAnotherSideRequired(
+ ShuffleType.EXECUTION_BUCKETED, leftHashSpec,
rightHashSpec,
+ (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
+ (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
+ }
+ if (shouldCheckrightBucketDownGrade &&
isBucketShuffleDownGrade(rightChild)) {
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java:
##########
@@ -544,24 +533,46 @@ public List<List<PhysicalProperties>>
visitPhysicalHashJoin(
} else if ((leftHashSpec.getShuffleType() ==
ShuffleType.STORAGE_BUCKETED
&& rightHashSpec.getShuffleType() ==
ShuffleType.STORAGE_BUCKETED)) {
- if (bothSideShuffleKeysAreSameOrder(rightHashSpec, leftHashSpec,
+ if (!bothSideShuffleKeysAreSameOrder(rightHashSpec, leftHashSpec,
(DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec(),
(DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec())) {
- return ImmutableList.of(originChildrenProperties);
- }
- if (children.get(0).getPlan() instanceof PhysicalDistribute) {
- updatedForLeft = Optional.of(calAnotherSideRequired(
- ShuffleType.STORAGE_BUCKETED, rightHashSpec,
leftHashSpec,
- (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec(),
- (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec()));
- } else {
- updatedForRight = Optional.of(calAnotherSideRequired(
- ShuffleType.STORAGE_BUCKETED, leftHashSpec,
rightHashSpec,
- (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
- (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
+ if (children.get(0).getPlan() instanceof PhysicalDistribute) {
+ shouldCheckrightBucketDownGrade = true;
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java:
##########
@@ -386,6 +386,9 @@ public List<List<PhysicalProperties>> visitPhysicalHashJoin(
Optional<PhysicalProperties> updatedForLeft = Optional.empty();
Optional<PhysicalProperties> updatedForRight = Optional.empty();
+ boolean shouldCheckLeftBucketDownGrade = false;
+ boolean shouldCheckrightBucketDownGrade = false;
Review 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.
```suggestion
boolean shouldCheckRightBucketDownGrade = false;
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java:
##########
@@ -497,45 +483,48 @@ public List<List<PhysicalProperties>>
visitPhysicalHashJoin(
}
} else if (leftHashSpec.getShuffleType() ==
ShuffleType.EXECUTION_BUCKETED
&& rightHashSpec.getShuffleType() ==
ShuffleType.EXECUTION_BUCKETED) {
- if (bothSideShuffleKeysAreSameOrder(rightHashSpec, leftHashSpec,
+
+ if (!bothSideShuffleKeysAreSameOrder(rightHashSpec, leftHashSpec,
(DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec(),
(DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec())) {
- return ImmutableList.of(originChildrenProperties);
+ shouldCheckLeftBucketDownGrade = true;
+ updatedForRight = Optional.of(calAnotherSideRequired(
+ ShuffleType.EXECUTION_BUCKETED, leftHashSpec,
rightHashSpec,
+ (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
+ (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
}
- updatedForRight = Optional.of(calAnotherSideRequired(
- ShuffleType.EXECUTION_BUCKETED, leftHashSpec,
rightHashSpec,
- (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
- (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
} else if ((leftHashSpec.getShuffleType() ==
ShuffleType.EXECUTION_BUCKETED
&& rightHashSpec.getShuffleType() ==
ShuffleType.STORAGE_BUCKETED)) {
if (children.get(0).getPlan() instanceof PhysicalDistribute) {
+ shouldCheckrightBucketDownGrade = true;
updatedForLeft = Optional.of(calAnotherSideRequired(
ShuffleType.STORAGE_BUCKETED, rightHashSpec,
leftHashSpec,
(DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec(),
(DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec()));
} else {
+ shouldCheckLeftBucketDownGrade = true;
updatedForRight = Optional.of(calAnotherSideRequired(
ShuffleType.EXECUTION_BUCKETED, leftHashSpec,
rightHashSpec,
(DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
(DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
}
} else if ((leftHashSpec.getShuffleType() ==
ShuffleType.STORAGE_BUCKETED
&& rightHashSpec.getShuffleType() == ShuffleType.NATURAL)) {
- // TODO: we must do shuffle on right because coordinator could not
do right be selection in this case,
- // since it always to check the left most node whether olap scan
node.
- // after we fix coordinator problem, we could do right to left
bucket shuffle
+ shouldCheckLeftBucketDownGrade = true;
updatedForRight = Optional.of(calAnotherSideRequired(
ShuffleType.STORAGE_BUCKETED, leftHashSpec, rightHashSpec,
(DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
(DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
} else if ((leftHashSpec.getShuffleType() ==
ShuffleType.STORAGE_BUCKETED
&& rightHashSpec.getShuffleType() ==
ShuffleType.EXECUTION_BUCKETED)) {
if (children.get(0).getPlan() instanceof PhysicalDistribute) {
+ shouldCheckrightBucketDownGrade = true;
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java:
##########
@@ -497,45 +483,48 @@ public List<List<PhysicalProperties>>
visitPhysicalHashJoin(
}
} else if (leftHashSpec.getShuffleType() ==
ShuffleType.EXECUTION_BUCKETED
&& rightHashSpec.getShuffleType() ==
ShuffleType.EXECUTION_BUCKETED) {
- if (bothSideShuffleKeysAreSameOrder(rightHashSpec, leftHashSpec,
+
+ if (!bothSideShuffleKeysAreSameOrder(rightHashSpec, leftHashSpec,
(DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec(),
(DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec())) {
- return ImmutableList.of(originChildrenProperties);
+ shouldCheckLeftBucketDownGrade = true;
+ updatedForRight = Optional.of(calAnotherSideRequired(
+ ShuffleType.EXECUTION_BUCKETED, leftHashSpec,
rightHashSpec,
+ (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
+ (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
}
- updatedForRight = Optional.of(calAnotherSideRequired(
- ShuffleType.EXECUTION_BUCKETED, leftHashSpec,
rightHashSpec,
- (DistributionSpecHash)
requiredProperties.get(0).getDistributionSpec(),
- (DistributionSpecHash)
requiredProperties.get(1).getDistributionSpec()));
} else if ((leftHashSpec.getShuffleType() ==
ShuffleType.EXECUTION_BUCKETED
&& rightHashSpec.getShuffleType() ==
ShuffleType.STORAGE_BUCKETED)) {
if (children.get(0).getPlan() instanceof PhysicalDistribute) {
+ shouldCheckrightBucketDownGrade = true;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
