ulysses-you commented on code in PR #58339:
URL: https://github.com/apache/spark/pull/58339#discussion_r3920220911
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -1100,6 +1153,15 @@ object PartitioningCollection {
case _ => None
}
+ /**
+ * Whether `p` or any nested [[KeyedPartitioning]] may contain unknown
partition keys, read
+ * from its first keyed member, the same one
`checkKeyedPartitioningInvariant` compares
+ * against. Keyless inputs answer false: `EnsureRequirements` normalizes a
shuffled join's
+ * children onto one template, so they never meet a keyed sibling here.
+ */
+ private[sql] def mayContainUnknownPartitionKeys(p: Partitioning): Boolean =
Review Comment:
Thanks -- and thanks for settling this one properly. Adopted verbatim in
74bf881: the helper is `keyedMarkerOf(p): Option[Boolean]`, so "no keyed
member" and "unmarked keyed member" stay distinct values. Your framing is the
part I had wrong in an earlier round: the keyless argument is a property of the
`ShuffledJoin` call site, not of the helper, and `GroupPartitionsExec` passes a
single child where "meets a keyed sibling" is not even a question -- so the
helper documents neither, and the reachability sentence is gone from its doc.
`ShuffledJoin` drops keyless inputs through the `flatMap` (with
`markers.isEmpty` covering the all-keyless case), the per-input guard is
`keyedMarkerOf(partitioning).contains(true)`, and `GroupPartitionsExec` reads
`contains(true)` for the same value it read before.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/AliasAwareOutputExpression.scala:
##########
@@ -131,9 +131,20 @@ trait PartitioningPreservingUnaryExecNode extends
UnaryExecNode
if (projectablePositions.isEmpty) return LazyList.empty
- // All input KPs share the same partitionKeys and isCollapsed flag by
invariant, so the first
- // one projects the keys and both flags for every combination below. Only
the expressions
- // differ.
+ // Collection members carry a uniform marker (mixed collections are
cleared at
Review Comment:
Fixed in 74bf881, thanks -- you are right that the attribution had gone
stale, and that the failure mode of the stale version is the dangerous kind: it
reads as if deleting the clearing would break `kps.head`. All three comments
now credit the constructor `require` plus the `fromPartitionings` OR for
uniformity, with the clearing's role stated as precision: it keeps the OR from
spreading a spurious marker onto the accurate side.
`AliasAwareOutputExpression` takes your two-line version, the `ShuffledJoin`
doc now says marked and unmarked *inputs* meet, and the `@param`'s rule (2)
puts the normalization first and the clearing second.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/DistributionSuite.scala:
##########
@@ -447,6 +448,59 @@ class DistributionSuite extends SparkFunSuite {
assert(interned.partitionKeys eq kpX.partitionKeys)
}
+ test("SPARK-59050: a marked one-partition layout keeps the global ordering
claim") {
+ // The one-partition exemption inside `keysSatisfy`'s ordered branch: a
single partition
+ // holds every row, so an out-of-set key cannot break the cross-partition
sequence, while
+ // two partitions can (the e2e `ORDER BY` repro measures that). Positive
control: an
+ // always-false gate would shuffle these plans for nothing.
+ val a = AttributeReference("a", IntegerType)()
+ val ordered = OrderedDistribution(
+ Seq(org.apache.spark.sql.catalyst.expressions.SortOrder(a,
Review Comment:
Fixed in 74bf881, thanks: `Ascending, SortOrder` joined the existing
`catalyst.expressions` import, `SQLConf` got its own, `InternalRow` is bare,
and the assertions are `checkSatisfied(markedOne, ordered, true)` /
`checkSatisfied(markedTwo, ordered, false)` -- the helper's failure output says
more than my two messages did.
--
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]
