alamb commented on issue #9972: URL: https://github.com/apache/arrow-datafusion/issues/9972#issuecomment-2041431362
> Not AggregateUDF because it does not have ordering info. Does it need the ordering info? Or could we keep the code that makes the new [`AggregateFunctionExpr`](https://github.com/alamb/arrow-datafusion/blob/dfd4442da8332116c7e1f9fcd9de7c6da856442b/datafusion/physical-plan/src/udaf.rs#L74-L87) in the physical optimizer? Doesn't reverse simply need to needs to know what function to use when if the sort order is "reversed"? ```rust impl AggregateUDFImpl { ... /// returns the "reverse" of this aggregate. Reverse means the function to use when the input ordering is /// reversed. For example, `first_value` can be reversed to `last_value` if the ordering is reversed. /// if the function is its own reverse, fn reverse(&self) -> Option<Arc<Aggregate>> ``` This seems to mirror how the current AggregateExpr works: https://github.com/alamb/arrow-datafusion/blob/a1ae15826245097e7c12d4f0ed3425b25af6c431/datafusion/physical-expr/src/aggregate/average.rs#L144-L146 🤔 that is an interesting idea, but I feel like it may be a pretty substantial API for a relatively narrow usecase (and thus make it harder to make DataFusion aggregates as it will be harder to figure out what APIs are needed) It seems like the core issue is that the current implementation of `reverse()` actually depends on What if we added an API like this: ```rust /// arguments for a aggregate (this is a subset of the fields of AggregateFunctionExpr which might be changed) struct PhysicalAggregateArgs { fun: AggregateUDF, args: Vec<Arc<dyn PhysicalExpr>>, /// Output / return type of this aggregate data_type: DataType, name: String, schema: Schema, // The logical order by expressions sort_exprs: Vec<Expr>, // The physical order by expressions ordering_req: LexOrdering, ignore_nulls: bool, ordering_fields: Vec<Field>, } impl AggregateUDFImpl { /// given the invocation of the aggregate, if it can be reversed, returns a new UDF fn reverse(&self, ordering: &[Expr]) -> ... ``` -- 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]
