alamb commented on code in PR #10216:
URL: https://github.com/apache/datafusion/pull/10216#discussion_r1578538332
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -105,13 +106,9 @@ impl OptimizerRule for OptimizeProjections {
fn optimize_projections(
plan: &LogicalPlan,
config: &dyn OptimizerConfig,
- indices: &[usize],
+ indices: RequiredIndicies,
) -> Result<Option<LogicalPlan>> {
- // `child_required_indices` stores
Review Comment:
this documentation moved to `RequiredIndices`
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -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();
Review Comment:
This call copies all expressions in the plan
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -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();
Review Comment:
Likewise, this was also copying all expressions
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -408,26 +392,6 @@ fn optimize_projections(
}
}
-/// This function applies the given function `f` to the projection indices
Review 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
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -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();
Review Comment:
Another copy
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -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();
Review Comment:
And another copy removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
