Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Superseded by #68199, which backports the whole local shuffle series as one PR. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review completed against ba53396 across all 12 changed files and the surrounding optimizer-property, memo, translation, local-exchange, distributed-scheduling, and BE sender paths. I found two correctness issues: (1) the optimizer and translator disagree for enable_local_shuffle=false with enable_local_shuffle_planner=false, allowing bucket-shuffle sinks while omitting the marker needed to fill missing-bucket receivers; and (2) set-operation derivation can cache a specific layout on one equivalent output position while its children were aligned on another, allowing later exact-property memo reuse to reconstruct stale child mappings and skip a required re-key. No existing inline review covered either issue at the final duplicate check. Per the review constraints, this was static-only; no builds or tests were run. Note: the PR was closed as superseded by #68199 while this review was in progress; these findings apply to the exact head above and should also be checked wherever this change is carried forward.
| // 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()) { |
There was a problem hiding this 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.
| } | ||
| // 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 |
There was a problem hiding this 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.
2 participants
Cherry-picked from #65129