924060929 commented on code in PR #66477:
URL: https://github.com/apache/doris/pull/66477#discussion_r4346392534
##########
fe/fe-core/src/main/java/org/apache/doris/planner/LocalExchangeNode.java:
##########
@@ -56,6 +60,12 @@ public LocalExchangeNode(PlanNodeId id, PlanNode inputNode,
LocalExchangeType ex
this.children.add(inputNode);
this.exchangeType = exchangeType;
this.fragment = inputNode.getFragment();
+ // Preserve the effective storage layout through passthrough/unary
nodes as well as direct
+ // ExchangeNode and OlapScanNode children.
+ HashDistributionInfo.HashType childHashType =
inputNode.getStorageDistributionHashType();
+ if (childHashType != null) {
+ this.distributionHashType = childHashType;
+ }
Review Comment:
Could we preserve an explicit layout state here instead of turning a
missing/ambiguous layout into CRC32?
This PR adds both `PlanNode.getStorageDistributionHashType()` and
`collectStorageHashTypes()`. The former returns `null` for both "no known
bucket layout" and "conflicting child layouts"; the latter traverses the tree
again to distinguish them. Both traversals must independently implement the
same Exchange boundary and broadcast/NLJ probe-side rules. Here, however, a
null child result leaves the new field at its default CRC32, and
`getStorageDistributionHashType()` then advertises a definite layout.
A reduced, manually constructed tree demonstrates the contract inconsistency:
```text
Union(IDENTITY scan, CRC32 scan)
-> null; PlanFragment.toThrift() detects the conflict
PASSTHROUGH
Union(IDENTITY scan, CRC32 scan)
-> this wrapper reports CRC32; the fragment skips that conflict check
```
A passthrough wrapper should not erase a layout conflict. This is a
source-derived internal-contract example, **not a demonstrated production
bug**: I have not established a normal SQL path producing this tree. The
current UNION ANY path inserts execution-exchange boundaries, and
storage-bucketed set operations align their inputs. This is also distinct from
the earlier cross-fragment/broadcast collection comment, whose boundary fixes
are present at this head.
A minimal change within this PR would be:
1. Use one shared layout result that distinguishes `NONE`,
`KNOWN(hashType)`, and `CONFLICT` (and `UNKNOWN` separately if needed), rather
than an enum/null result plus a second collection traversal. Retain the
existing receiver/probe boundary rules in that one derivation.
2. Make PASSTHROUGH preserve the child's state. A bucket local exchange must
receive an explicit target storage layout; an execution shuffle must not claim
the source storage algorithm merely through inheritance. Keep legacy CRC32
defaults at the compatibility boundary, rather than using them to resolve
current-plan ambiguity.
3. Resolve/validate this result when forming the translated fragment/local
exchange; let serialization encode it and assert the contract. The existing
fragment-level tag for BE-native exchanges can remain for this change—no
pipeline redesign is required.
4. Add focused tests for bare versus passthrough-wrapped conflicts,
layout-less inputs, and the existing Exchange/broadcast/NLJ boundaries.
This would give the new algorithm propagation a single, explicit contract
without requiring all layout information to come from `DistributionSpecHash`,
which is insufficient for some STORAGE_ANY/multi-partition paths.
--
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]
