yashmayya commented on code in PR #19330:
URL: https://github.com/apache/pinot/pull/19330#discussion_r3865394619
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotRelDistributionTraitRule.java:
##########
@@ -161,6 +162,25 @@ private static RelDistribution deriveDistribution(RelNode
node) {
if (inputRelDistribution != null) {
return inputRelDistribution;
}
+ } else if (node instanceof SetOp) {
+ // A set operation sits above the exchanges inserted by
PinotSetOpExchangeNodeInsertRule. Only a hash exchange
+ // on all output columns leaves the output hash distributed on those
columns. That holds whether the exchange
+ // genuinely shuffles or is pre-partitioned by the
is_colocated_by_set_op_keys hint, because the hint asserts
+ // that rows equal across all projected columns already share a worker
-- exactly the claimed distribution. We
+ // take the hint at its word here, just as the exchange itself does.
+ // A local (SINGLETON) exchange, which is what UNION ALL gets, does not
redistribute anything and therefore
+ // guarantees nothing about the output. Claiming a distribution there
would let a deduplicating consumer above
+ // it -- for example the aggregate UnionToDistinctRule puts over a
distinct UNION -- skip a shuffle it needs.
+ // All inputs are checked so a future per-branch decision cannot
silently invalidate this.
+ for (RelNode setOpInput : inputs) {
+ RelNode unboxedInput = PinotRuleUtils.unboxRel(setOpInput);
+ if (!(unboxedInput instanceof PinotLogicalExchange)
+ || ((PinotLogicalExchange)
unboxedInput).getDistribution().getType()
Review Comment:
Good call, done — and it turned out to be more than cosmetic.
We return the input exchange's own distribution, which correctly describes
the set op's output for every type *except* `SINGLETON`: hash stays hash on
those keys, broadcast stays broadcast (every worker holds all rows of every
branch), and random claims nothing anyway. `SINGLETON` is the only unsafe one,
because a local exchange redistributes nothing while `SINGLETON` literally
means "everything on one worker", which isn't true of the output.
So checking `== SINGLETON` names the actual invariant, and as a bonus it
stops needlessly downgrading a broadcast or random input to
`RANDOM_DISTRIBUTED".
--
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]
