alamb commented on code in PR #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079#discussion_r1385101907
##########
datafusion/expr/src/udaf.rs:
##########
@@ -112,4 +114,35 @@ impl AggregateUDF {
order_by: None,
})
}
+
+ /// Returns this function's name
+ pub fn name(&self) -> &str {
+ &self.name
+ }
+
+ /// Returns this function's signature (what input types are accepted)
+ pub fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ /// Return the type of the function given its input types
+ pub fn return_type(&self, args: &[DataType]) -> Result<DataType> {
+ // Old API returns an Arc of the datatype for some reason
Review Comment:
Another change I did was to move the handling of function pointers into the
implementation of `AggregateUDF` etc -- so rather than passing out parts of
themselves (a function pointer) the rest of DataFusion now just calls the
function to get the appropriate information
This is in preparation for potentially changing the implementation of `*UDF`
internally
##########
datafusion/expr/src/window_function.rs:
##########
@@ -171,12 +171,8 @@ impl WindowFunction {
WindowFunction::BuiltInWindowFunction(fun) => {
fun.return_type(input_expr_types)
}
- WindowFunction::AggregateUDF(fun) => {
- Ok((*(fun.return_type)(input_expr_types)?).clone())
- }
- WindowFunction::WindowUDF(fun) => {
- Ok((*(fun.return_type)(input_expr_types)?).clone())
- }
+ WindowFunction::AggregateUDF(fun) =>
fun.return_type(input_expr_types),
Review Comment:
I think this now reads more nicely and idomatically
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -1523,12 +1523,12 @@ fn roundtrip_window() {
Ok(Box::new(DummyWindow {}))
}
- let dummy_window_udf = WindowUDF {
- name: String::from("dummy_udwf"),
- signature: Signature::exact(vec![DataType::Float64],
Volatility::Immutable),
- return_type: Arc::new(return_type),
- partition_evaluator_factory: Arc::new(make_partition_evaluator),
- };
+ let dummy_window_udf = WindowUDF::new(
Review Comment:
This is a pretty good example of the changes that would be required from
user code
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -77,14 +77,14 @@ impl ScalarFunctionExpr {
name: &str,
fun: ScalarFunctionImplementation,
args: Vec<Arc<dyn PhysicalExpr>>,
- return_type: &DataType,
+ return_type: DataType,
monotonicity: Option<FuncMonotonicity>,
) -> Self {
Self {
fun,
name: name.to_owned(),
args,
- return_type: return_type.clone(),
+ return_type,
Review Comment:
There is no need to clone within the function -- I also changed this
signature so the caller can pass in a DataType if they already have it without
forcing a new clone
--
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]
