peter-toth commented on code in PR #58338:
URL: https://github.com/apache/spark/pull/58338#discussion_r3872839769
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -579,7 +581,17 @@ case class KeyedPartitioning(
def groupedSatisfies(required: Distribution): Boolean = {
required match {
case c @ ClusteredDistribution(requiredClustering,
requireAllClusterKeys, _, _) =>
- if (requireAllClusterKeys) {
+ if (isNarrowed && !isGrouped &&
Review Comment:
You are right, and this was the most useful comment on the PR - I had
convinced myself the conjunct was redundant. It is not: `nonGroupedSatisfies`
is the default `Partitioning` implementation, so a grouped KP reaches a
`ClusteredDistribution` only through `groupedSatisfies`, and a narrowing
projection that keeps its keys distinct is precisely a grouped narrowed KP.
Dropping the term would cost it a shuffle, and the test you name pins that.
Fixed the class-level doc to describe both callers and to record the reason,
and added a note at the guard itself.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -579,7 +581,17 @@ case class KeyedPartitioning(
def groupedSatisfies(required: Distribution): Boolean = {
required match {
case c @ ClusteredDistribution(requiredClustering,
requireAllClusterKeys, _, _) =>
- if (requireAllClusterKeys) {
+ if (isNarrowed && !isGrouped &&
+ !SQLConf.get.v2BucketingAllowKeysSubsetOfPartitionKeys) {
+ // A narrowed, non-grouped partitioning carries the same skew risk
as using a subset of
+ // partition keys for a join: GroupPartitionsExec will merge
partitions that held
Review Comment:
Done, in the guard comment and in the `isNarrowed` scaladoc, together with
the reason for "may": the duplicate keys can also come from a source reporting
several splits per partition key.
--
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]
