You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/11/07 15:23:14 UTC
[PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
alamb opened a new pull request, #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079
## Which issue does this PR close?
Part of https://github.com/apache/arrow-datafusion/issues/8045
Closes https://github.com/apache/arrow-datafusion/pull/8039
## Rationale for this change
Rationale is that we can add new features to ScalarUDF to bring it up to parity with BuiltInScalarFunction (such as adding `montonicity`) in a backwards compatible way if the fields are not pub. If the fields
In order to maintain parity with the other user defined APIs (aggregates and window functions) and to prepare them for an eventual consolidation as well, apply the same to `AggregateUDF` and `WindowUDF`
see https://github.com/apache/arrow-datafusion/issues/8045 for more details
## What changes are included in this PR?
Make all fields of ScalarUDF private and add accessors.
2. Change return_type to just call the function rather than passing back a function implementation
## Are these changes tested?
yes, by existing tests
## Are there any user-facing changes?
yes, users can no longer create `ScalarUDF`, `AggregateUDF` or `WindowUDF` with the `struct` syntax:
So previously this was allowed:
```rust
let udf = ScalarUDF {
name: "foo".to_string()
signature,
return_type,
fun
}
```
Now, users must call `ScalarUDF::new`:
```rust
let udf = ScalarUDF::new(
"foo",
signature,
return_type,
fun
);
```
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
Posted by "yukkit (via GitHub)" <gi...@apache.org>.
yukkit commented on code in PR #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079#discussion_r1393939817
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -86,7 +86,7 @@ impl ExprSchemable for Expr {
.iter()
.map(|e| e.get_type(schema))
.collect::<Result<Vec<_>>>()?;
- Ok((fun.return_type)(&data_types)?.as_ref().clone())
+ Ok(fun.return_type(&data_types)?)
Review Comment:
This piece of code looks really clean.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079#issuecomment-1806787180
I plan to merge this on 2023-11-14 unless anyone objects or would like additional time to review (after it has been open for a week)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079#issuecomment-1807096767
@andygrove or @Dandandan I wonder if you might have time to review (and hopefully approve) this PR?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079#issuecomment-1806408005
> When ScalarUDF fields needs to be extended, it should look like:
Yes, that is a great idea!
I am actually hoping it looks more like
```rust
let udf = ScalarUDF::new_from_trait(
MyStructThatImplementsATrait{}
)
.with_aliases(["fo", "foo_func"])
```
But we'll see !
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079#issuecomment-1812477447
Thank you everyone
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make fields of `ScalarUDF` , `AggregateUDF` and `WindowUDF` non `pub` [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #8079:
URL: https://github.com/apache/arrow-datafusion/pull/8079#issuecomment-1812162091
Looks like this had merge conflicts, but there are also other merge conflicts in-flight... - https://github.com/apache/arrow-datafusion/pull/8186
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org