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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
