Remove AggregateExpr trait
#12096
Remove AggregateExpr trait
#12096
Conversation
github-actions
bot
added
physical-expr
|
Marking as draft as CI is currnetly not passing so I don't think this is ready for review |
| @@ -229,7 +230,7 @@ mod tests { | |||
| fn partial_aggregate_exec( | |||
| input: Arc<dyn ExecutionPlan>, | |||
| group_by: PhysicalGroupBy, | |||
| aggr_expr: Vec<Arc<dyn AggregateExpr>>, | |||
| aggr_expr: Vec<Arc<AggregateFunctionExpr>>, | |||
There was a problem hiding this comment.
If AggregateFunctionExpr is not cloned massively, we could remove Arc, similar to that we doesn't wrap Arc for Expr.
There was a problem hiding this comment.
I could try this in a followup pr.
There was a problem hiding this comment.
Thanks @lewiszlw for the clean-up. To avoid breaking any downstream, let's keep this PR focused solely on the removal of the AggregateExpr trait by moving all its methods to AggregateFunctionExpr. Then in a new PR, we can focus on determining what we really need and what can be removed in DF.
|
Thanks for working on this @lewiszlw . As @berkaysynnada mentioned, let's keep all the methods please. BTW, I don't see a way to remove with_new_expressions without breaking downstream. We also plan to contribute some code upstream in the near future that uses this functionality, so it is probably here for good. However, it may be possible to remove all_expressions (the functionality may be implemented with existing methods - though I should verify that). We can explore if that is possible and give you an indication next week so we can plan the follow on work. Thanks again for the awesome collaboration 🚀 |
|
Now I think there is no additional change breaking downstream. |
There was a problem hiding this comment.
Thanks @lewiszlw @jayzhan211 and @ozankabak
I skimmed this PR and it looks like an improvement to me. Given there are already a good number of eyes on this PR, I'll defer review to other community members. Let me know if I can help
|
Let's have @berkaysynnada take one final look tomorrow and then merge 🚀 |
|
I did a final review and I think everything looks good. A nice simplification step, thank you @lewiszlw. |
|
🚀 |
Which issue does this PR close?
Closes #11810.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?