Consolidate EliminateNestedUnion and EliminateOneUnion optimizer rules'
#18678
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
alamb
force-pushed
the
alamb/consolidate_passes
branch
from
2dda8cd to
23ec0b6
Compare
alamb
commented
Nov 13, 2025
| @@ -420,4 +423,28 @@ mod tests { | |||
| TableScan: table_1 | |||
| ") | |||
| } | |||
|
|
|||
| #[test] | |||
| fn eliminate_one_union() -> Result<()> { | |||
Contributor
Author
There was a problem hiding this comment.
single input unions can't be created easily, so this test looks a little different
|
|
||
| #[test] | ||
| fn eliminate_nothing() -> Result<()> { | ||
| let plan_builder = table_scan(Some("table"), &schema(), None)?; |
Contributor
Author
There was a problem hiding this comment.
there is already the same test in datafusion/optimizer/src/eliminate_nested_union.rs
| } | ||
|
|
||
| #[test] | ||
| fn eliminate_one_union() -> Result<()> { |
Contributor
Author
There was a problem hiding this comment.
I ported this test over
alamb
force-pushed
the
alamb/consolidate_passes
branch
from
23ec0b6 to
70b34d9
Compare
Omega359
reviewed
Nov 13, 2025
| @@ -55,6 +55,9 @@ impl OptimizerRule for EliminateNestedUnion { | |||
| _config: &dyn OptimizerConfig, | |||
| ) -> Result<Transformed<LogicalPlan>> { | |||
| match plan { | |||
| LogicalPlan::Union(Union { mut inputs, .. }) if inputs.len() == 1 => Ok( | |||
| Transformed::yes(Arc::unwrap_or_clone(inputs.pop().unwrap())), | |||
| ), | |||
Contributor
There was a problem hiding this comment.
Wow, that's it? Nice find, will be curious if this shows up in planning benchmark
Contributor
Author
|
🤖 |
Contributor
Author
|
🤖: Benchmark completed Details
|
Contributor
Author
|
🤖 |
Contributor
Author
|
🤖: Benchmark completed Details
|
Jefffrey
reviewed
Nov 14, 2025
| Arc::new(EliminateCrossJoin::new()), | ||
| Arc::new(EliminateLimit::new()), | ||
| Arc::new(PropagateEmptyRelation::new()), | ||
| // Must be after PropagateEmptyRelation |
Contributor
There was a problem hiding this comment.
It seems this comment was added with EliminateOneUnion so need to remove it, reference: 704e034#diff-42d66905c3fa6b245c3493fb4df4816e0fde3036941315ab42198fb16f3907df
(Any impact that the logic is now before PropagateEmptyRelation? Or running multiple passes of optimizer makes order redundant anyway?)
Contributor
Author
There was a problem hiding this comment.
Suggested change
| // Must be after PropagateEmptyRelation |
Contributor
Author
There was a problem hiding this comment.
Since all tests pass with OptimizeUnions in its current location I think we can just remove this comment as stale. I have done so in 4ed0786
| @@ -55,6 +55,9 @@ impl OptimizerRule for EliminateNestedUnion { | |||
| _config: &dyn OptimizerConfig, | |||
| ) -> Result<Transformed<LogicalPlan>> { | |||
| match plan { | |||
| LogicalPlan::Union(Union { mut inputs, .. }) if inputs.len() == 1 => Ok( | |||
Contributor
There was a problem hiding this comment.
Worth adding a minor comment here, considering its not exactly related to nested unions?
(Or could rename away from EliminateNestedUnions to something like SimplifyUnions to be all encompassing)
Contributor
Author
There was a problem hiding this comment.
(Or could rename away from EliminateNestedUnions to something like SimplifyUnions to be all encompassing)
I like this idea -- done in 9041a17
Contributor
|
I find it odd that the perf slightly decreased in some benchmarks. I'll try and reproduce it today locally. |
Contributor
Author
i think looking at the two runs (of the planning benchmark) I would say that there is no noticable performance difference (kind of as expected). I will rerun to see if we can see a repeating pattern emerge |
Contributor
Author
|
🤖 |
Contributor
Author
|
🤖: Benchmark completed Details
|
Contributor
Author
|
🤖 |
Contributor
Author
|
🤖: Benchmark completed Details
|
Contributor
|
I'm going to go with the benchmarks being within a range of noise. The first 10 or so do seem to be slightly higher on average though that could just be because they were the first to run. |
Jefffrey
approved these changes
Nov 15, 2025
Comment on lines
34
to
36
| // Backwards compatibility name | ||
| #[deprecated(since = "52.0.0", note = "Please use OptimizeUnions instead")] | ||
| pub type EliminateNestedUnion = OptimizeUnions; |
Contributor
There was a problem hiding this comment.
Do we need to consider the mod too? Since it'll now be optimize_unions::EliminateNestedUnion which is breaking anyway 🤔
Contributor
Author
There was a problem hiding this comment.
That is a good point -- I will update
Contributor
|
I got ambitious (it happens once every few moons 🤣) and ran the benchmark locally just to see if I saw the same uptick in the first few benchmarks as @alamb's results seem to have. Looks like my machine has about +/- 3-5% overall which is good to know. Details
|
Contributor
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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?
Rationale for this change
Each time a LogicalPlan is rewritten to eliminate a Union, we traverse the entire plan tree and copy some non trivial parts of it
Thus it is faster to plan when we have fewer passes over the plan tree
the EliminateNestedUnion and EliminateOneUnion rules both do similar things, and
the EliminateNestedUnion rule is very simple. So let's combine them into a
single rule that does both things in one pass over the plan tree.
What changes are included in this PR?
Consolidate
EliminateNestedUnionandEliminateOneUnionoptimizer rules into a single passAre these changes tested?
Yes with existing tests
I will also run planning benchmarks
Are there any user-facing changes?
No