alamb commented on code in PR #10648: URL: https://github.com/apache/datafusion/pull/10648#discussion_r1614514043
########## datafusion/physical-expr-common/src/utils.rs: ########## @@ -100,15 +103,37 @@ pub fn reverse_order_bys(order_bys: &[PhysicalSortExpr]) -> Vec<PhysicalSortExpr .collect() } +/// Converts `datafusion_expr::Expr` into corresponding `Arc<dyn PhysicalExpr>`. +/// If conversion is not supported yet, returns Error. +pub fn convert_logical_expr_to_physical_expr( Review Comment: I think @jayzhan211 ran into this as well when trying to move these functions (e.g. that the AggregateUDFImpl is in terms of logical `Expr` but often needs physical exps). One poential challenge with this implementation is the limited type support We already have this API to convert logical to physical exprs (but it isn't available in the implementation of first and last) https://docs.rs/datafusion/latest/datafusion/execution/context/struct.SessionState.html#method.create_physical_expr So I guess it feels to me like it would be good to avoid the duplication somehow, but I don't have a great idea of how to do it. Maybe we could call this function (and the sort one) something like `limited_convert_logical_expr_to_physical_expr` to convey that it only does some basic ones 🤔 ########## datafusion/expr/src/udaf.rs: ########## @@ -360,6 +391,27 @@ pub trait AggregateUDFImpl: Debug + Send + Sync { &[] } + /// Sets the flag specifying whether the requirement of the UDF is satisfied. Review Comment: I found this name / documenation to be a little confusing - specifically it isn't clear from just this documentation (the public API of `AggregageUDFImpl` *what* type of requirement is being satisfied. The documentation on AggregateExpr makes it clearer -- maybe we could find a way to bring some of that description to this location, or link to there for more details Also I think we could improve the name of this function 1. `with_...` is often used for builder style APIs in rust, which this is not 2. It uses "requirement" which is general Maybe it would be clearer it we called it `has_beneficial_ordering` or `input_ordered_correctly` 🤔 -- 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