Jefffrey commented on code in PR #19603:
URL: https://github.com/apache/datafusion/pull/19603#discussion_r2657077653
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -678,6 +609,33 @@ impl ExprSchemable for Expr {
}
}
+/// Verify that function is invoked with correct number and type of arguments
as
+/// defined in `TypeSignature`.
+fn verify_function_arguments<F: UDFCoercionExt>(
Review Comment:
With the new `UDFCoercionExt` trait we can now more easily share common code
between scalar, aggregate and window UDFs
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -688,93 +646,6 @@ fn unwrap_certainly_null_expr(expr: &Expr) -> &Expr {
}
}
-impl Expr {
- /// Common method for window functions that applies type coercion
- /// to all arguments of the window function to check if it matches
- /// its signature.
- ///
- /// If successful, this method returns the data type and
- /// nullability of the window function's result.
- ///
- /// Otherwise, returns an error if there's a type mismatch between
- /// the window function's signature and the provided arguments.
- fn data_type_and_nullable_with_window_function(
Review Comment:
This has been inlined to `to_field()`
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -692,13 +686,11 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
let args = match &fun {
expr::WindowFunctionDefinition::AggregateUDF(udf) => {
- coerce_arguments_for_signature_with_aggregate_udf(
- args,
- self.schema,
- udf,
- )?
+ coerce_arguments_for_signature(args, self.schema,
udf.as_ref())?
+ }
+ expr::WindowFunctionDefinition::WindowUDF(udf) => {
+ coerce_arguments_for_signature(args, self.schema,
udf.as_ref())?
Review Comment:
This arm for coercing non-aggregate window UDFs actually was missing; adding
this back in lead to discovering some UDF window tests were incorrect (see next
comment)
##########
datafusion/core/tests/user_defined/user_defined_window_functions.rs:
##########
@@ -536,7 +536,7 @@ impl OddCounter {
impl SimpleWindowUDF {
fn new(test_state: Arc<TestState>) -> Self {
let signature =
- Signature::exact(vec![DataType::Float64],
Volatility::Immutable);
+ Signature::exact(vec![DataType::Int64],
Volatility::Immutable);
Review Comment:
This sample UDF declared it accepted f64's but internally it acted as if
input was i64; this was never caught because apparently type coercion wasn't
being run on non-aggregate window functions, and the input data was already
i64. Nice little fix.
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -94,58 +94,6 @@ impl UDFCoercionExt for WindowUDF {
}
}
-/// Performs type coercion for scalar function arguments.
Review Comment:
Reorganizing these so the only public function of this file is near the top
(`fields_with_udf()`)
##########
datafusion/functions-nested/src/make_array.rs:
##########
@@ -80,10 +79,7 @@ impl Default for MakeArray {
impl MakeArray {
pub fn new() -> Self {
Self {
- signature: Signature::one_of(
- vec![TypeSignature::Nullary, TypeSignature::UserDefined],
Review Comment:
I don't really like the idea of having `UserDefined` nested within a `OneOf`
signature, so amending some functions which are doing this
##########
datafusion/expr/src/utils.rs:
##########
@@ -937,6 +937,7 @@ pub fn find_valid_equijoin_key_pair(
/// round(Float32)
/// ```
#[expect(clippy::needless_pass_by_value)]
+#[deprecated(since = "52.0.0", note = "Internal function")]
pub fn generate_signature_error_msg(
Review Comment:
Similarly removing this from our public API since I see no need for it to be
public
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -152,43 +152,10 @@ impl ExprSchemable for Expr {
}
}
}
- Expr::ScalarFunction(_func) => {
- let return_type = self.to_field(schema)?.1.data_type().clone();
- Ok(return_type)
- }
- Expr::WindowFunction(window_function) => self
- .data_type_and_nullable_with_window_function(schema,
window_function)
- .map(|(return_type, _)| return_type),
- Expr::AggregateFunction(AggregateFunction {
- func,
- params: AggregateFunctionParams { args, .. },
- }) => {
- let fields = args
- .iter()
- .map(|e| e.to_field(schema).map(|(_, f)| f))
- .collect::<Result<Vec<_>>>()?;
- let new_fields = fields_with_udf(&fields, func.as_ref())
- .map_err(|err| {
- let data_types = fields
- .iter()
- .map(|f| f.data_type().clone())
- .collect::<Vec<_>>();
- plan_datafusion_err!(
- "{} {}",
- match err {
- DataFusionError::Plan(msg) => msg,
- err => err.to_string(),
- },
- utils::generate_signature_error_msg(
- func.name(),
- func.signature().clone(),
- &data_types
- )
- )
- })?
- .into_iter()
- .collect::<Vec<_>>();
- Ok(func.return_field(&new_fields)?.data_type().clone())
+ Expr::ScalarFunction(_)
+ | Expr::WindowFunction(_)
+ | Expr::AggregateFunction(_) => {
+ Ok(self.to_field(schema)?.1.data_type().clone())
Review Comment:
Reusing existing logic from `to_field()` implementation
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -800,6 +800,7 @@ fn maybe_data_types_without_coercion(
/// (losslessly converted) into a value of `type_to`
///
/// See the module level documentation for more detail on coercion.
+#[deprecated(since = "52.0.0", note = "Unused internal function")]
Review Comment:
This was only ever used in a test; trying to minimize our API surface area
for simplicity so deprecating this
--
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]
