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/10/15 14:43:26 UTC
[PR] [SPARK-60044][SQL][4.3] Freeze the k-way merge ordering of GroupPartitionsExec when the merge is planned [spark]
peter-toth opened a new pull request, #59508:
URL: https://github.com/apache/spark/pull/59508
### Differences from #59306
This is the branch-4.3 backport of #59306. The description below is the original one.
- The bug is on branch-4.3. Without the fix, the new config-change test numbers the rows of `id` 1 `[1, aa, 2]` and `[1, bb, 1]`, as in the example below. The new `debug()` test fails too, since the merge concatenates under a `DebugExec`.
- For that run the three main files were restored from branch-4.3. `GroupPartitionsExecSuite.scala` and `EnsureRequirementsSuite.scala` were restored too, and one line of `KeyGroupedPartitioningSuite.scala` went back to `enableSortedMerge = true`. Those changes need the new field.
- `GroupPartitionsExec` on 4.3 is the older node. It has `expectedPartitionKeys`, `distributePartitions` and a default for every field but `child`, and no factory. `sortedMergeOrdering` replaces `enableSortedMerge` there, with `None` as its default.
- `withKeyPositionsFor`, `checkChildStillMatches`, `sendDriverMetrics` and the factory are not on 4.3. The hunks in `withKeyPositionsFor` and the factory are dropped. So `checkPlannedMergeRuns` is the first check in `doExecute`, and its Scaladoc no longer points to `checkChildStillMatches`.
- The comment on the EXPLAIN string keeps the 4.3 reason for not reading `usesSortedMerge`. On 4.3 that forces `grouping`, which can throw.
- `EnsureRequirements.scala` has no change. On 4.3 `withJoinKeyPositions` builds the node with `copy`, which already keeps the ordering.
- The `withKeyPositionsFor` test in `GroupPartitionsExecSuite` is dropped, since the method is not on 4.3. The rest of `GroupPartitionsExecSuite.scala` conflicted only on imports and on a master-only test helper next to the new ones.
- `KeyGroupedPartitioningSuite.scala` only conflicted on imports. In `EnsureRequirementsSuite.scala` only the new test is taken. The conflict there was master-only tests next to it.
- The new tests already set the configs they need. `partitionKeyOrdering` is off by default on 4.3. `preserveOrderingOnCoalesce` is off by default on every branch. The window is a single-child operator, so it needs no `spark.sql.requireAllClusterKeysForCoPartition`.
- The new `GroupPartitionsExecSuite` and `EnsureRequirementsSuite` tests need the new field, so they cannot run on the unfixed branch. Ablations on the fixed branch:
- With `kWayMergeOrdering` reading the child's ordering, the new `GroupPartitionsExecSuite` test and the config-change test fail, the latter with the wrong row numbers.
- With `withNewChildInternal` dropping the ordering, the new `GroupPartitionsExecSuite` test fails.
- With `outputOrdering` reading the child's ordering, that test and the config-change test fail.
- Without `checkPlannedMergeRuns`, or with `supportsColumnar` reading `usesSortedMerge`, the adjusted SPARK-55715 test fails.
- Without `DebugExec` in the whitelist, the `debug()` test fails with the planned-merge error.
- With `withJoinKeyPositions` dropping the ordering, the new `EnsureRequirementsSuite` test fails.
- Ran `GroupPartitionsExecSuite` (24), `KeyGroupedPartitioningSuite` (190), `EnsureRequirementsSuite` (59), `CombineAdjacentAggregationSuite` (10), `DebuggingSuite` (7) and `DebuggingSuiteAE` (5). All 295 tests pass. `KeyGroupedPartitioningSuite` is the only `KeyGroupedPartitioning*` suite on 4.3.
- `dev/lint-scala` passes.
### What changes were proposed in this pull request?
`GroupPartitionsExec` now freezes the ordering its k-way merge merges by, when the merge is planned:
- `sortedMergeOrdering: Option[Seq[SortOrder]]` replaces the `enableSortedMerge: Boolean` field. `enableSortedMerge` stays, as a method.
- `tryEnableSortedMerge` captures the child's ordering. That is the ordering `EnsureRequirements` checks against the parent's requirement.
- Every rebuild over the same child, or a wrapper of it, passes it on: `copy`, `withNewChildInternal` and `EnsureRequirements.withJoinKeyPositions`. Only `tryEnableSortedMerge` captures it.
- `withKeyPositionsFor` turns a merging node away, since it rebuilds the node over another child. No merging node reaches it today.
- For a merging node, `kWayMergeOrdering` and the merge branch of `outputOrdering` read the frozen ordering instead of `child.outputOrdering`. The frozen ordering is never empty.
- A merging node whose child is no longer safe for the merge fails at execution instead of concatenating. The error names the node outside the `SafeForKWayMerge` whitelist. That needs a rule after `EnsureRequirements` to insert such a node. No built-in rule does.
- The columnar path never merges, so a planned merge never reports itself columnar, whatever its child turns into later.
- `DebugExec` joins the whitelist. Without AQE, the `debug()` helper of `org.apache.spark.sql.execution.debug` wraps every node in it, including the ones under a merging node. Its per-partition state lives in its evaluator, and its accumulators only add.
- `tryEnableSortedMerge` leaves a node that already merges as it is, so the merge is decided once.
- EXPLAIN shows the frozen ordering instead of `SortedMerge: true`, e.g. `SortedMerge: [id#1 ASC NULLS FIRST, name#2 ASC NULLS FIRST]`.
### Why are the changes needed?
SPARK-59279 froze the merge decision, but the node still read the ordering it merges by from `child.outputOrdering`. When the source reports no ordering, the scan derives one from the partition keys under `spark.sql.sources.v2.bucketing.partitionKeyOrdering.enabled`, which it reads live. So turning that conf off after planning emptied the ordering of a planned merge. What the node did then depended on whether it had already read the ordering:
- If it had, it still merged, by an empty ordering, so it treated every row as equal. The new test takes this path without AQE, since it checks the merge's ordering before turning the conf off.
- Otherwise it found the merge infeasible and concatenated the splits. With AQE that always happens, since the result stage's node is made after the change.
Either way, a parent that relied on the merge read its rows out of order.
For example, take a table partitioned by `(identity(id), days(arrive_time), identity(name))`, with `allowKeysSubsetOfPartitionKeys` and `preserveOrderingOnCoalesce` on. The scan derives the ordering `[id, name]`, while Spark sorts the splits by the full key. `ROW_NUMBER() OVER (PARTITION BY id ORDER BY name)` plans a k-way merge and no sort. With the conf turned off before the query runs, the rows of `id` 1 were numbered in split order: `[1, aa, 2]` and `[1, bb, 1]` instead of `[1, aa, 1]` and `[1, bb, 2]`.
### Does this PR introduce _any_ user-facing change?
Yes. A query planned with the k-way merge keeps the merge's ordering when `spark.sql.sources.v2.bucketing.partitionKeyOrdering.enabled` changes after planning. It used to give wrong results then, such as wrong `ROW_NUMBER` values. The bug exists since 4.2.0, which has both the k-way merge (SPARK-55715) and the key-derived ordering (SPARK-56241). EXPLAIN of a planned k-way merge now shows its ordering instead of `SortedMerge: true`.
### How was this patch tested?
- New test in `KeyGroupedPartitioningSuite`, "SPARK-60044: a planned k-way merge keeps its ordering after a later config change". It plans the example above, turns the conf off, then checks the rows and that the merge still reports `[id, name]`, in both AQE modes. It fails without the fix with the wrong row numbers above.
- New test in `EnsureRequirementsSuite`: pushing join key positions into a merging node keeps its frozen ordering.
- New tests in `GroupPartitionsExecSuite`:
- `withKeyPositionsFor` turns a merging node away. It fails without the check.
- `tryEnableSortedMerge` captures the child's ordering. `copy`, `withNewChildren` and `outputOrdering` keep it after the child's ordering becomes empty. It also checks the EXPLAIN string, that the merge is decided once, that deciding again after the change finds no ordering, and that an empty ordering is refused. It fails if `withNewChildInternal` drops the ordering or `outputOrdering` reads the child's.
- A merge over a child that is not safe for it fails at execution on both execute paths, checked in an existing `GroupPartitionsExecSuite` test.
- New test in `KeyGroupedPartitioningSuite`: the `debug()` helper runs a planned k-way merge without AQE. It checks that the merge runs under the `DebugExec` wrapping, and fails if `DebugExec` is not whitelisted.
- The tests that build `GroupPartitionsExec` with the merge on now pass the ordering.
- Ran `GroupPartitionsExecSuite` (35), the `KeyGroupedPartitioning*` suites (234), `EnsureRequirementsSuite` (80), `CombineAdjacentAggregationSuite` (10) and `DebuggingSuite`, plus `dev/lint-scala`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.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-60044][SQL][4.3] Freeze the k-way merge ordering of GroupPartitionsExec when the merge is planned [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59508:
URL: https://github.com/apache/spark/pull/59508#issuecomment-6214893208
**Merge Summary:**
- merged into branch-4.3 https://github.com/apache/spark/commit/268afa2477f9dc537aef5541e4add7c3e94487f3
*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-60044][SQL][4.3] Freeze the k-way merge ordering of GroupPartitionsExec when the merge is planned [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #59508: [SPARK-60044][SQL][4.3] Freeze the k-way merge ordering of GroupPartitionsExec when the merge is planned
URL: https://github.com/apache/spark/pull/59508
--
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-60044][SQL][4.3] Freeze the k-way merge ordering of GroupPartitionsExec when the merge is planned [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #59508:
URL: https://github.com/apache/spark/pull/59508#issuecomment-6215178149
Thank you @dongjoon-hyun!
--
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