jayzhan211 commented on issue #9972:
URL:
https://github.com/apache/arrow-datafusion/issues/9972#issuecomment-2041069224
@alamb
To support `reverse_expr` for FirstValue, we need to implement it in `impl
AggregateExpr for AggregateFunctionExpr` .
Since **I think this method should be customizable for the user**, so I
think we should implement like
```rust
// fun: AggregateUDF,
fn reverse_expr(&self) -> Option<Arc<dyn AggregateExpr>> {
self.fun.reverse_expr()
}
```
Therefore, we need to have `reverse_expr` for `AggregateUDF`, and the only
choice for us is `self.inner.reverse_expr`. That indicates that we need
`reverse_expr` for `AggregateUDFImpl`.
To be able to return `Option<Arc<dyn AggregateExpr>>` for
`AggregateUDFImpl`, we need to import `AggregateExpr` which stays in
`physical-expr-common`.
I agree that `AggregateExpr` be kept in `physical-expr-common` now.
How about `AggregateUDFImpl`? It is weird for it to go
`physical-expr-common`. We need another expression layer that is able to import
`physical-expr-common`. If we move `AggregateUDFImpl` to `aggregate-functions`
then the user need to pull out all builtin functions which is not good too.
1. Find another way to do `reverse_expr`.
2. Introduce another crate :)
--
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]
