peter-toth commented on PR #58351: URL: https://github.com/apache/spark/pull/58351#issuecomment-5443074052
Thanks for the review. You are right on (2), and it reverses a call I made earlier: I had left the members free to disagree, reasoning that each records its own side's history. But they describe one physical layout and share the key list, so if any side is coarse an output partition really does cover several finer ones, whichever member's expressions name it. And the `find` in `EnsureRequirements` makes that concrete -- a mixed collection reaching a `GroupPartitionsExec` through the plain member is a hole, not a design choice. So `PartitioningCollection.fromPartitionings` now normalizes `isCollapsed` by OR, alongside the `partitionKeys` interning, and `checkKeyedPartitioningInvariant` enforces it. Both stay O(members) per level: one representative per member is enough, since every collection agrees internally by the same construction, and a nested collection is only descended into when it disagrees -- the property your interning code deliberately protects for linearly-nested same-key joins. That answers (1) too: the flag is now computed once in `GroupPartitionsExec.outputPartitioning`, outside the transform, so the two producers no longer disagree. Both doc caveats are gone. On (3): deliberate conservatism, and I have written that at the call site. The shuffled side has no finer layout of its own -- its partitions are what a hash partitioning would give -- but the two sides are co-located on one key set, and a later grouping of that key set carries the coarsened side's risk. It can only add shuffles, never remove one. (4) done: the gate comment is trimmed to what the class doc does not already carry. Also added a unit test that a coarsened member marks the whole collection, including through a nested one. Pushed as a separate commit. -- 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]
