cloud-fan commented on code in PR #59535:
URL: https://github.com/apache/spark/pull/59535#discussion_r4333013703
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -727,7 +758,7 @@ case class KeyedPartitioning(
*/
override protected def stringArgs: Iterator[Any] =
Iterator(expressions, partitionKeys, isGrouped, isCollapsed,
- mayContainUnknownPartitionKeys) ++ ungroupingOrigin
+ unknownKeysOrigin) ++ ungroupingOrigin
Review Comment:
Thanks, verified: the origin is flattened like `ungroupingOrigin`, and the
new ShuffleSpecSuite test pins the bare names and the unmarked case.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4324836957","thread_id":"inline:4324836957","verdict_sha256":"7089052e7047ccaa4bfd0c1c87df8559df7f9efa44aaac13090af6585efe82f7"}
-->
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/EnsureRequirements.scala:
##########
@@ -937,9 +937,10 @@ case class EnsureRequirements(
// The pairing is only worth committing to if both children still declare
the same aligned key
// sequence once the grouping has been pushed into them. A
`GroupPartitionsExec` gives up its
- // keyed claim when it turns out to regroup a layout that pins undeclared
rows to
- // `hash(key) % numPartitions` (see
`KeyLayout.mayContainUnknownPartitionKeys`), and only the
- // node knows the permutation it performs, so that answer arrives after
the pairing was chosen.
+ // keyed claim when the grouping re-labels the keys or replicates them
into expected slots,
Review Comment:
Thanks, verified: the EnsureRequirements, createShuffleSpec,
areKeysCompatible and sweep comments now distinguish plain regrouping from key
rewrites and non-identity alignment, and no `keyedMarkerOf` reference remains.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4324836979","thread_id":"inline:4324836979","verdict_sha256":"7089052e7047ccaa4bfd0c1c87df8559df7f9efa44aaac13090af6585efe82f7"}
-->
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -447,6 +447,21 @@ case object REPLICATED_FOR_JOIN extends UngroupingOrigin
// A spread an operator's ordering requirement is read through, which pairs
with nothing.
case object SPLIT_FOR_ORDERING extends UngroupingOrigin
+/**
+ * Why a layout may hold rows whose key it does not declare, and what became
of the hash routing
+ * that placed them. `KeyGroupedPartitioner` routes such a row by
+ * `nonNegativeMod(key.hashCode, numPartitions)`, so a marked layout
co-locates whole keys, never a
+ * subset of their columns. Anything that regrouped, sorted or projected the
keys moves the rows off
+ * that routing, and only the co-location survives.
+ */
+sealed trait UnknownKeysOrigin
+
+// An undeclared row sits at `nonNegativeMod(key.hashCode, numPartitions)`;
the relation holds.
+case object HASH_ROUTING extends UnknownKeysOrigin
+
+// A regrouping, a sort or a projection moved the rows off that relation; only
co-location remains.
Review Comment:
Thanks, verified: the producer list now matches the code, the union carry
rule no longer says the union expands the declared set, and the `project`
assert comment explains the undeclared-row versus label mismatch.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4324836985","thread_id":"inline:4324836985","verdict_sha256":"7089052e7047ccaa4bfd0c1c87df8559df7f9efa44aaac13090af6585efe82f7"}
-->
--
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]
