peter-toth commented on code in PR #58339:
URL: https://github.com/apache/spark/pull/58339#discussion_r3893121466
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/GroupPartitionsExec.scala:
##########
@@ -71,10 +71,31 @@ case class GroupPartitionsExec(
// can only differ in `expressions`; their `partitionKeys` reference
is shared (enforced by
// `PartitioningCollection`), so `groupedPartitions` is computed only
once.
val partitionKeys = groupedPartitions.map(_._1)
+ // Only a collection whose KPs are all unknown-keyed genuinely holds
unknown keys; a
+ // mixed one comes from a `ShuffledJoin` `InnerLike` arm and its
marker is spurious
+ // (see `KeyedPartitioning.mayContainUnknownPartitionKeys`).
+ val kps = p.collect { case k: KeyedPartitioning => k }
+ val mayContainUnknownKeys = kps.nonEmpty &&
kps.forall(_.mayContainUnknownPartitionKeys)
+ // The output declares reduced keys (when `reducers` is defined) or
keys projected to
+ // `joinKeyPositions` -- either coarsens the declared set, which an
unknown-keyed claim
+ // cannot survive. Drop the keyed partitioning entirely: the
regrouping rewrote the
+ // layout, so there is no child claim to fall back to. The `reducers`
term never fires
+ // for the built-in transforms (`areKeysCompatible` pairs an
unknown-keyed spec only with
+ // a same-function partner, which has no reducer); it guards
third-party
+ // `ReducibleFunction`s whose reducer is defined against themselves.
+ val narrowsDeclaredKeys = kps.headOption.exists { kp =>
+ joinKeyPositions.exists(_.length < kp.expressions.length)
+ }
+ if (mayContainUnknownKeys && (reducers.isDefined ||
narrowsDeclaredKeys)) {
Review Comment:
**Finding 14.** No test reaches this branch, so neither the guard nor the
`groupedPartitions.size` count that replaced `UnknownPartitioning(0)` is
covered.
Measured on `9a450ca8e00`:
- a `println` inside the branch, then `KeyGroupedPartitioningSuite`,
`GroupPartitionsExecSuite`, `EnsureRequirementsSuite`,
`ValidateRequirementsSuite` and my probes: **0 hits across 181 tests**;
- `mayContainUnknownKeys` is true 83 times, always with `joinKeyPositions =
Some(List(0))` against an arity-1 partitioning and `reducers = false`, so
neither term of the `||` ever holds;
- replacing `UnknownPartitioning(groupedPartitions.size)` with
`UnknownPartitioning(0)` — the value that threw at planning in round 2 — leaves
all 138 tests green.
That reads as expected rather than wrong: `createShuffleSpec` now refuses
one hop earlier, so `applyGroupPartitions` never hands an unknown-keyed side a
narrowing `joinKeyPositions`, and the `reducers` term is already documented as
unreachable for the built-in transforms. If that is the conclusion, say it here
the way the `reducers` sentence already does, so a later reader does not take
the count for tested behaviour. If you can reach it — a third-party
`ReducibleFunction` whose reducer is defined against itself would — a test
pinning the partition count keeps the round-2 planning throw from coming back.
--
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]
