ulysses-you commented on code in PR #58339:
URL: https://github.com/apache/spark/pull/58339#discussion_r3924045689
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/GroupPartitionsExec.scala:
##########
@@ -78,6 +78,18 @@ case class GroupPartitionsExec(
// data types match the reduced partition keys for the
identity-vs-transform and
// single-side-transform reducers; for the both-sides-reduce shape no
single transform
// describes the keys (see `KeyedShuffleSpec.reducersBothWays`).
+ //
+ // A marked claim pins undeclared rows to hash(key) % numPartitions
(see
+ // `KeyedPartitioning.mayContainUnknownPartitionKeys`). Only an
identity grouping keeps
+ // that relationship: any other grouping -- a reorder, a coalesce, a
resize, or the
+ // collapse a reduction applies -- moves those rows. Clearing only the
marker would
+ // misreport the undeclared rows that remain, so give up the keyed
partitioning at the
+ // physical output count (one per group, padding included) that a
parent's
+ // `PartitioningCollection` requires for uniformity.
`identityGrouping` is a lazy val, so
+ // repeated `outputPartitioning` calls scan it at most once.
+ if (PartitioningCollection.keyedMarkerOf(p).contains(true) &&
!identityGrouping) {
Review Comment:
Thank you for the re-review!
**Finding 23** - fixed in `740e6a2`: `identityGrouping` now also requires
the partition count to be unchanged. `alignToExpectedKeys` emits only the
expected keys, so a declared key the merged set drops never enters the
grouping; when it is trailing, every kept group still reads identity while the
count -- and the hash modulus the claim pins -- shrinks. Pinned: a trailing-key
shrink gives up the claim at the physical count; fails without the count check.
**Finding 24** - reworded as suggested.
**Finding 22** - on where the give-up belongs: I thought about moving the
check to the adjacent operators -- having `PartitioningPreservingUnaryExecNode`
and the join test whether their child is a marked `GroupPartitionsExec` and
output unknown there -- but concluded the give-up belongs in
`GroupPartitionsExec` itself. It is the only site that knows whether the
regrouping is identity; a parent only sees "my child is a marked GPE", not
whether anything moved, so it would give up even for identity regroups where
the claim is still valid. It would also be redundant -- once the GPE gives up
to `UnknownPartitioning`, the parents already propagate that from its output.
So the give-up stays in `GroupPartitionsExec`, and the comment now states that
it deliberately under-reports and that a plan containing it does not pass
`ValidateRequirements`.
--
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]
