ulysses-you commented on code in PR #58447:
URL: https://github.com/apache/spark/pull/58447#discussion_r3913547045
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -1598,24 +1559,40 @@ case class KeyedShuffleSpec(
}
}
- private def isExpressionCompatible(left: Expression, right: Expression):
Boolean =
- (left, right) match {
- case (_: LeafExpression, _: LeafExpression) => true
- case (left: TransformExpression, right: TransformExpression) =>
- if (SQLConf.get.v2BucketingPushPartValuesEnabled &&
- !SQLConf.get.v2BucketingPartiallyClusteredDistributionEnabled &&
- SQLConf.get.v2BucketingAllowCompatibleTransforms) {
- left.isCompatible(right)
- } else {
- left.isSameFunction(right)
- }
- case (_: AttributeReference, _: TransformExpression) |
- (_: TransformExpression, _: AttributeReference) =>
- SQLConf.get.v2BucketingPushPartValuesEnabled &&
- !SQLConf.get.v2BucketingPartiallyClusteredDistributionEnabled &&
- SQLConf.get.v2BucketingAllowCompatibleTransforms
- case _ => false
+ private def isExpressionCompatible(left: Expression, right: Expression):
Boolean = {
+ if (TransformExpression.hasReducedKeys(left) ||
TransformExpression.hasReducedKeys(right)) {
+ // Reduced keys are in a key space that neither transform names, so
comparing the transforms
+ // says nothing about whether the two sides are laid out the same way.
The pair that was
+ // reduced together is laid out the same way, since its two sides came
out of one reduce onto
+ // one key space. Anything else has to shuffle. That includes a pair
that reduced onto the
+ // same space through a different pairing, which nothing here can tell
apart, and an identity
+ // side, which holds raw values.
+ (left, right) match {
+ case (l: TransformExpression, r: TransformExpression) =>
l.hasSameReducedKeys(r)
Review Comment:
+1 to make the condition strict.
The information is already lost at reduce time. After a both-sides reduce
the real key space is `r1(f1(x))`, which no transform names, and the current
`Reducer` API cannot say which space it reduced onto. All the planner has is
the reported original expression (e.g. `bucket(32, id)`).
If the gate were widened to allow deriving another reducer from the reported
expression (i.e. reducing already-reduced keys again), it would admit not only
the divisible, benign case above but also failure 2 of this PR: `bucket(12)
JOIN bucket(8)` lands on `id % 4`, then joining `bucket(6)` derives `(id % 4) %
6` — the left keys stay put while the right side moves to `id % 6`, and the
query loses rows silently.
This is not an implementation accident; it is mathematically undecidable
from the reduced values: a key of `id % 4 = 0` can stand for `id = 0, 4, 8`,
whose `id % 6` values are `0, 4, 2` respectively. Mapping from a reduced space
to a non-divisible one is simply not well-defined. So unless the exact reduced
space is known, "reduce again" is only sound in the divisible case, and the
available information cannot tell divisible from non-divisible.
So `hasSameReducedKeys` admits only the one provably sound case: both sides
came out of the same reduce (same pairing), which guarantees the same layout.
Everything else has to shuffle. That includes:
1. A further reduce onto a divisor (the `32/12` vs `64/24` case above: `id %
4` vs `id % 8`).
2. Different pairings that happen to land on the same space (`12 JOIN 8` and
`12 JOIN 20` both give `id % 4`).
3. Meeting an unreduced table that is already laid out on that space
(`bucket(12) JOIN bucket(8)` reduced onto `id % 4`, joined to a raw `bucket(4,
id)` table whose keys are already aligned).
The cost in all three is an extra shuffle, never a wrong answer. On the base
these shapes ran with 0 shuffles — correct by accident in the divisible cases,
losing rows in the non-divisible ones — and the API cannot keep only the good
half, so tightening the gate wholesale was the only safe choice available.
--
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]
