You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "peter-toth (via GitHub)" <gi...@apache.org> on 2026/04/24 13:20:40 UTC
[PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
peter-toth opened a new pull request, #55538:
URL: https://github.com/apache/spark/pull/55538
### What changes were proposed in this pull request?
`KeyedPartitioning.satisfies0` previously delegated unconditionally to both `nonGroupedSatisfies` and `groupedSatisfies`, meaning an ungrouped KP would incorrectly claim to satisfy `ClusteredDistribution` if called directly. The correct form is:
nonGroupedSatisfies(required) || (isGrouped && groupedSatisfies(required))
`EnsureRequirements` never calls `satisfies()` on an ungrouped `KeyedPartitioning` (it uses `splitKeyedPartitionings` and calls the two helpers directly), so the missing guard has had no runtime effect. This PR adds the guard to make the semantics correct for any future direct caller.
Also adds documentation to `KeyedPartitioning` and `EnsureRequirements` explaining the intended call contract: `groupedSatisfies` answers "would this KP satisfy if it were grouped?", and `EnsureRequirements` calls it directly on non-grouped KPs to decide whether inserting `GroupPartitionsExec` would satisfy the distribution.
### Why are the changes needed?
The missing `isGrouped &&` guard in `satisfies0` is a latent correctness issue. The documentation clarifies a subtle and previously undocumented contract between `KeyedPartitioning` and `EnsureRequirements`.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
No new tests needed -- the fix aligns the implementation with the contract that `EnsureRequirements` already relies on. Existing `KeyGroupedPartitioningSuite` and related suites cover the behaviour.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Sonnet 4.6
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #55538:
URL: https://github.com/apache/spark/pull/55538#discussion_r3139008973
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -472,7 +466,7 @@ case class KeyedPartitioning(
KeyedPartitioning.reduceKeys(partitionKeys, expressionDataTypes, reducers)
override def satisfies0(required: Distribution): Boolean = {
- nonGroupedSatisfies(required) || groupedSatisfies(required)
+ nonGroupedSatisfies(required) || (isGrouped && groupedSatisfies(required))
Review Comment:
Got it, @peter-toth .
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #55538:
URL: https://github.com/apache/spark/pull/55538#issuecomment-4314671538
@dongjoon-hyun , @cloud-fan, @szehon-ho, @yaooqinn
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth closed pull request #55538: [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract
URL: https://github.com/apache/spark/pull/55538
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #55538:
URL: https://github.com/apache/spark/pull/55538#discussion_r3139004057
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -472,7 +466,7 @@ case class KeyedPartitioning(
KeyedPartitioning.reduceKeys(partitionKeys, expressionDataTypes, reducers)
override def satisfies0(required: Distribution): Boolean = {
- nonGroupedSatisfies(required) || groupedSatisfies(required)
+ nonGroupedSatisfies(required) || (isGrouped && groupedSatisfies(required))
Review Comment:
Technically, the missing `isGrouped &&` was not an issue as we called this only on grouped `KeyedPartitioning`s.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #55538:
URL: https://github.com/apache/spark/pull/55538#discussion_r3139004057
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -472,7 +466,7 @@ case class KeyedPartitioning(
KeyedPartitioning.reduceKeys(partitionKeys, expressionDataTypes, reducers)
override def satisfies0(required: Distribution): Boolean = {
- nonGroupedSatisfies(required) || groupedSatisfies(required)
+ nonGroupedSatisfies(required) || (isGrouped && groupedSatisfies(required))
Review Comment:
Technically, the missing `isGrouped &&` was not an issue as we called this only on grouped `KeyedPartitioning`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #55538:
URL: https://github.com/apache/spark/pull/55538#issuecomment-4318385926
Thank you @dongjoon-hyun and @yaooqinn for the review!
Merged to `master` (4.2.0).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #55538:
URL: https://github.com/apache/spark/pull/55538#issuecomment-4314730331
I got your point, but it would be great if we can have a small unit test case specifically to prevent any regressions (not from this PR) for future proof.
> No new tests needed -- the fix aligns the implementation with the contract that EnsureRequirements already relies on. Existing KeyGroupedPartitioningSuite and related suites cover the behaviour.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-56615][SQL] Clarify `KeyedPartitioning` `satisfies0()` and `nonGroupedSatisfies()`/`groupedSatisfies()` contract [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #55538:
URL: https://github.com/apache/spark/pull/55538#issuecomment-4314955606
> I got your point, but it would be great if we can have a small unit test case specifically to prevent any regressions (not from this PR) for future proof. Of course, this is nit.
Added a simple test in https://github.com/apache/spark/pull/55538/commits/cb2c03e3253576961d8eb201386949aa4a8e0556.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org