peter-toth commented on code in PR #59165:
URL: https://github.com/apache/spark/pull/59165#discussion_r4172168311
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -2086,6 +2119,48 @@ case class KeyedShuffleSpec(
}
}
+ /**
+ * Whether every argument of `transform` is a bare column.
`isExpressionCompatible` asks it of
+ * both sides before it compares anything, `canCreatePartitioning` asks it
of a template, and
+ * `rebuiltOver` asserts it.
+ *
+ * `keyPositions` pairs two sides up by the column each expression
references. That pairing
+ * describes one key space when the argument is the column itself.
`bucket(4, b + 1)` is a
+ * function of `b`, but not the same function of it as `bucket(4, x)` is of
`x`. Nor is
+ * `bucket(4, s.a)` the same function of `s` as `bucket(4, t.b)` is of `t`.
So such a transform
+ * pairs only with a transform over the same `argumentShape`. Over the same
shape, the two
+ * compare as two transforms of columns do, e.g. `bucket(4, b + 1)` with
`bucket(4, c + 1)` on a
+ * join `b = c`. A reduce of `bucket(8, b + 1)` onto `bucket(4, c + 1)` is
fine too, since it
+ * maps the partition keys and evaluates no argument. An identity side is
not reduced onto such a
+ * transform, since that would evaluate `id + 1` on keys the query never
computes it for, which
+ * can overflow.
+ */
+ private def argumentsAreColumns(transform: TransformExpression): Boolean =
+ transform.children.forall(_.isInstanceOf[Attribute])
+
+ /**
+ * `transform` with its argument replaced by `column`. Only a transform of
bare columns can be
+ * rebuilt this way. Any other argument would lose what surrounds its
column, e.g. the `+ 1` of
+ * `bucket(4, b + 1)`.
+ */
+ private def rebuiltOver(transform: TransformExpression, column: Expression)
Review Comment:
Done in
[71c37c6](https://github.com/apache/spark/pull/59165/commits/71c37c6eedc6dbfd10abdcecf6c89b472dca02a6).
The parameter is `key` now, and the doc says that `transform`'s arguments are
bare columns while `key` may be any expression.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -1941,8 +1949,20 @@ case class KeyedShuffleSpec(
* `KeyedPartitioning.positionsCoveringClusterKeys` counts it. See that
method for why the two
* stay apart.
*
- * The assertion is what makes the reference reading sound. With one
reference per expression,
- * "some reference is a cluster key" and "every reference is" are the same
statement.
+ * Only an expression with a single reference maps to a position. With one
reference, "some
+ * reference is a cluster key" and "every reference is" are the same
statement, which is what
+ * makes the reference reading sound. A scan reports no other expression
+ * (`KeyedPartitioning.supportsExpressions`), but two planner paths build
one over the other
Review Comment:
Thanks, confirmed by running it. An in-memory table partitioned by
`bucket(4, s.a)` reports `bucket(4, s.a AS a)`, and its scan's
`outputPartitioning` is `UnknownPartitioning`. The same holds for
`identity(s.a)`. I dropped the bullet, and the description now says that a scan
reports neither a transform of an expression nor one of a struct field.
--
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]
