szehon-ho commented on code in PR #58858:
URL: https://github.com/apache/spark/pull/58858#discussion_r4031580580
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/EnsureRequirements.scala:
##########
@@ -541,17 +541,52 @@ case class EnsureRequirements(
def bothUnprojected(l: KeyedShuffleSpec, r: KeyedShuffleSpec): Boolean =
l.joinKeyPositions.isEmpty && r.joinKeyPositions.isEmpty
+ // Whether the merged key list below may be narrowed to what the join type
allows. A marked
+ // layout is left alone: only an identity regrouping keeps its claim (see
+ // `GroupPartitionsExec`), and losing it costs the pair its join at the
gate at the end of this
+ // method.
+ //
+ // Filtering is then the only thing that can shrink the merged list
*below* the marked side's
+ // own declared keys. The merging arms take the union, and
`KeyedShuffleSpec.areKeysCompatible`
+ // pairs a marked layout only with one whose keys are a subset of its
declared keys, so the
+ // union is that side's own key set. The count its hash is taken modulo
survives the dedup
+ // because a marked layout is always grouped: `canCreatePartitioning` is
the only producer of
+ // the marker and it refuses an ungrouped one. A reduce is the other way a
merged list comes
+ // out smaller, and it cannot happen here, since the marked arm of
`areKeysCompatible` admits
+ // only positions holding the same transform function and
`reducersBothWays` finds nothing to
+ // reduce between those.
+ //
+ // What filtering does instead: an intersection with a strictly smaller
partner, or the
+ // one-sided arm that keeps the *other* side's keys, drops groups the
marked side holds, and
+ // the regrouping stops being the identity. The pair would then trade its
whole join for
+ // pruning those groups.
+ //
+ // Sorting is the other way a regrouping stops being the identity, and is
not addressed here.
+ // `mergeAndDedupPartitions` sorts, so a marked layout whose declared
order is not the sorted
+ // one is relabelled even where the set is unchanged. Handing it its own
list verbatim looks
+ // like the same fix and is not, because
`KeyedPartitioning.createShuffleSpec` sorts through
+ // `toGrouped` under `v2BucketingAllowKeysSubsetOfPartitionKeys` while it
hands a marked layout
+ // back unprojected: the two children would hold one partitioning and
report two specs that
+ // `describesSameKeys` calls different, and `ValidateRequirements` rejects
the join this method
+ // just allowed. Measured on the generated sweep in
`EnsureRequirementsSuite`, cell
Review Comment:
the comment is a bit too detailed, even mentioning a specific unit test
here. seems it will not be easily maintianed, can we shorten it to the basic
idea? (in follow up)
--
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]
