peter-toth commented on PR #58262: URL: https://github.com/apache/spark/pull/58262#issuecomment-5438648022
@ulysses-you @dongjoon-hyun so you both know what I am planning in this area, here is what I have collected while working on this. Each will get its own JIRA as I get to it. 1. **Reduced SPJ partition keys: the reported expressions still do not describe them.** #58335 fixes the identity-versus-transform shape. Two remain. When both sides are transforms and only one reduces, an exact expression does exist - the other side's transform retargeted at this side's child - but the un-reduced one is reported. When both sides reduce, the keys land in a space no transform describes, and the `ClassCastException` survives; I measured that on #58335's head with the existing `SPARK-56164` test plus `allowKeysSubsetOfPartitionKeys`. The stale expression is also a silent wrong-results bug at a chained join, and that one reaches released branches. This is the follow-up I offered on #58335, and I am happy either way: I can take it, or carry it after whatever you do there. 2. **The multi-child block pushes the wrong join key positions.** `withJoinKeyPositions` is handed the *best* spec's positions for every child rather than each child's own `specs(idx)`, and `KeyedShuffleSpec.isCompatibleWith` compares the projected partitionings without comparing the positions. So two sides can be compatible while their positions differ, and one side ends up grouped on the wrong column. Pre-existing, silent wrong results. SPARK-59025 makes it reachable in one more shape, because the head of an unwrapped `ShuffleSpecCollection` now supplies the positions for a child whose own spec may carry different ones. 3. **`ShuffleSpecCollection` members can disagree on `numPartitions`.** Under `allowKeysSubsetOfPartitionKeys`, `KeyedPartitioning.createShuffleSpec` projects each member of a `PartitioningCollection` to *its own* join-key subset, so the resulting specs can differ in `numPartitions`. `ShuffleSpecCollection.numPartitions` then reports the head's count while `createPartitioning` requires them all equal and throws. Reproduces both before and after SPARK-59025, so it is independent of that change. 4. **A coalesce keeps a stale non-`KeyedPartitioning` sibling.** `GroupPartitionsExec.outputPartitioning` rewrites only the `KeyedPartitioning` members of its child's partitioning and passes any other member through with its old `numPartitions`, so rebuilding the enclosing `PartitioningCollection` trips its equal-count requirement. No producer builds such a collection from a query today, so this is reachable only from a hand-built partitioning. The repair belongs in the node: after coalescing, an inherited `HashPartitioning` is not mis-counted, it is false, and should be dropped. 5. **Make the validation check strict.** `satisfies` answers `true` for a partitioning that still needs a projection, so `ValidateRequirements` cannot catch a missing `GroupPartitionsExec`. Giving that check the strict test, and housing admission and projection next to each other so the two cannot drift apart, is the shape I want. This is what @dongjoon-hyun asked about in his review here. -- 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]
