[SPARK-58996][SQL][4.2] Fix SPJ partially clustered data correctness when EnsureRequirements re-runs - #58561
Closed
ulysses-you wants to merge 1 commit into
Closed
Conversation
…EnsureRequirements re-runs ### What changes were proposed in this pull request? `EnsureRequirements` is not idempotent for a storage-partitioned join that uses a partially clustered distribution: re-running it on a plan it already produced stacks a second `GroupPartitionsExec` on top of the first, and the result duplicates rows. The re-run is not an AQE quirk: `AdaptiveSparkPlanExec` builds one `EnsureRequirements` instance, and `ConvertSortMergeJoinToShuffledHashJoin` and `OptimizeSkewedJoin` both hand the whole tree back to that same instance after rewriting some other join, all within one `queryStagePreparationRules` pass. Idempotency is therefore a requirement of the current rule composition. On that second pass a join child is `SortExec(GroupPartitionsExec(...))` rather than a bare scan. A partially clustered `KeyedPartitioning` reports `isGrouped = false` by design, so the distribution step treats it as satisfied "only after grouping" and adds a plain `GroupPartitionsExec` on top; `applyGroupPartitions` then writes the join's `expectedPartitionKeys` and `distributePartitions` into that fresh outer node. The alignment is therefore re-derived from an already-aligned layout. The inner node replicates an input partition across the expected partitions, and the outer node concatenates those replicas back into a single partition before replicating again, so every row of the replicated side is emitted twice: ``` outer GroupPartitions groupedPartitions = [([1],[0,1]), ([1],[0,1]), ([2],[2])] inner GroupPartitions groupedPartitions = [([1],[0]), ([1],[0]), ([2],[1])] ``` The fix makes the rule reuse what an earlier pass inserted instead of deriving from it: * `innermostGroupPartition` is the single descent through the nodes this rule itself inserted directly above the child: a `GroupPartitionsExec` and a local `SortExec`. A `GroupPartitionsExec` hidden behind any other node belongs to a different operator and is never reused. Both the rewrite and the reads below go through it. * `rewriteGroupPartitions` rewrites the innermost `GroupPartitionsExec` and drops what sits above it, reproducing the plan a single pass would have produced. Only `applyGroupPartitions` calls it, reached from `checkKeyGroupCompatible`, which runs for joins alone. * `unwrapGroupPartitions` peels down to the pre-alignment plan along the same descent, so the statistics-based replicate-side choice and the original partition keys below are read from that plan on every pass. Peeling one level reads them from the sort this rule added, which carries no `logicalLink` and reports the aligned layout, and deterministically flips the replicate-side choice. The positions projecting the original keys come from the innermost grouping as well, for the same reason `applyGroupPartitions` keeps them, and the partition-count fallback compares the pre-alignment split counts for the same reason. * `applyGroupPartitions` keeps the `joinKeyPositions` a reused node already holds: they were computed against the node child's raw partition keys, while the incoming ones were computed against the node's own, already projected report, and applying them would project a second time. * `withJoinKeyPositions`, reached from the children loop for every multi-child operator and not just joins, reuses only a topmost `GroupPartitionsExec` and does not descend. * The `ShuffleExchangeExec` site strips every grouping this rule inserted instead of one level: a replicating grouping repeats every row, so none of them may feed a shuffle. ### Why are the changes needed? The join returns duplicated rows. Reproduction, against a DSv2 catalog that reports `KeyGroupedPartitioning` with one split per row (the in-memory test catalog does this with `numRowsPerSplit = 1`; on a real connector the same shape is a partition value with more than one data file whose files are not combined into a single task): ``` spark.sql.sources.v2.bucketing.enabled=true spark.sql.sources.v2.bucketing.pushPartValues.enabled=true spark.sql.sources.v2.bucketing.partiallyClusteredDistribution.enabled=true spark.sql.adaptive.maxShuffledHashJoinLocalMapThreshold=100m ``` ```sql CREATE TABLE sp1 (id BIGINT, data STRING) PARTITIONED BY (id); INSERT INTO sp1 VALUES (1, 'aa'), (1, 'ab'), (2, 'bb'); -- two splits for id = 1 CREATE TABLE sp2 (id BIGINT, data STRING) PARTITIONED BY (id); INSERT INTO sp2 VALUES (1, 'p'), (2, 'q'); CREATE TABLE np1 (id BIGINT, data STRING); INSERT INTO np1 VALUES (7, 'x'); CREATE TABLE np2 (id BIGINT, data STRING); INSERT INTO np2 VALUES (7, 'y'); SELECT /*+ MERGE(a, b) */ a.id AS k FROM sp1 a JOIN sp2 b ON a.id = b.id UNION ALL SELECT c.id AS k FROM np1 c JOIN np2 d ON c.id = d.id; ``` Returns `(1, 1, 1, 1, 2, 7)`; the correct answer is `(1, 1, 2, 7)`. One detail is load-bearing. The `np1`/`np2` branch exists only to create a materialized shuffle stage, which is what makes `ConvertSortMergeJoinToShuffledHashJoin` fire and re-run `EnsureRequirements` over the whole plan -- the storage-partitioned join has no shuffle of its own, so it cannot trigger the re-run by itself. A single pass is self-consistent: within one `ensureDistributionAndOrdering` call the distribution step creates the `GroupPartitionsExec` and `applyGroupPartitions` rewrites that same node. The stacking only appears once the rule is applied to its own output. ### Does this PR introduce _any_ user-facing change? Yes. It fixes wrong results (duplicated rows) for a storage-partitioned join under a partially clustered distribution when AQE re-runs `EnsureRequirements`. For the query above, the result changes from `(1, 1, 1, 1, 2, 7)` to the correct `(1, 1, 2, 7)`. It also changes the plan in one corner: the partition-count fallback that chooses the replicate side when there are no plan statistics now compares the pre-alignment split counts instead of the aligned report's distinct keys. On a first pass of a query where one side holds more than one split per key, the replicated side can differ from earlier releases (the side with fewer splits is replicated, which is the cheaper choice). ### How was this patch tested? New tests: * `KeyGroupedPartitioningSuite`, "partially clustered join keeps its row count when EnsureRequirements re-runs" -- the end-to-end query above, whose row count was wrong before the fix. It also asserts the storage-partitioned side stays shuffle-free and no `GroupPartitionsExec` is stacked over another. * `KeyGroupedPartitioningSuite`, "partially clustered join keeps its replicate-side choice when EnsureRequirements re-runs" -- the smaller side is replicated by plan statistics on the first pass. The data keeps rows on both sides of the multi-split key, so the re-run's flipped choice returns wrong results already on master, and the smaller side's five splits for id = 1 pin the guard: the flipped distribute side overflows the expected count of one (`padTo` never truncates) and the join sides end up with an unequal number of partitions. * `KeyGroupedPartitioningSuite`, "partially clustered subset-key join keeps its join key positions when EnsureRequirements re-runs" -- the left table is partitioned by `(extra, id)` and the join uses only `id`, the second partition key. On master the stacked re-grouping duplicates rows; without the position fix the re-run projects the pre-alignment keys with aligned-space positions and throws the `PartitioningCollection` invariant. * `EnsureRequirementsSuite`, "only a local sort is looked through when reusing GroupPartitionsExec" -- covers the bare, local-sort, stacked and global-sort cases, so a `GroupPartitionsExec` serving a global sort's `OrderedDistribution` is never reused. * `EnsureRequirementsSuite`, "withJoinKeyPositions reuses only a topmost GroupPartitionsExec" -- non-join multi-child operators reach this path, so it must not descend to nodes that may belong to another operator. * `EnsureRequirementsSuite`, "reusing a GroupPartitionsExec keeps its tags" -- tags are instance state a `copy` does not carry, so every rewrite copies them back. * `EnsureRequirementsSuite`, "the shuffle site strips every grouping the rule inserted" -- pins the `ShuffleExchangeExec` site's descent directly, as no query reaches its stacked shape today. * `EnsureRequirementsSuite`, "the replicate-side fallback counts pre-alignment partitions" -- forces the statistics-less fallback and pins that it counts the pre-alignment splits rather than the aligned reports. * `EnsureRequirementsSuite`, "a single-child operator over a partially clustered layout still gets grouped" -- pins the intentional non-idempotence of the children loop's wrap for single-child operators. Existing suites run locally: `KeyGroupedPartitioningSuite`, `GroupPartitionsExecSuite`, `ProjectedOrderingAndPartitioningSuite`, `EnsureRequirementsSuite`, `PlannerSuite`, `AdaptiveQueryExecSuite`, `DataFrameJoinSuite`, `DisableUnnecessaryBucketedScanWithoutHiveSupportSuite(AE)`. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes apache#58279 from ulysses-you/worktree-dedup-grouppartition. Authored-by: Xiduo You <ulyssesyou18@gmail.com> Signed-off-by: Xiduo You <ulyssesyou@apache.org> (cherry picked from commit 0b5e7d8) Signed-off-by: Xiduo You <ulyssesyou@apache.org>
ulysses-you
commented
Sep 7, 2026
| * One [[KeyedPartitioning]] standing for every one in this partitioning, if there is any. By the | ||
| * invariant in the class doc, any of them describes the layout. | ||
| */ | ||
| private[physical] def representativeOf(p: Partitioning): Option[KeyedPartitioning] = p match { |
Contributor
Author
There was a problem hiding this comment.
this is an additionally introducing for branch-4.2
peter-toth
approved these changes
Sep 7, 2026
ulysses-you
added a commit
that referenced
this pull request
Sep 7, 2026
…when EnsureRequirements re-runs ### What changes were proposed in this pull request? Backport of SPARK-58996 (#58279, already merged to master, `branch-4.x` and `branch-4.3`) to `branch-4.2`. `EnsureRequirements` is not idempotent for a storage-partitioned join under a partially clustered distribution: re-running it on a plan it already produced stacks a second `GroupPartitionsExec` over the first and duplicates rows. The fix makes the rule reuse the grouping an earlier pass inserted instead of re-deriving the alignment from an already-aligned layout: - `innermostGroupPartition` descends only through a `GroupPartitionsExec` and the local `SortExec` this rule itself added; both the rewrite and the reads below go through it. - `rewriteGroupPartitions` rewrites the innermost node and drops redundant groupings stacked above it, reproducing the plan a single pass would have produced. - `unwrapGroupPartitions` peels to the pre-alignment plan so the statistics and original partition keys are read from that plan on every pass. - `applyGroupPartitions` keeps the `joinKeyPositions` a reused node already holds (they were computed against the raw partition keys) instead of re-projecting a second time. - `withJoinKeyPositions`, which every multi-child operator reaches and not just joins, reuses only a topmost `GroupPartitionsExec`. - The `ShuffleExchangeExec` site strips every grouping this rule inserted before re-shuffling. On `branch-4.2` this additionally introduces a small `PartitioningCollection` companion object (with `representativeOf` and `numKeyedPartitions`) in `partitioning.scala`, mirroring master; `branch-4.2`'s `PartitioningCollection` has no companion object yet. ### Why are the changes needed? The join returns duplicated rows when AQE re-runs `EnsureRequirements`. The re-run is not an AQE quirk: `AdaptiveSparkPlanExec` builds one rule instance, and `ConvertSortMergeJoinToShuffledHashJoin` and `OptimizeSkewedJoin` hand the whole tree back to it after rewriting some other join, all within one `queryStagePreparationRules` pass. A join child then arrives as `SortExec(GroupPartitionsExec(...))`; the partially clustered `KeyedPartitioning` reports `isGrouped = false` by design, so the distribution step adds a plain `GroupPartitionsExec` on top, and `applyGroupPartitions` rewrites that fresh outer node instead of the inherited one. A replicating grouping repeats every row, so the row set is emitted twice. Reproduction: ``` spark.sql.sources.v2.bucketing.enabled=true spark.sql.sources.v2.bucketing.pushPartValues.enabled=true spark.sql.sources.v2.bucketing.partiallyClusteredDistribution.enabled=true spark.sql.adaptive.maxShuffledHashJoinLocalMapThreshold=100m ``` ```sql CREATE TABLE sp1 (id BIGINT, data STRING) PARTITIONED BY (id); INSERT INTO sp1 VALUES (1, 'aa'), (1, 'ab'), (2, 'bb'); -- two splits for id = 1 CREATE TABLE sp2 (id BIGINT, data STRING) PARTITIONED BY (id); INSERT INTO sp2 VALUES (1, 'p'), (2, 'q'); CREATE TABLE np1 (id BIGINT, data STRING); INSERT INTO np1 VALUES (7, 'x'); CREATE TABLE np2 (id BIGINT, data STRING); INSERT INTO np2 VALUES (7, 'y'); SELECT /*+ MERGE(a, b) */ a.id AS k FROM sp1 a JOIN sp2 b ON a.id = b.id UNION ALL SELECT c.id AS k FROM np1 c JOIN np2 d ON c.id = d.id; ``` returns `(1, 1, 1, 1, 2, 7)` instead of `(1, 1, 2, 7)`. ### Does this PR introduce _any_ user-facing change? Yes. It fixes wrong results (duplicated rows) for a storage-partitioned join under a partially clustered distribution when AQE re-runs `EnsureRequirements`. As on master, the partition-count fallback that picks the replicate side when there are no plan statistics now compares the pre-alignment split counts instead of the aligned report's distinct keys, which can change the replicated side on a first pass. ### How was this patch tested? - `KeyGroupedPartitioningSuite`: new tests for the row count, the replicate-side choice, and the subset-key join key positions under an `EnsureRequirements` re-run. The subset-key test sets `spark.sql.requireAllClusterKeysForCoPartition=false`, because on `branch-4.2` a join on a subset of the partition keys only engages storage-partitioned join with that config off. - `EnsureRequirementsSuite`: new unit tests for the local-sort reuse bound, topmost-only `withJoinKeyPositions` reuse, tag preservation, the shuffle site, the fallback counting, and the intentional single-child wrap. - Both suites pass on `branch-4.2`. Closes #58561 from ulysses-you/spark-58996-backport-4.2. Authored-by: Xiduo You <ulyssesyou18@gmail.com> Signed-off-by: Xiduo You <ulyssesyou@apache.org>
Contributor
Author
|
Merge Summary:
Posted by |
Contributor
Author
|
thank you @peter-toth |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Backport of SPARK-58996 (#58279, already merged to master,
branch-4.xandbranch-4.3) tobranch-4.2.EnsureRequirementsis not idempotent for a storage-partitioned join under a partially clustered distribution: re-running it on a plan it already produced stacks a secondGroupPartitionsExecover the first and duplicates rows. The fix makes the rule reuse the grouping an earlier pass inserted instead of re-deriving the alignment from an already-aligned layout:innermostGroupPartitiondescends only through aGroupPartitionsExecand the localSortExecthis rule itself added; both the rewrite and the reads below go through it.rewriteGroupPartitionsrewrites the innermost node and drops redundant groupings stacked above it, reproducing the plan a single pass would have produced.unwrapGroupPartitionspeels to the pre-alignment plan so the statistics and original partition keys are read from that plan on every pass.applyGroupPartitionskeeps thejoinKeyPositionsa reused node already holds (they were computed against the raw partition keys) instead of re-projecting a second time.withJoinKeyPositions, which every multi-child operator reaches and not just joins, reuses only a topmostGroupPartitionsExec.ShuffleExchangeExecsite strips every grouping this rule inserted before re-shuffling.On
branch-4.2this additionally introduces a smallPartitioningCollectioncompanion object (withrepresentativeOfandnumKeyedPartitions) inpartitioning.scala, mirroring master;branch-4.2'sPartitioningCollectionhas no companion object yet.Why are the changes needed?
The join returns duplicated rows when AQE re-runs
EnsureRequirements. The re-run is not an AQE quirk:AdaptiveSparkPlanExecbuilds one rule instance, andConvertSortMergeJoinToShuffledHashJoinandOptimizeSkewedJoinhand the whole tree back to it after rewriting some other join, all within onequeryStagePreparationRulespass. A join child then arrives asSortExec(GroupPartitionsExec(...)); the partially clusteredKeyedPartitioningreportsisGrouped = falseby design, so the distribution step adds a plainGroupPartitionsExecon top, andapplyGroupPartitionsrewrites that fresh outer node instead of the inherited one. A replicating grouping repeats every row, so the row set is emitted twice. Reproduction:returns
(1, 1, 1, 1, 2, 7)instead of(1, 1, 2, 7).Does this PR introduce any user-facing change?
Yes. It fixes wrong results (duplicated rows) for a storage-partitioned join under a partially clustered distribution when AQE re-runs
EnsureRequirements. As on master, the partition-count fallback that picks the replicate side when there are no plan statistics now compares the pre-alignment split counts instead of the aligned report's distinct keys, which can change the replicated side on a first pass.How was this patch tested?
KeyGroupedPartitioningSuite: new tests for the row count, the replicate-side choice, and the subset-key join key positions under anEnsureRequirementsre-run. The subset-key test setsspark.sql.requireAllClusterKeysForCoPartition=false, because onbranch-4.2a join on a subset of the partition keys only engages storage-partitioned join with that config off.EnsureRequirementsSuite: new unit tests for the local-sort reuse bound, topmost-onlywithJoinKeyPositionsreuse, tag preservation, the shuffle site, the fallback counting, and the intentional single-child wrap.branch-4.2.