Optimize Projections during Logical Plan #8340
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
logical-expr
Dandandan
approved these changes
Nov 28, 2023
Dandandan
reviewed
Nov 28, 2023
Dandandan
reviewed
Nov 28, 2023
| ----------Projection: aggregate_test_100.c1 | ||
| ------------Filter: aggregate_test_100.c13 != Utf8("C2GT5KVyOPZpgKVl110TyZO0NcJ434") | ||
| --------------TableScan: aggregate_test_100 projection=[c1, c13], partial_filters=[aggregate_test_100.c13 != Utf8("C2GT5KVyOPZpgKVl110TyZO0NcJ434")] | ||
| ------Projection: |
Contributor
There was a problem hiding this comment.
Plans seem to be all slightly better :)
jonahgao
reviewed
Nov 28, 2023
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
Member
There was a problem hiding this comment.
Could we move these tests into optimize_projection.rs 🤔, so that we can delete the merge_projection.rs file?
Contributor
Author
There was a problem hiding this comment.
I plan to do so in next PR. I didn't want to increase diff because of test movement as this is PR already big.
jonahgao
reviewed
Nov 28, 2023
| "Aggregate: groupBy=[[]], aggr=[[COUNT(Int64(1))]]\ | ||
| \n Filter: test.col_date64 >= Date64(\"890179200000\") AND test.col_date64 <= Date64(\"897955200000\")\ | ||
| \n TableScan: test projection=[col_date64]"; | ||
| \n Projection: \ |
Member
There was a problem hiding this comment.
It seems there is an additional empty projection here compared to before.
Contributor
There was a problem hiding this comment.
Yes, but this (empty) projection is fine, as the aggregate doesn't need any columns.
This was referenced Dec 6, 2023
kavirajk
added a commit
to kavirajk/datafusion
that referenced
this pull request
Apr 13, 2024
…ze_projections.rs` Fixes: apache#9978 The PR apache#8340 removed `push_down_projections.rs` in favour of `optimize_projections.rs`. This PR moves the tests as well. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
kavirajk
added a commit
to kavirajk/datafusion
that referenced
this pull request
Apr 14, 2024
…ze_projections.rs` Fixes: apache#9978 The PR apache#8340 removed `push_down_projections.rs` in favour of `optimize_projections.rs`. This PR moves the tests as well. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
alamb
pushed a commit
that referenced
this pull request
Apr 15, 2024
…ze_projections.rs` (#10071) * cleanup(tests): Move tests from `push_down_projections.rs` to `optimize_projections.rs` Fixes: #9978 The PR #8340 removed `push_down_projections.rs` in favour of `optimize_projections.rs`. This PR moves the tests as well. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * remove the file `push_down_projection.rs` Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * Fix method signatures that are broken by other PRs Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> * remove `push_down_projections.rs` from `lib.rs` Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> --------- Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
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?
Closes 8296.
Rationale for this change
What changes are included in this PR?
This PR removes unnecessary
Columns(Fields)from theLogicalPlan, when it is beneficial, by adding a projection that removes unnecessary columns from the plan.This feature is accomplished by
optimize_projectionsLogicalPlanrule.New
OptimizeProjectionsrule covers the functionality ofMergeProjections,PushDownProjectionandEliminateProjection. Hence as part of this PR, these rules are removed also.With this PR
LogicalPlans that contain only absolutely necessary fields by subsequent operators (when doing so is desirable, or beneficial e.g most of the case)Consider
will not be merged to
The reason is that, in the first plan, computation
t.a/2is calculated once, and used by subsequent projection twice.However, in the second plan computation
t.a/2calculated twice. (This feature solves the problem in the issue.)Are these changes tested?
Yes
Are there any user-facing changes?