You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2026/08/26 17:09:03 UTC
[PR] [SPARK-59027][SQL] Ensure `createShuffleSpec` and `GroupPartitionsExec` sort grouped partition keys identically [spark]
dongjoon-hyun opened a new pull request, #58318:
URL: https://github.com/apache/spark/pull/58318
### What changes were proposed in this pull request?
Extract the ordering of grouped partition keys into a shared helper, `KeyedPartitioning.groupedKeyRowOrdering`, and use it from both `KeyedPartitioning.keyRowOrdering` (used by `toGrouped`, and thus by `createShuffleSpec`'s subset-keys branch) and `GroupPartitionsExec.groupAndSortByKeys`. Add a test pinning that the two orders agree.
### Why are the changes needed?
With `spark.sql.sources.v2.bucketing.allowKeysSubsetOfPartitionKeys.enabled`, one join side may be shuffled onto the key order declared by `createShuffleSpec`, while the keyed side's physical layout is produced by `GroupPartitionsExec`. The two sorts must be identical, or the sides are silently mis-aligned at runtime. Today they agree only because both call `RowOrdering.createNaturalAscendingOrdering` on the same data types, and the requirement was recorded only in a comment -- the same comment-only ordering coupling that regressed in SPARK-59022. This PR is behavior-neutral and makes the contract shared, documented, and tested.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass the CIs with the newly added test case.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Fable 5
--
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-59027][SQL] Share grouped partition key ordering between `createShuffleSpec` and `GroupPartitionsExec` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #58318: [SPARK-59027][SQL] Share grouped partition key ordering between `createShuffleSpec` and `GroupPartitionsExec`
URL: https://github.com/apache/spark/pull/58318
--
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-59027][SQL] Share grouped partition key ordering between `createShuffleSpec` and `GroupPartitionsExec` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58318:
URL: https://github.com/apache/spark/pull/58318#issuecomment-5432762220
All tests passed
--
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-59027][SQL] Ensure `createShuffleSpec` and `GroupPartitionsExec` sort grouped partition keys identically [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58318:
URL: https://github.com/apache/spark/pull/58318#issuecomment-5429408987
Thank you for the careful review, @peter-toth!
**On 1:** You are right about the inner-join case, and about the mechanism: `ShuffledJoin` builds a `PartitioningCollection` from both sides only for inner-like joins, and on the subset-keys path the two `partitionKeys` are separate objects built by `projectKeys`, so the `eq` fast path does not apply and SPARK-56877's `require` runs. I reproduced your experiment and got the same planning-time failure on the `SPARK-58988` test.
That loudness is a side effect of inner joins exposing both sides' partitionings, though, not a guard on the physical layout. For join types where `ShuffledJoin.outputPartitioning` exposes only one side (`LeftOuter`, `RightOuter`, `LeftSemi`, `LeftAnti`), nothing compares the two orders. With the same reversed ordering, a LEFT JOIN variant of the `SPARK-58988` scenario plans fine (same shape: one shuffle, one `GroupPartitionsExec`) and silently returns wrong results:
```
!== Correct Answer - 3 == == Spark Answer - 3 ==
![1,aa,2021,1,aa] [1,aa,2021,null,null]
```
I updated the scaladoc, the test comment, and the PR description to state both failure modes precisely instead of claiming the divergence is always silent, and added that LEFT JOIN variant to `KeyGroupedPartitioningSuite` as a regression test -- for the one-sided join types the answer check is the only guard, and it also covers the shuffle partitioner's physical layout, which the unit test in `GroupPartitionsExecSuite` does not reach.
**On 2:** Fair point -- SPARK-59022 was `getPartitioner` deriving a third order that ignored the declared one, not two sorts drifting apart. I reworded the description to call it a related but distinct hazard, and to note this coupling was found during the review of #58311.
--
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-59027][SQL] Share grouped partition key ordering between `createShuffleSpec` and `GroupPartitionsExec` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58318:
URL: https://github.com/apache/spark/pull/58318#issuecomment-5429639378
Thank you, @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-59027][SQL] Ensure `createShuffleSpec` and `GroupPartitionsExec` sort grouped partition keys identically [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58318:
URL: https://github.com/apache/spark/pull/58318#issuecomment-5428515430
cc @peter-toth , too.
--
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-59027][SQL] Share grouped partition key ordering between `createShuffleSpec` and `GroupPartitionsExec` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58318:
URL: https://github.com/apache/spark/pull/58318#issuecomment-5432769427
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/8c4c00d9fe864c9e15eae11b313e0357a54fd734
- merged into branch-4.x https://github.com/apache/spark/commit/a1d7b2a9647865fe15a09a79b4443b21208f08a1
- merged into branch-4.3 https://github.com/apache/spark/commit/fd87d09f88299264a2882ba649e82349daa90de2
*Posted by `merge_spark_pr.py`*
--
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-59027][SQL] Share grouped partition key ordering between `createShuffleSpec` and `GroupPartitionsExec` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #58318:
URL: https://github.com/apache/spark/pull/58318#issuecomment-5430026642
Since I merged the following, I'm going to rebase to the master branch.
- https://github.com/apache/spark/pull/58311
--
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