peter-toth commented on code in PR #58351: URL: https://github.com/apache/spark/pull/58351#discussion_r3879299898
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala: ########## @@ -478,19 +478,45 @@ case class CoalescedNullAwareHashPartitioning( * unique partition keys, or (2) `GroupPartitionsExec` coalesces partitions with duplicate keys. * * == Distribution Satisfaction and Grouping == - * Besides the default `satisfies()`, `KeyedPartitioning` exposes two additional methods used by - * `EnsureRequirements` to handle grouped and non-grouped KPs separately: + * Besides the default `satisfies()`, `KeyedPartitioning` exposes two additional methods: * - * - `nonGroupedSatisfies()`: called on non-grouped KPs to check as-is satisfaction (without - * inserting `GroupPartitionsExec`). - * - `groupedSatisfies()`: called on non-grouped KPs to check whether inserting - * `GroupPartitionsExec` would satisfy the distribution. When it returns true, the distribution - * is NOT yet satisfied -- `EnsureRequirements` will insert `GroupPartitionsExec` to coalesce - * duplicate partition keys. + * - `nonGroupedSatisfies()`: as-is satisfaction (without inserting `GroupPartitionsExec`). It is + * the default `Partitioning` implementation, so for a `ClusteredDistribution` it is always false. + * - `groupedSatisfies()`: whether the distribution would be satisfied once the duplicate partition + * keys are coalesced. It has two callers, and they ask different questions: + * - `EnsureRequirements` calls it on non-grouped KPs to ask whether inserting a + * `GroupPartitionsExec` would help. When it returns true, the distribution is NOT yet + * satisfied -- `EnsureRequirements` will insert one to coalesce the duplicate keys. + * - `satisfies0()` calls it on grouped KPs. Since `nonGroupedSatisfies()` is false for a + * `ClusteredDistribution`, this is the only route by which a grouped KP satisfies one, and no + * grouping is involved: the keys are already unique. + * + * That second caller is why the coarsening guard in `groupedSatisfies()` is a conjunction with + * `!isGrouped`. A coarsened KP can be grouped again -- by `GroupPartitionsExec`, or by reducing its + * keys onto a coarser transform -- and dropping the `!isGrouped` term would stop such a KP from + * satisfying a `ClusteredDistribution` and cost it a shuffle, even though grouping it would merge + * nothing. * * For `OrderedDistribution`, `GroupPartitionsExec` must also sort the partition keys to meet the * ordering requirement. * + * == Coarsened Partitionings == Review Comment: Good point, and dongjoon made the same one about the section title -- "coarsened" was a second word for what `isCollapsed` already names. Unified on collapse, and the word is gone from all the touched files. The section is now `== Key Collapse ==` and opens with your two entries, with the examples: - **Key collapse**: a projection or a reduction maps two keys that were distinct onto the same new key. `[(1, 'a'), (1, 'b'), (2, 'c')]` projected onto the first position gives `[1, 1, 2]`: three distinct keys became two. `isCollapsed` records this. - **Grouping**: `GroupPartitionsExec` physically combines the partitions that share a key. `[1, 1, 2]` becomes `[1, 2]`. `isGrouped` says the keys are unique, however they got that way -- a source with natively unique keys reports it too. Then one paragraph on why the pair matters: grouping *after* a collapse is what produces a partition larger than any the source declared, which is what needs the config; grouping without one only merges partitions that already shared a key, and needs no opt-in. Writing it out also caught a real bug in the config doc, which was still phrased the old way: it claimed the config gates "a partitioning that was narrowed to a subset of its keys and whose keys are no longer distinct". Both halves are false now, and two of this PR's own tests prove it -- a subset projection that keeps every distinct key is not gated, and a reduction that drops no key position at all is. Fixed in the same wording as the class doc. -- 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]
