alamb commented on code in PR #10835:
URL: https://github.com/apache/datafusion/pull/10835#discussion_r1632235419
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -184,90 +189,138 @@ impl CommonSubexprEliminate {
Ok((rewrite_exprs, new_input))
}
- fn try_optimize_window(
+ fn try_optimize_proj(
&self,
- window: &Window,
+ projection: Projection,
config: &dyn OptimizerConfig,
- ) -> Result<LogicalPlan> {
- let mut window_exprs = vec![];
- let mut arrays_per_window = vec![];
- let mut expr_stats = ExprStats::new();
-
- // Get all window expressions inside the consecutive window operators.
- // Consecutive window expressions may refer to same complex expression.
- // If same complex expression is referred more than once by subsequent
`WindowAggr`s,
- // we can cache complex expression by evaluating it with a projection
before the
- // first WindowAggr.
- // This enables us to cache complex expression "c3+c4" for following
plan:
- // WindowAggr: windowExpr=[[sum(c9) ORDER BY [c3 + c4] RANGE BETWEEN
UNBOUNDED PRECEDING AND CURRENT ROW]]
- // --WindowAggr: windowExpr=[[sum(c9) ORDER BY [c3 + c4] RANGE BETWEEN
UNBOUNDED PRECEDING AND CURRENT ROW]]
- // where, it is referred once by each `WindowAggr` (total of 2) in the
plan.
- let mut plan = LogicalPlan::Window(window.clone());
- while let LogicalPlan::Window(window) = plan {
- let Window {
- input, window_expr, ..
- } = window;
- plan = input.as_ref().clone();
Review Comment:
clone removed
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -127,21 +133,21 @@ impl CommonSubexprEliminate {
/// Returns the rewritten expressions
fn rewrite_exprs_list(
&self,
- exprs_list: &[&[Expr]],
+ exprs_list: Vec<Vec<Expr>>,
arrays_list: &[&[Vec<(usize, String)>]],
expr_stats: &ExprStats,
common_exprs: &mut CommonExprs,
) -> Result<Vec<Vec<Expr>>> {
exprs_list
- .iter()
+ .into_iter()
.zip(arrays_list.iter())
.map(|(exprs, arrays)| {
exprs
- .iter()
- .cloned()
Review Comment:
another clone
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -319,98 +379,201 @@ impl CommonSubexprEliminate {
if common_exprs.is_empty() {
// Alias aggregation expressions if they have changed
let new_aggr_expr = new_aggr_expr
- .iter()
- .zip(aggr_expr.iter())
- .map(|(new_expr, old_expr)| {
- new_expr.clone().alias_if_changed(old_expr.display_name()?)
Review Comment:
another
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -870,37 +870,7 @@ impl LogicalPlan {
LogicalPlan::Filter { .. } => {
assert_eq!(1, expr.len());
let predicate = expr.pop().unwrap();
-
- // filter predicates should not contain aliased expressions so
we remove any aliases
Review Comment:
This code is extract into `Filter::remove_aliases` as it is needed for CSE.
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -173,9 +179,8 @@ impl CommonSubexprEliminate {
&mut common_exprs,
)?;
- let mut new_input = self
- .try_optimize(input, config)?
- .unwrap_or_else(|| input.clone());
Review Comment:
The whole point of this PR is to remove clones -- I'll try and point out
example places such as this where the plans and/or expressions were being cloned
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -184,90 +189,138 @@ impl CommonSubexprEliminate {
Ok((rewrite_exprs, new_input))
}
- fn try_optimize_window(
+ fn try_optimize_proj(
&self,
- window: &Window,
+ projection: Projection,
config: &dyn OptimizerConfig,
- ) -> Result<LogicalPlan> {
- let mut window_exprs = vec![];
- let mut arrays_per_window = vec![];
- let mut expr_stats = ExprStats::new();
-
- // Get all window expressions inside the consecutive window operators.
- // Consecutive window expressions may refer to same complex expression.
- // If same complex expression is referred more than once by subsequent
`WindowAggr`s,
- // we can cache complex expression by evaluating it with a projection
before the
- // first WindowAggr.
- // This enables us to cache complex expression "c3+c4" for following
plan:
- // WindowAggr: windowExpr=[[sum(c9) ORDER BY [c3 + c4] RANGE BETWEEN
UNBOUNDED PRECEDING AND CURRENT ROW]]
- // --WindowAggr: windowExpr=[[sum(c9) ORDER BY [c3 + c4] RANGE BETWEEN
UNBOUNDED PRECEDING AND CURRENT ROW]]
- // where, it is referred once by each `WindowAggr` (total of 2) in the
plan.
- let mut plan = LogicalPlan::Window(window.clone());
Review Comment:
clone 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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
