Jefffrey commented on code in PR #19518:
URL: https://github.com/apache/datafusion/pull/19518#discussion_r2649559283
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -239,20 +250,23 @@ pub fn data_types(
}
fn is_well_supported_signature(type_signature: &TypeSignature) -> bool {
- if let TypeSignature::OneOf(signatures) = type_signature {
- return signatures.iter().all(is_well_supported_signature);
- }
-
- matches!(
- type_signature,
+ match type_signature {
Review Comment:
Using match here makes it more obvious which variants are not well supported
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -323,69 +339,15 @@ fn get_valid_types_with_scalar_udf(
// Every signature failed, return the joined error
if res.is_empty() {
- internal_err!(
+ return internal_err!(
"Function '{}' failed to match any signature, errors: {}",
func.name(),
errors.join(",")
- )
+ );
} else {
- Ok(res)
+ res
}
}
- _ => get_valid_types(func.name(), signature, current_types),
- }
-}
-
-fn get_valid_types_with_aggregate_udf(
- signature: &TypeSignature,
- current_types: &[DataType],
- func: &AggregateUDF,
-) -> Result<Vec<Vec<DataType>>> {
- let valid_types = match signature {
- TypeSignature::UserDefined => match func.coerce_types(current_types) {
- Ok(coerced_types) => vec![coerced_types],
- Err(e) => {
- return exec_err!(
- "Function '{}' user-defined coercion failed with {:?}",
- func.name(),
- e.strip_backtrace()
- );
- }
- },
- TypeSignature::OneOf(signatures) => signatures
Review Comment:
The scalar fn version actually has extra functionality in that it collects
the errors together for oneof; unifying this code with aggregate/window udfs
brings in this functionality for them, hence test change was needed
##########
datafusion/ffi/src/udf/mod.rs:
##########
@@ -140,8 +140,16 @@ unsafe extern "C" fn coerce_types_fn_wrapper(
) -> FFIResult<RVec<WrappedSchema>> {
let arg_types = rresult_return!(rvec_wrapped_to_vec_datatype(&arg_types));
+ let arg_fields = arg_types
+ .iter()
+ .map(|dt| Field::new("f", dt.clone(), true))
+ .map(Arc::new)
+ .collect::<Vec<_>>();
let return_types =
- rresult_return!(data_types_with_scalar_udf(&arg_types, udf.inner()));
+ rresult_return!(fields_with_udf(&arg_fields, udf.inner().as_ref()))
+ .into_iter()
+ .map(|f| f.data_type().to_owned())
+ .collect::<Vec<_>>();
Review Comment:
Same as how UDAF handles this:
https://github.com/apache/datafusion/blob/e5ca510dcf03f892bab0465619cffb0d23196d0c/datafusion/ffi/src/udaf/mod.rs#L329-L350
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -197,6 +207,7 @@ pub fn fields_with_window_udf(
///
/// For more details on coercion in general, please see the
/// [`type_coercion`](crate::type_coercion) module.
+#[deprecated(since = "52.0.0", note = "use fields_with_udf")]
pub fn data_types(
Review Comment:
This function was only ever used in unit tests below 🤔
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -39,45 +39,81 @@ use datafusion_expr_common::{
use itertools::Itertools as _;
use std::sync::Arc;
+/// Extension trait to unify common functionality between [`ScalarUDF`],
[`AggregateUDF`]
+/// and [`WindowUDF`] for use by signature coercion functions.
+pub trait UDFCoercionExt {
+ /// Should delegate to [`ScalarUDF::name`], [`AggregateUDF::name`] or
[`WindowUDF::name`].
+ fn name(&self) -> &str;
+ /// Should delegate to [`ScalarUDF::signature`],
[`AggregateUDF::signature`]
+ /// or [`WindowUDF::signature`].
+ fn signature(&self) -> &Signature;
+ /// Should delegate to [`ScalarUDF::coerce_types`],
[`AggregateUDF::coerce_types`]
+ /// or [`WindowUDF::coerce_types`].
+ fn coerce_types(&self, arg_types: &[DataType]) -> Result<Vec<DataType>>;
+}
+
+impl UDFCoercionExt for ScalarUDF {
+ fn name(&self) -> &str {
+ self.name()
+ }
+
+ fn signature(&self) -> &Signature {
+ self.signature()
+ }
+
+ fn coerce_types(&self, arg_types: &[DataType]) -> Result<Vec<DataType>> {
+ self.coerce_types(arg_types)
+ }
+}
+
+impl UDFCoercionExt for AggregateUDF {
+ fn name(&self) -> &str {
+ self.name()
+ }
+
+ fn signature(&self) -> &Signature {
+ self.signature()
+ }
+
+ fn coerce_types(&self, arg_types: &[DataType]) -> Result<Vec<DataType>> {
+ self.coerce_types(arg_types)
+ }
+}
+
+impl UDFCoercionExt for WindowUDF {
+ fn name(&self) -> &str {
+ self.name()
+ }
+
+ fn signature(&self) -> &Signature {
+ self.signature()
+ }
+
+ fn coerce_types(&self, arg_types: &[DataType]) -> Result<Vec<DataType>> {
+ self.coerce_types(arg_types)
+ }
+}
+
/// Performs type coercion for scalar function arguments.
///
/// Returns the data types to which each argument must be coerced to
/// match `signature`.
///
/// For more details on coercion in general, please see the
/// [`type_coercion`](crate::type_coercion) module.
+#[deprecated(since = "52.0.0", note = "use fields_with_udf")]
Review Comment:
Personally I'm of the mind to just remove these functions completely
--
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]
