Stop copying LogicalPlan and Exprs in ReplaceDistinctWithAggregate
#10460
Conversation
Signed-off-by: cailue <cailue@bupt.edu.cn>
Signed-off-by: 蔡略 <cailue@bupt.edu.cn>
| let aggr_expr = normalize_cols(aggr_expr, input.as_ref())?; | ||
| let group_expr = normalize_cols(on_expr, input.as_ref())?; |
| let aggr_expr = select_expr | ||
| .iter() | ||
| .map(|e| { | ||
| Expr::AggregateFunction(AggregateFunction::new( | ||
| AggregateFunctionFunc::FirstValue, | ||
| vec![e.clone()], | ||
| false, | ||
| None, | ||
| sort_expr.clone(), | ||
| None, | ||
| )) | ||
| }) | ||
| .collect::<Vec<Expr>>(); | ||
| let aggr_expr = vec![Expr::AggregateFunction(AggregateFunction::new( | ||
| AggregateFunctionFunc::FirstValue, | ||
| select_expr, | ||
| false, | ||
| None, | ||
| sort_expr.clone(), | ||
| None, | ||
| ))]; |
There was a problem hiding this comment.
I don't know if they are semantically identical.
Instructions welcomed.
There was a problem hiding this comment.
CI failed, i guess the answer is no. :(
There was a problem hiding this comment.
The difference is that the select_expr.iter()... makes a vec with the same number of elements as select_expr
To avoid a clone, perhaps you can use into_iter instead of iter, like
let aggr_expr = select_expr
.into_iter() // <---- Use into_iter() here
.map(|e| {
Expr::AggregateFunction(AggregateFunction::new(
AggregateFunctionFunc::FirstValue,
vec![e], // <--- to avoid a clone here
false,
None,
sort_expr.clone(),
None,
))
})
.collect::<Vec<Expr>>();There was a problem hiding this comment.
The difference is that the
select_expr.iter()...makes avecwith the same number of elements asselect_exprTo avoid a clone, perhaps you can use
into_iterinstead ofiter, likelet aggr_expr = select_expr .into_iter() // <---- Use into_iter() here .map(|e| { Expr::AggregateFunction(AggregateFunction::new( AggregateFunctionFunc::FirstValue, vec![e], // <--- to avoid a clone here false, None, sort_expr.clone(), None, )) }) .collect::<Vec<Expr>>();
What bothers me most is that I cannot avoid cloning sort_expr. However, it looks like the only way to make it correct.
Signed-off-by: 蔡略 <cailue@bupt.edu.cn>
alamb
changed the title
ReplaceDistinctWithAggregate
There was a problem hiding this comment.
I reviewed this PR and I think it looks very nice to me @ClSlaid . Thank you
Also thank you for the suggestion about LogicalPlanBuilder -- perhaps you would be willing to do a follow on (to implement #10485 and then switch this code to use it)?
https://github.com/apache/datafusion/pull/10460/files#r1598480059
…pache#10460) * patch: implement rewrite for RDWA Signed-off-by: cailue <cailue@bupt.edu.cn> * refactor: rewrite replace_distinct_aggregate Signed-off-by: 蔡略 <cailue@bupt.edu.cn> * patch: recorrect aggr_expr Signed-off-by: 蔡略 <cailue@bupt.edu.cn> * Update datafusion/optimizer/src/replace_distinct_aggregate.rs --------- Signed-off-by: cailue <cailue@bupt.edu.cn> Signed-off-by: 蔡略 <cailue@bupt.edu.cn> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Closes #10293.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?