jayzhan211 commented on issue #10074: URL: https://github.com/apache/arrow-datafusion/issues/10074#issuecomment-2061149382
I would like to continue the discussion in #9972 since I don't have a good reason not to **support directly mutating physical-expr**. And the solution (moving conversion to optimizer rule) is specific to FIRST/LAST for me, I'm not sure moving all the function including `reverse_expr` to optimizer rule is a good idea. > I still think it would be nice to have a design that can deal with `physical-expr` for UDAF 😞 Should we move AggregateUDFImpl to a crate that imports both datafusion-expr and datafusion-physical-expr-common I think we can consider moving `AggregateUDFImpl` to `functions-aggregate-common` that imports both `datafusion-expr `and `datafusion-physical-expr-common`. The pros are that we can avoid doing conversion of `ordering` twice in `accumulator` where it is already computed while creating udaf expr. https://github.com/apache/arrow-datafusion/blob/4ad4f90d86c57226a4e0fb1f79dfaaf0d404c273/datafusion/core/src/physical_planner.rs#L1926-L1933 Not only `accumulator` but also `reverse_expr` has the similar issue. https://github.com/apache/arrow-datafusion/blob/4ad4f90d86c57226a4e0fb1f79dfaaf0d404c273/datafusion/physical-expr/src/aggregate/array_agg_ordered.rs#L138-L150 If we have only `sort_exprs: Vec<Expr>`, we need to compute `PhysicalSortExpr` again. I think the benefit of avoiding re-computing is large than the complexity of introducing yet another crate `functions-aggregate-common`. And, note that we have aggregate function in window function too. We may have `functions-window-common` and `functions-window` crates if follow the design. Unless the cost of re-computing logical-expr to physical-expr is negligible otherwise I think able to directly mutating physical-expr is good for long term. -- 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...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
