minor: implement with_new_expressions for AggregateFunctionExpr #16897
Conversation
github-actions
bot
added
logical-expr
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
The code looks good to me @berkaysynnada -- thank you for the contribution.
My only concern is that since there are no tests or paths that use this new code it may be broken as part of some future refactor without knowing. Is there some way to test it (or take advantage of the new APIs?)
| order_by_exprs: Vec<Arc<dyn PhysicalExpr>>, | ||
| ) -> Option<AggregateFunctionExpr> { | ||
| None | ||
| debug_assert_eq!(args.len(), self.args.len()); |
There was a problem hiding this comment.
Perhaps we can return None here if the lengths don't match up so in production builds we don't get wrong results 🤔 Likewise below with the check on order sensitivity
| fun: self.fun.clone(), | ||
| args, | ||
| return_field: Arc::clone(&self.return_field), | ||
| // TODO: This name and display fields should be updated after re-write to not mislead |
There was a problem hiding this comment.
I am not 100% sure about this TODO - perhaps the human_display should be changed, but maybe not the name
| /// Get the reverse expression of this [WindowExpr]. | ||
| fn get_reverse_expr(&self) -> Option<Arc<dyn WindowExpr>>; | ||
|
|
||
| /// Creates a new instance of the window function evaluator. |
There was a problem hiding this comment.
this is a new API too, right?
There was a problem hiding this comment.
Yes, this util becomes useful when updating WindowAggState's from deserialized window_fn states. If it seems too specific, I can remove it
…he#16897) * minor * Update aggregate.rs
3 participants
Which issue does this PR close?
Rationale for this change
AggregateFunctionExpr::with_new_expressions()
has been implemented. This is useful when updating column indices within the expressions of anAggregateFunctionExpr`.Additionally,
create_window_fn()method has been added to theWindowExprtrait. This provides a convenient way to directly access the underlying window function evaluator.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?