alamb commented on issue #11359: URL: https://github.com/apache/datafusion/issues/11359#issuecomment-2226297000
I agree with @jayzhan211 that the core of the problem is that the user defined API for aggregates is in datafusion_expr so can only use `Expr` but is invoked / instantiated as part of the physical plan. However, the same basic problem could be claimed for `ScalarUDFImpl` and `WindowUDFImpl` 🤔 It does feel like the way out of this is to make the spit between API and implementation more explicit. Maybe instead of `expr-common` maybe we could call it `expr-api` and `physical-common` --> `physical-api` So like * `expr-api`: Traits like `UDF`etc other than Expr and LogicalPlan can place it here * `expr`: (existing crate) Mainly for Expr and LogicalPlan. Import functions-aggregate-common for UDAF. * `physical-api`: Physical expr trait or other common things. Similar to what it is now except physical-expr like Column, Cast, Literal which should be moved to physical-expr * `physical-expr-common` Physical Expr are here + Column, Cast, Literal * `aggregate-api: Import physical-api and expr-api -- TBD what does this have? ScalarUDAF? Accumulator? * `functions-aggregate`: datafusion builtin functions -- 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
