milenkovicm commented on code in PR #10354:
URL: https://github.com/apache/datafusion/pull/10354#discussion_r1592018104
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1307,6 +1309,39 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for
Simplifier<'a, S> {
ExprSimplifyResult::Simplified(expr) => Transformed::yes(expr),
},
+ Expr::AggregateFunction(AggregateFunction {
Review Comment:
I agree with `AggregateFunctionsArgs`, consistency part got me on board 😀
I'm not sure about `Transformed` part, reason being is that
`ExprSimplifyResult` returns two types, `Expr` and currently `Vec<Expr>`
in that case `simplify` signature should be changed to:
```rust
trait AggreagateUDFImpl {
...
fn simplify(
&self,
agg_args: AggregateFunctionsArgs,
) -> Result<Transformed<Expr> {
let original : Expr = agg_args.into();
Ok(Transformed::no(original))
}
```
Thus simplify function should return 're-composed' original function or new
function ...
Anyway, I'll have a look what can be done, we can discuss
--
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
