jayzhan211 commented on PR #10354:
URL: https://github.com/apache/datafusion/pull/10354#issuecomment-2100125239
> ## Option 1 - Original expression as a parameter
> ```rust
> fn simplify(
> &self,
> expr: Expr,
> ) -> Result<datafusion_common::tree_node::Transformed<Expr>> {
> // we know it'll always be AggregateFunction but we have to match
on it
> // which is not terribly hard but it makes api a bit odd
> if let Expr::AggregateFunction(agg) = expr {
> ...
> Ok(datafusion_common::tree_node::Transformed::yes(...))
> } else {
> // no re-creation of expression if not used
> Ok(datafusion_common::tree_node::Transformed::no(expr))
> }
> }
> ```
>
> default implementation is just:
>
> ```rust
> fn simplify(
> &self,
> expr: Expr,
> ) -> Result<datafusion_common::tree_node::Transformed<Expr>> {
> // no re-creation of expression if not used
> Ok(datafusion_common::tree_node::Transformed::no(expr))
> }
> ```
>
> ## Option 2 - AggregationFunction as a parameter
> ```rust
> fn simplify(
> &self,
> agg: AggregateFunction,
> ) -> Result<datafusion_common::tree_node::Transformed<Expr>> {
> // we know its aggregate function, no need to do extra matching,
but we need to re-create expression
> // issue user can return `no` with new expression in it
>
Ok(datafusion_common::tree_node::Transformed::no(Expr::AggregateFunction(agg)))
> }
> ```
>
> ## Option 3 - AggregationFunction as a parameter with parametrised
`ExprSimplifyResult`
> ```rust
> fn simplify(
> &self,
> agg: AggregateFunction,
> ) -> Result<ExprSimplifyResult<AggregateFunction>> {
> // user cant return `no` with new expression
> // simplifier will re-crete expression in case original has been
returned
> Ok(ExprSimplifyResult::Original(agg))
> }
> ```
>
> IMHO, I prefer option number 3, which would involve parametrizing
`ExprSimplifyResult`, second best is option number 2. wdyt @jayzhan211, @alamb
Given these 3, I prefer 2. And we can do even further,
If user does not implement simplify, it is equivalent to Transformed::no or
ExprSimplifyResult::Original, so we can just have `Result<Expr>`
```rust
fn simplify(
&self,
agg: AggregateFunction,
) -> Result<Expr> {
// we know its aggregate function, no need to do extra matching, but
we need to re-create expression
// issue user can return `no` with new expression in it
Ok(Expr::AggregateFunction(agg))
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
