szehon-ho commented on code in PR #58351:
URL: https://github.com/apache/spark/pull/58351#discussion_r3875988956
##########
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:
this is just collapsed right? is another new term necessary? is 'grouping'
ok?
On that note, wdyt we have a quick glossary in the top of the section for
the terms we are defining here?
- Key collapse: A projection or reduction maps two distinct old keys to the
same new key.
(1,A), (1,B) -> 1, 1
- Grouping: Physically combines partitions that now share the same key.
1, 1, 2 -> 1, 2
Maybe its me but the javadoc is a bit hard to read , i think example is
worth 1000 words
##########
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:
this is just collapsed right? is another new term necessary? is 'grouping'
ok?
On that note, wdyt we have a quick glossary somewhere for the terms we are
defining here?
- Key collapse: A projection or reduction maps two distinct old keys to the
same new key.
(1,A), (1,B) -> 1, 1
- Grouping: Physically combines partitions that now share the same key.
1, 1, 2 -> 1, 2
Maybe its me but the javadoc is a bit hard to read , i think example is
worth 1000 words
--
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]
