Remove coalesce batches rule and deprecate CoalesceBatchesExec #19622
+271
−778
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
CoalesceBatchesExec
github-actions
bot
added
optimizer
Contributor
|
Looks great, can you fix the clippy errors @feniljain ? |
Contributor
|
run benchmarks |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
QQuery 23 in clickbench_partitioned just halved the time 😲 |
Contributor
I think this is not correct, it is super flaky lately. @alamb do you know the source of this? Is it running OOM / swapping perhaps or slow / not predictable block storage? |
Contributor
Author
Ouuh
Did you mean to tag Andrew instead of bot? |
Contributor
Yeah @alamb :D |
Contributor
|
run benchmarks |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
QQuery 23 still seems to be leading ahead! |
Contributor
I think it is the same flakiness |
Dandandan
reviewed
Jan 3, 2026
datafusion/core/tests/physical_optimizer/filter_pushdown/mod.rs
Outdated
Show resolved
Hide resolved
Contributor
I suspect this has to do with timing. Basically Q23 is like This can now takes advantage of dynamic filter pushdown which can prune some of the files. However, the updates of the dynamic filters is not deterministic so I think that accounts for the variance run to run -- depending on what rows have been seen already will potentially skip entire files I suspect this accounts for the non determinsim I will see if I can reproduce the results locally |
Contributor
|
run benchmark clickbench_partitioned |
|
🤖 |
alamb
approved these changes
Jan 5, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @feniljain -- this looks great to me
| Boundedness::Bounded => memory_exec_sorted(&schema, ordering.clone()), | ||
| }; | ||
| let repartition_rr = repartition_exec_round_robin(source); | ||
| let repartition_hash = repartition_exec_hash(repartition_rr); |
Contributor
There was a problem hiding this comment.
not for this PR, but I think now actually we have fixed the double repartition as well, as described in https://datafusion.apache.org/blog/output/2025/12/15/avoid-consecutive-repartitions/
@gene-bordegaray is that correct? if so I will file a ticket to try and update these tests so they match what datafusion creates now (a single round robin hash)
|
🤖: Benchmark completed Details
|
Contributor
|
Epic work @feniljain -- thanks again |
11 tasks
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?
CoalesceBatchesoptimizer rule #19591Rationale for this change
Explained in issue itself
What changes are included in this PR?
CoalesceBatchesExecAre these changes tested?
Yes
Are there any user-facing changes?
Yes, added a deprecation tag on
CoalesceBatchesExec