github-actions[bot] commented on code in PR #65129:
URL: https://github.com/apache/doris/pull/65129#discussion_r3550479738
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriver.java:
##########
@@ -440,53 +443,89 @@ public PhysicalProperties
visitPhysicalSetOperation(PhysicalSetOperation setOper
return PhysicalProperties.GATHER;
}
- // TODO: open comment when support `enable_local_shuffle_planner`
- // int distributeToChildIndex
- // =
setOperation.<Integer>getMutableState(PhysicalSetOperation.DISTRIBUTE_TO_CHILD_INDEX).orElse(-1);
- // if (distributeToChildIndex >= 0
- // && childrenDistribution.get(distributeToChildIndex)
instanceof DistributionSpecHash) {
- // 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()) {
- // boolean isUnion = setOperation instanceof Union;
- // boolean shuffleToRight = distributeToChildIndex > 0;
- // if (!isUnion && shuffleToRight) {
- // return new PhysicalProperties(
- // new DistributionSpecHash(
- // setOperationDistributeColumnIds,
- // ShuffleType.EXECUTION_BUCKETED
- // )
- // );
- // } else {
- // // keep the distribution as the child
- // return new PhysicalProperties(
- // new DistributionSpecHash(
- // setOperationDistributeColumnIds,
- // childDistribution.getShuffleType(),
- // childDistribution.getTableId(),
- // childDistribution.getSelectedIndexId(),
- // childDistribution.getPartitionIds()
- // )
- // );
- // }
- // }
- // }
+ // 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).
+ // Rules 1 and 2 only apply when every child is hash-distributed: with
a mixed shape
+ // such as one execution-any child plus one storage-bucketed child
(reachable through
+ // the ANY child request of union), the set operation output is not
distributed by the
+ // storage layout, so it falls through to the generic derivation below.
+ // 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 firstNaturalIndex = -1;
+ int firstStorageBucketedIndex = -1;
+ boolean allChildrenHash = true;
+ for (int i = 0; i < childrenDistribution.size(); i++) {
+ if (!(childrenDistribution.get(i) instanceof
DistributionSpecHash)) {
+ allChildrenHash = false;
+ break;
+ }
+ ShuffleType childShuffleType
+ = ((DistributionSpecHash)
childrenDistribution.get(i)).getShuffleType();
+ if (childShuffleType == ShuffleType.NATURAL) {
+ if (firstNaturalIndex < 0) {
+ firstNaturalIndex = i;
+ }
+ } else if (childShuffleType == ShuffleType.STORAGE_BUCKETED
+ && firstStorageBucketedIndex < 0) {
+ firstStorageBucketedIndex = i;
+ }
+ }
+ int distributeToChildIndex = -1;
+ if (allChildrenHash) {
+ distributeToChildIndex = firstNaturalIndex >= 0 ?
firstNaturalIndex : firstStorageBucketedIndex;
+ }
+ if (distributeToChildIndex >= 0) {
+ DistributionSpecHash childDistribution
Review Comment:
This branch still needs to prove that all hash children describe the same
physical distribution before it returns the first storage child's layout. The
current check only verifies that every child has some `DistributionSpecHash`;
after that it picks the first `STORAGE_BUCKETED` child and maps only that
child's shuffled columns to the set-op output.
A reachable shape is:
```text
Agg(group by u.id)
UnionAll(u.id)
Intersect(t1.id, t2.id) // bucket-shuffle output, layout T1
Intersect(t3.id, t4.id) // bucket-shuffle output, layout T3
```
The outer `PhysicalUnion` still has the cheap `ANY` child request, so the
regulator does not align those two nested set-operation children; it leaves
their `STORAGE_BUCKETED` outputs as-is. This deriver then claims layout T1 for
the union, and a parent `ShuffleType.REQUIRE` aggregate/join can accept that
claim via `DistributionSpecHash.satisfy()` and skip the realignment exchange,
even though rows from the second child are bucketed by a different storage
function. The same issue applies to a `STORAGE_BUCKETED` child mixed with an
`EXECUTION_BUCKETED` hash child.
Please either validate every hash child maps to the same set-output
positions and, for storage-bucketed children, the same table/index/partition
layout before returning `STORAGE_BUCKETED`, or fall back to a
non-specific/execution-bucketed property so the parent inserts a real exchange.
--
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]
