github-actions[bot] commented on code in PR #68188:
URL: https://github.com/apache/doris/pull/68188#discussion_r4045840657
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -2628,6 +2628,35 @@ public PlanFragment visitPhysicalSetOperation(
setOperationNode.setColocate(true);
}
+ // Storage-bucketed children only appear when the FE local shuffle
planner is active:
+ // ChildrenPropertiesRegulator and RequestPropertyDeriver both gate
the bucket-shuffle
+ // alternative on enableLocalShufflePlanner. Gate the marker on the
same flag so the
+ // dependency is explicit and a future planner change that produced a
STORAGE_BUCKETED
+ // distribution outside the local-shuffle planner cannot silently mark
BUCKET_SHUFFLE here.
+ //
+ // Within that gate a storage-bucketed child means the regulator chose
the bucket shuffle
+ // alternative (it enforces the other children onto the basic child's
buckets), so the marker
+ // simply follows that decision. It must not re-check the table id
independently: the basic
+ // child selection in the regulator is the single place that vets the
layout, and re-checking
+ // here could suppress a bucket shuffle the property model already
committed to and desync a
+ // parent that aligned to the set operation output.
+ //
+ // 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.
+ if (context.getSessionVariable() != null
+ && context.getSessionVariable().isEnableLocalShufflePlanner())
{
Review Comment:
`setOperationBucketShuffleAllowed()` permits bucket-shuffled set operations
when `enable_local_shuffle=false` even if `enable_local_shuffle_planner=false`,
but this guard leaves the translated node unmarked. With a bucket-pruned UNION
(or right/later-basic EXCEPT),
`UnassignedScanBucketOlapTableJob.shouldFillUpInstances()` then skips
missing-bucket receivers because it recognizes the set operation only through
`isBucketShuffle()`. The exchange still targets those bucket indexes, whose
dummy destinations discard rows. Please derive the marker from the actual
storage-bucketed plan/the same eligibility predicate and add a
`false/false/true` result case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriver.java:
##########
@@ -453,6 +455,80 @@ public PhysicalProperties
visitPhysicalSetOperation(PhysicalSetOperation setOper
if
(childrenDistribution.stream().allMatch(DistributionSpecGather.class::isInstance))
{
return PhysicalProperties.GATHER;
}
+
+ // After ChildrenPropertiesRegulator the children distributions are
already legal, so the
+ // output is derived by describing what the children provide:
+ // 1. one or more NATURAL children: output the first NATURAL child's
distribution
+ // (several NATURAL children behave like colocate);
+ // 2. no NATURAL but some STORAGE_BUCKETED child: output its
STORAGE_BUCKETED distribution;
+ // 3. all EXECUTION_BUCKETED children: output the execution hash (the
generic loop below);
+ // 4. anything else (e.g. random): output a non-specific property
(also below).
+ // When the basic child does not directly output its shuffle columns,
or the children do
+ // not agree, this falls through to the generic loop below, which is
equivalence-set aware
+ // and checks that every child maps its shuffle columns to the same
set-operation output
+ // positions before it claims a bucketed output. The basic child is
recomputed from the
+ // children distributions instead of being carried as mutable planner
state, because mutable
+ // state does not survive the with-copies in chooseBestPlan() and the
+ // RecomputePhysicalPropertiesPostProcessor re-derivation, while this
recomputation is
+ // deterministic on any copy of the plan.
+ int distributeToChildIndex = -1;
+ int firstStorageBucketedIndex = -1;
+ for (int i = 0; i < childrenDistribution.size(); i++) {
+ if (childrenDistribution.get(i) instanceof DistributionSpecHash) {
+ ShuffleType childShuffleType
+ = ((DistributionSpecHash)
childrenDistribution.get(i)).getShuffleType();
+ if (childShuffleType == ShuffleType.NATURAL) {
+ distributeToChildIndex = i;
+ break;
+ } else if (childShuffleType == ShuffleType.STORAGE_BUCKETED
+ && firstStorageBucketedIndex < 0) {
+ firstStorageBucketedIndex = i;
+ }
+ }
+ }
+ if (distributeToChildIndex < 0) {
+ distributeToChildIndex = firstStorageBucketedIndex;
+ }
+ if (distributeToChildIndex >= 0) {
+ DistributionSpecHash childDistribution
+ = (DistributionSpecHash)
childrenDistribution.get(distributeToChildIndex);
+ List<SlotReference> childToIndex =
setOperation.getRegularChildrenOutputs().get(distributeToChildIndex);
+ Map<ExprId, Integer> idToOutputIndex = new LinkedHashMap<>();
+ for (int j = 0; j < childToIndex.size(); j++) {
+ idToOutputIndex.put(childToIndex.get(j).getExprId(), j);
+ }
+
+ List<ExprId> orderedShuffledColumns =
childDistribution.getOrderedShuffledColumns();
+ List<ExprId> setOperationDistributeColumnIds = new ArrayList<>();
+ for (ExprId tableDistributeColumnId : orderedShuffledColumns) {
+ Integer index = idToOutputIndex.get(tableDistributeColumnId);
+ if (index == null) {
+ break;
+ }
+
setOperationDistributeColumnIds.add(setOperation.getOutput().get(index).getExprId());
+ }
+ // check whether the set operation output all distribution columns
of the child
+ if (setOperationDistributeColumnIds.size() ==
orderedShuffledColumns.size()) {
+ // Keep the basic child's specific storage layout as the set
operation output. When
Review Comment:
When the basic child's bucket key is directly exposed at output 0 but
equivalent to output 1, a parent request on output 1 makes the regulator align
the other set-operation arms on output 1. This fast path nevertheless records a
specific `NATURAL(output[0])` property. `calculateEnforce()` caches that exact
property with the output-1 child requirements before adding the immediate
corrective exchange; if another join-order alternative later requires output 0,
the equal-cost correct entry does not replace the stale child list because
`updateLowestCostTable()` only replaces on lower cost. A parent can then select
exact `NATURAL(output[0])`, and `chooseBestPlan()` reconstructs children still
hashed on output 1, which is wrong for UNION arms where the two values differ.
Please derive or validate the same ordered output-position mapping across every
child instead of returning from the basic child's direct ExprIds alone, and add
a memo-reuse result case with unequal sibling columns.
--
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]
