remove repartition exec from coalesce batches optimizer #19239
Merged
Conversation
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
github-actions
bot
added
optimizer
40 tasks
Contributor
|
run benchmarks |
Contributor
|
run benchmark tpch_mem10 |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes RepartitionExec from the CoalesceBatches optimizer by updating test expectations to reflect the optimizer change. The CoalesceBatchesExec node is no longer inserted after RepartitionExec operations in the physical execution plans.
- Removes
CoalesceBatchesExecoptimizer behavior after repartitioning operations - Updates all affected SQL logic test files to reflect the new physical plan structure
- Includes minor whitespace corrections in test files
Reviewed changes
Copilot reviewed 53 out of 55 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| window_limits.slt | Removed CoalesceBatchesExec from window aggregate plans |
| window.slt | Removed CoalesceBatchesExec from multiple window and aggregate operations |
| unnest.slt | Removed CoalesceBatchesExec from unnest aggregate plans |
| union.slt | Removed CoalesceBatchesExec from union and aggregate plans |
| tpch/plans/*.slt.part | Removed CoalesceBatchesExec from all TPC-H query plans (Q1-Q22) |
| timestamps.slt | Removed extra spaces in error message assertions |
| spark/math/hex.slt | Fixed line number formatting |
| subquery*.slt | Removed CoalesceBatchesExec from subquery aggregate plans |
| select.slt | Removed CoalesceBatchesExec from aggregate with limit plans |
| repartition.slt | Removed CoalesceBatchesExec from repartition aggregate plans |
| qualify.slt | Removed CoalesceBatchesExec from qualify window plans |
| push_down_filter.slt | Removed CoalesceBatchesExec from grouping sets plans |
| preserve_file_partitioning.slt | Removed CoalesceBatchesExec from partitioned file plans |
| predicates.slt | Removed CoalesceBatchesExec from join filter pushdown plans |
| order.slt | Removed CoalesceBatchesExec from ordered aggregate plans |
| limit.slt | Removed CoalesceBatchesExec from limited aggregate plans |
| joins.slt & join.slt.part | Removed CoalesceBatchesExec from various join plans |
| insert*.slt | Removed CoalesceBatchesExec from insert with window plans |
| explain_tree.slt | Removed CoalesceBatchesExec from tree visualization |
| distinct_on.slt | Removed CoalesceBatchesExec from distinct aggregate plans |
| cte.slt | Removed CoalesceBatchesExec from recursive CTE plans |
| count_star_rule.slt | Removed CoalesceBatchesExec from count aggregate plans |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alamb
approved these changes
Dec 14, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This is great -- thank you so much @jizezhang 🙏 I am sorry it took so long to review.
I'll plan to merge this tomorrow. Whoever merges this should make sure to rerun the tests as I think there is a non trivial chance of conflicts
| target_batch_size, | ||
| )))) | ||
| } else if let Some(async_exec) = plan_any.downcast_ref::<AsyncFuncExec>() { | ||
| if let Some(async_exec) = plan_any.downcast_ref::<AsyncFuncExec>() { |
Contributor
There was a problem hiding this comment.
looks like the only thing left is AsyncFuncExec and we could entirely remove the CoalesceBatches pass (and maybe even structure)
| 54)----------------------FilterExec: r_name@1 = AMERICA, projection=[r_regionkey@0] | ||
| 55)------------------------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1 | ||
| 56)--------------------------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/tpch/data/region.tbl]]}, projection=[r_regionkey, r_name], file_type=csv, has_header=false | ||
| 05)--------RepartitionExec: partitioning=Hash([o_year@0], 4), input_partitions=4 |
Contributor
There was a problem hiding this comment.
these complex query plans are so much easier to read now
Contributor
|
run benchmark clickbench_partitioned |
|
🤖 |
Dandandan
approved these changes
Dec 15, 2025
|
🤖: Benchmark completed Details
|
Contributor
|
I merged up to resolve some conflicts |
Contributor
|
run benchmarks |
Contributor
|
(I don't think this PR makes any meaningful difference on performance, as expected) |
Contributor
|
Thank you @jizezhang @Dandandan |
|
🤖 |
|
🤖: Benchmark completed Details
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Which issue does this PR close?
BatchCoalescerintoRepartitionExec#18782.Rationale for this change
What changes are included in this PR?
Removes
RepartitionExecfromCoalesceBatchesoptimizer.Are these changes tested?
Yes
Are there any user-facing changes?
No