ulysses-you commented on PR #58339: URL: https://github.com/apache/spark/pull/58339#issuecomment-5525053133
Thank you @peter-toth 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]
