github-actions[bot] commented on code in PR #65129:
URL: https://github.com/apache/doris/pull/65129#discussion_r3567943240
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -2496,14 +2497,56 @@ public PlanFragment visitPhysicalSetOperation(
setOperationNode.setColocate(true);
}
- // TODO: open comment when support `enable_local_shuffle_planner`
- // for (Plan child : setOperation.children()) {
- // PhysicalPlan childPhysicalPlan = (PhysicalPlan) child;
- // if
(JoinUtils.isStorageBucketed(childPhysicalPlan.getPhysicalProperties())) {
- //
setOperationNode.setDistributionMode(DistributionMode.BUCKET_SHUFFLE);
- // break;
- // }
- // }
+ // Mark BUCKET_SHUFFLE only when every child is bucket-distributed on
the same storage
+ // layout, i.e. ChildrenPropertiesRegulator actually aligned all
children to one basic
+ // child's buckets: it enforces the non-basic children to the basic
child's storage
+ // layout, and only does so when set-op bucket shuffle is allowed, so
the marker follows
+ // the regulator's decision without re-checking the session variables
here (that would
+ // risk drifting out of sync with the request-derivation gate).
+ //
+ // A single storage-bucketed child is not proof on its own that this
set operation chose
+ // bucket shuffle: under a non-bucket (ANY) request a child can stay
storage-bucketed
+ // independently while the other children are left unaligned, and
marking from that child
+ // alone would drive PlanFragment.hasBucketShuffleNode(), bucket
assignment and non-
+ // INTERSECT receiver fill-up on a set operation whose rows are not
actually bucket-co-
+ // located. Requiring one shared storage layout keeps the marker in
step with
+ // ChildOutputPropertyDeriver, which likewise only claims a bucket
distribution when all
+ // children agree on the layout.
+ //
+ // Unlike hash join, BUCKET_SHUFFLE is not exclusive with isColocate
above: for a set
+ // operation isColocate describes the bucket-aligned scheduling of the
fragment (the
+ // basic child scans buckets directly), while BUCKET_SHUFFLE describes
how the other
+ // children arrive (bucket-shuffle exchanges). Both routes converge to
the same
+ // bucket-hash local exchange requirement in
SetOperationNode.enforceAndDeriveLocalExchange.
+ boolean anyStorageBucketed = false;
+ boolean allChildrenBucketAligned = !setOperation.children().isEmpty();
+ DistributionSpecHash basicLayout = null;
+ for (Plan child : setOperation.children()) {
+ DistributionSpec childSpec = ((PhysicalPlan)
child).getPhysicalProperties().getDistributionSpec();
+ if (!(childSpec instanceof DistributionSpecHash)) {
+ allChildrenBucketAligned = false;
+ break;
+ }
+ DistributionSpecHash childHash = (DistributionSpecHash) childSpec;
+ ShuffleType shuffleType = childHash.getShuffleType();
+ if ((shuffleType != ShuffleType.NATURAL && shuffleType !=
ShuffleType.STORAGE_BUCKETED)
Review Comment:
This marker still is not using the same proof as
`ChildOutputPropertyDeriver`. The deriver maps every child's
`orderedShuffledColumns` through `getRegularChildrenOutputs()` and requires all
children to land on the same set-output positions before it claims a bucketed
set-operation output; if that proof fails it returns `createAnyFromHash(...)`
so parents insert a real exchange. This translator loop only checks that the
children are `NATURAL`/`STORAGE_BUCKETED` on the same table/index/partitions. A
same-layout shape can still have the bucket key feeding output position `k` in
one nested child and output position `v` in another, so the deriver refuses the
bucket property while the legacy node is still marked `BUCKET_SHUFFLE`. Since
that marker drives `hasBucketShuffleNode()`, bucket assignment, and
non-`INTERSECT` receiver fill-up, please mirror the output-position proof here
(or carry an explicit regulator-aligned marker) before setting `BUCKET_SHUFFLE`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -2496,14 +2497,56 @@ public PlanFragment visitPhysicalSetOperation(
setOperationNode.setColocate(true);
}
- // TODO: open comment when support `enable_local_shuffle_planner`
- // for (Plan child : setOperation.children()) {
- // PhysicalPlan childPhysicalPlan = (PhysicalPlan) child;
- // if
(JoinUtils.isStorageBucketed(childPhysicalPlan.getPhysicalProperties())) {
- //
setOperationNode.setDistributionMode(DistributionMode.BUCKET_SHUFFLE);
- // break;
- // }
- // }
+ // Mark BUCKET_SHUFFLE only when every child is bucket-distributed on
the same storage
+ // layout, i.e. ChildrenPropertiesRegulator actually aligned all
children to one basic
+ // child's buckets: it enforces the non-basic children to the basic
child's storage
+ // layout, and only does so when set-op bucket shuffle is allowed, so
the marker follows
+ // the regulator's decision without re-checking the session variables
here (that would
+ // risk drifting out of sync with the request-derivation gate).
+ //
+ // A single storage-bucketed child is not proof on its own that this
set operation chose
+ // bucket shuffle: under a non-bucket (ANY) request a child can stay
storage-bucketed
+ // independently while the other children are left unaligned, and
marking from that child
+ // alone would drive PlanFragment.hasBucketShuffleNode(), bucket
assignment and non-
+ // INTERSECT receiver fill-up on a set operation whose rows are not
actually bucket-co-
+ // located. Requiring one shared storage layout keeps the marker in
step with
+ // ChildOutputPropertyDeriver, which likewise only claims a bucket
distribution when all
+ // children agree on the layout.
+ //
+ // Unlike hash join, BUCKET_SHUFFLE is not exclusive with isColocate
above: for a set
+ // operation isColocate describes the bucket-aligned scheduling of the
fragment (the
+ // basic child scans buckets directly), while BUCKET_SHUFFLE describes
how the other
+ // children arrive (bucket-shuffle exchanges). Both routes converge to
the same
+ // bucket-hash local exchange requirement in
SetOperationNode.enforceAndDeriveLocalExchange.
+ boolean anyStorageBucketed = false;
+ boolean allChildrenBucketAligned = !setOperation.children().isEmpty();
+ DistributionSpecHash basicLayout = null;
+ for (Plan child : setOperation.children()) {
+ DistributionSpec childSpec = ((PhysicalPlan)
child).getPhysicalProperties().getDistributionSpec();
+ if (!(childSpec instanceof DistributionSpecHash)) {
+ allChildrenBucketAligned = false;
+ break;
+ }
+ DistributionSpecHash childHash = (DistributionSpecHash) childSpec;
+ ShuffleType shuffleType = childHash.getShuffleType();
+ if ((shuffleType != ShuffleType.NATURAL && shuffleType !=
ShuffleType.STORAGE_BUCKETED)
+ || childHash.getTableId() < 0) {
+ allChildrenBucketAligned = false;
+ break;
+ }
+ anyStorageBucketed |= shuffleType == ShuffleType.STORAGE_BUCKETED;
+ if (basicLayout == null) {
+ basicLayout = childHash;
+ } else if (childHash.getTableId() != basicLayout.getTableId()
Review Comment:
This new marker also exposes a downstream fill-up bug for `INTERSECT`:
`UnassignedScanBucketOlapTableJob.shouldFillUpInstances()` tries to skip
intersect set ops, but it imports the Nereids algebra `Intersect` and checks
`setOperationNode instanceof Intersect` while the translated node is
`org.apache.doris.planner.IntersectNode`. After this line marks a
bucket-shuffle `PhysicalIntersect`, that skip never fires, so bucket-pruned
intersect fragments create missing-bucket receivers and shuffle rows into
buckets that cannot produce output because the anchor side is empty. Please fix
the downstream type check (for example, check planner `IntersectNode`) and add
coverage that bucket-shuffle `INTERSECT` does not trigger fill-up while
`UNION`/`EXCEPT` still do.
--
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]
