Avoid Expr copies OptimizeProjection, 12% faster planning, encapsulate indicies
#10216
Conversation
alamb
force-pushed
the
alamb/optimize_proj_pt1
branch
3 times, most recently
from
f120d80
to
9bd4636
Compare
| /// See [Self::index_of_column] for a version that returns an error if the | ||
| /// column is not found | ||
| pub fn maybe_index_of_column(&self, col: &Column) -> Option<usize> { | ||
| self.index_of_column_by_name(col.relation.as_ref(), &col.name) |
There was a problem hiding this comment.
This hung me up for a while -- the original code uses flat_map which discards the Err if the column is not found, was not obvious to me
There was a problem hiding this comment.
This is much more clear, Thanks!
alamb
changed the title
OptimizeProjectionExpr copies, encapsulate the handling of child indicies in OptimizeProjection
alamb
force-pushed
the
alamb/optimize_proj_pt1
branch
from
2434484
to
4d4c2bd
Compare
alamb
changed the title
Expr copies, encapsulate the handling of child indicies in OptimizeProjectionExpr copies OptimizeProjection, 12% faster planning, encapsulate indicies
| ) -> Result<Option<LogicalPlan>> { | ||
| // `child_required_indices` stores |
There was a problem hiding this comment.
this documentation moved to RequiredIndices
| @@ -339,31 +318,35 @@ fn optimize_projections( | |||
| let left_len = join.left.schema().fields().len(); | |||
| let (left_req_indices, right_req_indices) = | |||
| split_join_requirements(left_len, indices, &join.join_type); | |||
| let exprs = plan.expressions(); | |||
There was a problem hiding this comment.
This call copies all expressions in the plan
| @@ -123,12 +120,13 @@ fn optimize_projections( | |||
| // that appear in this plan's expressions to its child. All these | |||
| // operators benefit from "small" inputs, so the projection_beneficial | |||
| // flag is `true`. | |||
| let exprs = plan.expressions(); | |||
There was a problem hiding this comment.
Likewise, this was also copying all expressions
| @@ -137,13 +135,9 @@ fn optimize_projections( | |||
| // that appear in this plan's expressions to its child. These operators | |||
| // do not benefit from "small" inputs, so the projection_beneficial | |||
| // flag is `false`. | |||
| let exprs = plan.expressions(); | |||
There was a problem hiding this comment.
And another copy removed
| @@ -178,16 +170,12 @@ fn optimize_projections( | |||
| Make sure `.necessary_children_exprs` implementation of the `UserDefinedLogicalNode` is \ | |||
| consistent with actual children length for the node."); | |||
| } | |||
| // Expressions used by node. | |||
| let exprs = plan.expressions(); | |||
| @@ -408,26 +392,6 @@ fn optimize_projections( | |||
| } | |||
| } | |||
|
|
|||
| /// This function applies the given function `f` to the projection indices | |||
There was a problem hiding this comment.
All of those code was moved into RequiredIndices as methods both to encapsulate the logic a bit more as well as to allow creation without copying
There was a problem hiding this comment.
Thanks @alamb. This PR is LGTM!. As well as improving performance, it improves maintainability also.
|
Thanks for the reiew @mustafasrepo -- I am slowly working through how to refactor this pass to avoid copies. I think a few more PRs and I'll have it |
Which issue does this PR close?
Part of #10209
Rationale for this change
This is part 1 of improving the performance of
OptimizeProjection(by less copying) in #10209.While trying to rework how
OptimizeProjection works, I found that the populationof child indices pretty much required calling
LogicalPlan::expressions()which results in an owed copy of all the plansExprsRemoving these copies themselves improves the situation (and I think may make the code easier to reasona about), and sets us up to stop copying the
LogicalPlannodes as a follow on PRWhat changes are included in this PR?
This PR encapsulates that logic for managing required indices into a new struct
RequiredIndicieswhich avoids some Expr copies (and sets up the rewrite to avoidLogicalPlancopies)Are these changes tested?
Covered by existing tests
Are there any user-facing changes?
no functional changes
performance benchmarks show an overall 14% improvements for tpch_all and tpcds_all, which is pretty neat
Details