-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Optimize planning for projected nested union #18713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🤖 |
|
I'll run the sql_planner_extended benchmark against this branch as that is the one that to me most reflects whether #17261 should be closed. I'm hopeful :) |
I have my vm in gc working on it too |
|
🤖: Benchmark completed Details
|
1.23.- 2.5x better certainly seems much better 🎉 ! Though I am not sure if we have fixed the exponential planning time |
|
Given the time estimate on my machine for the extended planner test I'm going to assume it's not impacting that benchmark as much as I'd like to see: |
|
I guess this won't be enough to close the issue but I will take any small wins for now. |
|
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @logan-keede -- sorry I haven't reviewed this earlier
Can you please add tests for this PR showing what behavior has changed? I didn't see anything different in the tests, so I worry the behavior might regress in the future and we wouldn't be able to tell
| _config: &dyn OptimizerConfig, | ||
| ) -> Result<Transformed<LogicalPlan>> { | ||
| match plan { | ||
| match plan.clone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you add this clone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover from something I was trying earlier, I thought clippy catches this stuff ...
… optimize_projected_union
84049a8 to
91b6c9a
Compare
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an improvement in my mind. Thank you @logan-keede and @Omega359
I don't think it closes the original issue, but it is a nice improvement
| .into_iter() | ||
| .map(Arc::unwrap_or_clone) | ||
| .collect::<Vec<_>>(), | ||
| LogicalPlan::Projection(Projection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right in understanding this change that it transforms
Projection
Union
to
Union
Projection
(aka it pushes the projection to each input of the union?)
Maybe some comments could help future readers
| } | ||
|
|
||
| #[test] | ||
| fn eliminate_nested_union_in_projection() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the change in this PR the test fails like
────────────┬───────────────────────────────────────────────────────────────────
1 1 │ Union
2 2 │ Projection: id AS table_id, key, value
3 │- TableScan: table
4 │- Projection: id AS table_id, key, value
5 │- TableScan: table
3 │+ Union
4 │+ TableScan: table
5 │+ TableScan: table
6 6 │ TableScan: table
────────────┴───────────────────────────────────────────────────────────────────
In other words, the plan is left as
Union
Projection: id AS table_id, key, value
Union
TableScan: table
TableScan: table
TableScan: table
|
Thanks again @logan-keede and @Omega359 |
Which issue does this PR close?
Rationale for this change
as explained in #17261 (comment) and discussion in the above mentioned issue
What changes are included in this PR?
Projected Nested Union are now merged with their parent union.
Are these changes tested?
Tested it by Github CI. Works for the scenario discussed in the above mentioned issue.
Are there any user-facing changes?
Yes? logical plans for some queries should be different(and better) now. No syntax changes.