peter-toth commented on code in PR #55885:
URL: https://github.com/apache/spark/pull/55885#discussion_r4147616455
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/TransformExpression.scala:
##########
@@ -52,23 +87,46 @@ case class TransformFunctionId(canonicalName: String,
numBucketsOpt: Option[Int]
case class TransformExpression(
function: BoundFunction,
children: Seq[Expression],
- numBucketsOpt: Option[Int] = None,
- reducedWith: Option[TransformFunctionId] = None) extends Expression {
+ reducedWith: Option[TransformFunctionId] = None) extends Expression with
Logging {
override def nullable: Boolean = true
/** Drops the trailing `reducedWith` when it is unset, so the ordinary form
is unchanged. */
override protected def stringArgs: Iterator[Any] =
if (reducedWith.isEmpty) super.stringArgs.take(productArity - 1) else
super.stringArgs
- /** The identity of this expression's transform function. */
- lazy val functionId: TransformFunctionId =
- TransformFunctionId(function.canonicalName(), numBucketsOpt)
+ /**
+ * Extract literal children (constant parameters) from this transform. These
are constant
+ * arguments like the bucket count in `bucket(n, col)` or the width in
`truncate(col, width)`.
+ * Positions are dropped, so this is for handing parameters to a connector
reducer, NOT for
+ * identity -- see [[functionId]] and [[TransformFunctionId.argumentShapes]].
+ */
+ private lazy val literalChildren: Seq[Literal] =
+ children.collect { case l: Literal => l }
/**
- * Whether this [[TransformExpression]] has the same semantics as `other`.
- * For instance, `bucket(32, c)` is equal to `bucket(32, d)`, but not to
`bucket(16, d)` or
- * `year(c)`.
+ * This transform's identity, or None if some argument is not a literal, a
nested transform or a
+ * column reference (e.g. `c + 1`); such a transform is not the same as any
other.
+ */
+ lazy val functionId: Option[TransformFunctionId] = {
+ import TransformFunctionId.ArgumentShape
+ val shapes = children.map {
+ case l: Literal => Some(ArgumentShape.Param(l))
+ case t: TransformExpression => t.functionId.map(ArgumentShape.Nested(_))
+ case c if TransformExpression.isColumnRef(c) =>
Some(ArgumentShape.Column)
Review Comment:
Following up on the hold above. I was wrong here: a transform over `b + 1`
must never be the same as one over a column, and your `None` is what guarantees
that on this head.
The total `functionId` makes `bucket(4, b + 1)` the same as `bucket(4, x)`,
and that loses rows:
- `KeyedShuffleSpec.createPartitioning` shuffles a side onto `bucket(4, b +
1)` when its join key is `b + 1`.
- A later join that clusters that side on the bare `b` pairs it with a
`bucket(4, x)` table, because `keyPositions` maps a partition expression to a
cluster key through its reference.
With `4c51b96` on top of this head, `t1 JOIN plain p ON t1.id = p.b + 1 JOIN
t3 ON p.b = t3.x` returns 0 of 7 rows. This head returns all 7.
Master has the same bug, since its `isSameFunction` ignores the arguments. I
opened SPARK-59887 and #59165 for it. It makes `isSameFunction` and `reducers`
refuse a transform whose argument is not a bare `Attribute`, keeps such a
layout from being the one other children are shuffled onto, and adds tests for
the shapes it fixes. Could you rebase onto it once it lands? Three things to
watch in the conflict:
- The new `argumentsAreAttributes` skips literal children, so it carries
over to your model as it is.
- It also refuses a struct field argument, which your `Option` treats as a
`Column`. A side shuffled onto `bucket(4, s.a)` pairs with one on `bucket(4,
s.b)` in a join on `s`, and loses rows the same way. Please keep that refusal,
either through the new guard or by making `ArgumentShape.Column` a bare
`Attribute` only.
- The first new test also asserts two shuffles. This head gives the right
rows but plans three, since it has no `canCreatePartitioning` clause yet, so it
passes only once that clause is in.
Please don't take `4c51b96`. `e93ae9b` (finding 1) is unrelated and still
stands.
--
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]
