szehon-ho commented on code in PR #58351: URL: https://github.com/apache/spark/pull/58351#discussion_r3884232342
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala: ########## @@ -491,14 +491,40 @@ case class CoalescedNullAwareHashPartitioning( * `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 narrowing guard in `groupedSatisfies()` is a conjunction with - * `!isGrouped`. A narrowed KP whose projected keys stayed distinct is grouped, and dropping the - * `!isGrouped` term would stop it from satisfying a `ClusteredDistribution` and cost it a shuffle, - * even though grouping it would merge nothing. + * That second caller is why the collapse guard in `groupedSatisfies()` is a conjunction with + * `!isGrouped`. A collapsed KP can end up grouped -- 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. * + * == Key Collapse == + * Two things happen to partition keys, and only the first is a loss of granularity: + * + * - '''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. + * + * Grouping after a collapse is what produces a partition holding more data than any the source + * declared -- the two `1` partitions above came from different `(1, 'a')` and `(1, 'b')` keys -- so + * it needs `allowKeysSubsetOfPartitionKeys`. Grouping without a collapse only merges partitions + * that already shared a key (a source reporting several splits per key, or a union of children that + * overlap), and needs no opt-in. `OrderedDistribution` is not gated at all: `GroupPartitionsExec` + * pads that path out to the expected split counts rather than coalescing, so nothing is merged. + * + * A collapsed partitioning is still kept rather than dropped to `UnknownPartitioning`, because Review Comment: Could we state the two cases explicitly here? - Collapsed and ungrouped: duplicate keys remain, so `ClusteredDistribution` is refused when the config is off. - Collapsed and grouped: keys are already unique, so it is accepted regardless of the config. The current paragraph mixes these states, which makes “a collapsed partitioning” ambiguous. -- 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]
