metanil commented on code in PR #59760:
URL: https://github.com/apache/spark/pull/59760#discussion_r4433782711
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -2570,17 +2602,27 @@ case class KeyedShuffleSpec(
// `createPartitioning` replaces a transform's argument with the other
child's cluster key,
// which drops whatever surrounds the key, e.g. the `+ 1` of
`bucket(4, b + 1)` or the `.a`
// of `bucket(4, s.a)`. Such a spec's own child would then be laid out
differently from the
- // children shuffled onto it.
+ // children shuffled onto it. Whether Spark can evaluate it is
`canEvaluatePartitioning`.
case t: TransformExpression => argumentsAreColumns(t)
case _ => false
} &&
// Shuffling another child onto these keys evaluates the partition
expressions per row to
// decide where each row goes, and reduced keys are not what those
expressions compute.
partitioning.expressionsDescribeKeys
+ /** Whether Spark can evaluate each transform of this layout, see
`canEvaluate`. */
+ override lazy val canEvaluatePartitioning: Boolean =
partitioning.expressions.forall {
Review Comment:
@dongjoon-hyun Thanks for the review.
Nice find! I can reproduce this.
> Could pickCoPartitionTarget ask evaluableFunction of the transforms
rebuilt over each child it shuffles, as createPartitioning builds them, rather
than of the reported ones?
I think I already did that, but had to dropped it because when the check
refuses, the planner falls back to the ordinary hash shuffle, and for a cogroup
with different key types that's worse. An INT 5 and a BIGINT 5 hash to
different partitions, so the UDF silently gets unmatched groups instead of
failing loudly. That's a pre-existing cogroup bug, since cogroups don't coerce
their key types. I filed it as SPARK-60782: on 4.3.0 it returns 20 half-groups
instead of 10 with AQE off, or with AQE on whenever it doesn't collapse
everything into one partition.
So I went with a narrower fix in 43c97a16474. At the top level a decimal
reaches the function as a `Decimal` object, so its precision doesn't change
what the function reads; only the scale does. A decimal of the declared scale
is now taken at any precision there, while nested decimals keep the precision
and storage rule. Your example evaluates as on master now, and it's pinned in
`EnsureRequirementsSuite`. Different types like INT vs BIGINT still fail
loudly, as on master. I also fixed the doc: it now cites
`BinaryComparison.sameType` and no longer promises a failure at planning.
--
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]
