github-actions[bot] commented on code in PR #65129:
URL: https://github.com/apache/doris/pull/65129#discussion_r3568077977
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriver.java:
##########
@@ -528,6 +527,101 @@ public PhysicalProperties
visitPhysicalSetOperation(PhysicalSetOperation setOper
return PhysicalProperties.createHash(request, firstType);
}
+ /**
+ * The basic (bucket-shuffle anchor) child candidate of a set operation:
the first NATURAL
+ * child, or else the first STORAGE_BUCKETED child, or -1 when no child is
bucket-distributed.
+ * The largest natural / storage-bucketed child keeps its buckets and the
others are
+ * bucket-shuffled to it, so this only picks the candidate; {@link
#isSetOperationBucketAligned}
+ * decides whether the children actually agree on that anchor's buckets.
+ */
+ public static int setOperationBucketBasicIndex(List<DistributionSpec>
childrenDistribution) {
+ int firstNaturalIndex = -1;
+ int firstStorageBucketedIndex = -1;
+ for (int i = 0; i < childrenDistribution.size(); i++) {
+ if (!(childrenDistribution.get(i) instanceof
DistributionSpecHash)) {
+ continue;
+ }
+ ShuffleType childShuffleType = ((DistributionSpecHash)
childrenDistribution.get(i)).getShuffleType();
+ if (childShuffleType == ShuffleType.NATURAL && firstNaturalIndex <
0) {
+ firstNaturalIndex = i;
+ } else if (childShuffleType == ShuffleType.STORAGE_BUCKETED &&
firstStorageBucketedIndex < 0) {
+ firstStorageBucketedIndex = i;
+ }
+ }
+ return firstNaturalIndex >= 0 ? firstNaturalIndex :
firstStorageBucketedIndex;
+ }
+
+ /**
+ * Whether every child of the set operation is genuinely bucket-aligned to
a single basic
+ * child, i.e. all children are hash NATURAL / STORAGE_BUCKETED on the
same storage layout
+ * (table / index / partitions) AND map their shuffle columns to the same
set-operation output
+ * positions. This is the single proof shared by {@link
#visitPhysicalSetOperation} (to decide
+ * the output distribution) and {@code PhysicalPlanTranslator} (to set the
legacy
+ * {@code BUCKET_SHUFFLE} marker), so the derived property and the marker
can never disagree: a
+ * same-layout shape whose bucket key feeds different set-operation output
positions in
+ * different children is not aligned and must not be treated as bucketed
by either side.
+ */
+ public static boolean isSetOperationBucketAligned(
+ PhysicalSetOperation setOperation, List<DistributionSpec>
childrenDistribution) {
+ int distributeToChildIndex =
setOperationBucketBasicIndex(childrenDistribution);
+ if (distributeToChildIndex < 0) {
+ return false;
+ }
+ DistributionSpecHash basicChildHash = (DistributionSpecHash)
childrenDistribution.get(distributeToChildIndex);
+ List<Integer> basicOutputPositions
+ = mapShuffleColumnsToOutputPositions(setOperation,
basicChildHash, distributeToChildIndex);
+ if (basicOutputPositions == null || basicOutputPositions.isEmpty() ||
basicChildHash.getTableId() < 0) {
+ return false;
+ }
+ for (int i = 0; i < childrenDistribution.size(); i++) {
+ if (!(childrenDistribution.get(i) instanceof
DistributionSpecHash)) {
+ return false;
+ }
+ DistributionSpecHash childHash = (DistributionSpecHash)
childrenDistribution.get(i);
+ ShuffleType childShuffleType = childHash.getShuffleType();
+ if (childShuffleType != ShuffleType.NATURAL && childShuffleType !=
ShuffleType.STORAGE_BUCKETED) {
+ return false;
+ }
+ // same storage layout (a NATURAL basic child and its enforced
STORAGE_BUCKETED siblings
+ // all carry the basic child's layout, so they share the bucket
function)
+ if (childHash.getTableId() != basicChildHash.getTableId()
+ || childHash.getSelectedIndexId() !=
basicChildHash.getSelectedIndexId()
+ ||
!childHash.getPartitionIds().equals(basicChildHash.getPartitionIds())) {
+ return false;
+ }
+ // same set-operation output positions (the bucket key must feed
the same output columns
+ // in every child, otherwise per-bucket set operation would
compare mismatched columns)
+ List<Integer> positions =
mapShuffleColumnsToOutputPositions(setOperation, childHash, i);
+ if (positions == null || !positions.equals(basicOutputPositions)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Map a child's ordered shuffle columns to the set operation output
positions, using the
+ * child's regular output slots. Returns null when any shuffle column is
not present in the
+ * child's output (so the set operation does not carry that distribution
column).
+ */
+ static List<Integer> mapShuffleColumnsToOutputPositions(
+ PhysicalSetOperation setOperation, DistributionSpecHash childHash,
int childIndex) {
+ List<SlotReference> childOutput =
setOperation.getRegularChildrenOutputs().get(childIndex);
+ Map<ExprId, Integer> idToOutputIndex = new LinkedHashMap<>();
+ for (int j = 0; j < childOutput.size(); j++) {
+ idToOutputIndex.put(childOutput.get(j).getExprId(), j);
+ }
+ List<Integer> positions = new ArrayList<>();
+ for (ExprId columnId : childHash.getOrderedShuffledColumns()) {
+ Integer index = idToOutputIndex.get(columnId);
Review Comment:
This direct lookup still disagrees with the regulator when the bucket key
reaches the set operation through an equivalence set. A reduced shape is:
```text
UnionAll(k)
Project(b.id AS k)
HashJoin(a.id = b.id) // hash property ordered by a.id, equivalence
also contains b.id/k
OlapScan a // bucketed by a.id
...
PhysicalDistribute[STORAGE_BUCKETED]
...
```
`ChildrenPropertiesRegulator` can choose the first child as the
bucket-shuffle basic because `canMapBucketKeysToRequire()` /
`calAnotherSideRequiredShuffleIds()` search the hash equivalence sets, then it
enforces the other children as `STORAGE_BUCKETED` on that storage layout. But
this helper only looks up the hidden `a.id` directly in the set-op child
output, where only `k`/`b.id` is visible, so `isSetOperationBucketAligned()`
returns false and the translator never sets the legacy `BUCKET_SHUFFLE` marker.
For bucket-pruned `UNION`/`EXCEPT`,
`UnassignedScanBucketOlapTableJob.shouldFillUpInstances()` then skips the
missing-bucket receiver fill-up even though a bucket-shuffle child exchange was
planned, so rows from the shuffled side can be dropped.
Please make this mapping equivalence-aware like the regulator, while still
requiring every bucket key to resolve to the same visible set-output position,
and add a fill-up regression where the basic child outputs the equivalent join
key rather than the storage bucket ExprId.
--
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]
