Refactor duplicate code in `type_coercion/functions.rs` #19518

Jefffrey · 2025-12-28T08:23:19Z

Which issue does this PR close?

Initial work before tackling Improve error messages when coercion fails #19004

Rationale for this change

Found lots of code duplicated here, so unifying them.

What changes are included in this PR?

Introduce new trait UDFCoercionExt to unify functions across scalar/aggregate/window UDFs for use in a single generic implementation. New unified functions fields_with_udf and get_valid_types_with_udf which are generic across this new trait.

Are these changes tested?

Existing tests.

Are there any user-facing changes?

Yes, deprecating functions.

Jefffrey · 2025-12-28T08:26:00Z

datafusion/ffi/src/udf/mod.rs

+    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<_>>();


Same as how UDAF handles this:

datafusion/datafusion/ffi/src/udaf/mod.rs

Lines 329 to 350 in e5ca510

unsafe extern "C" fn coerce_types_fn_wrapper(

udaf: &FFI_AggregateUDF,

arg_types: RVec<WrappedSchema>,

) -> FFIResult<RVec<WrappedSchema>> {

unsafe {

let udaf = udaf.inner();

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!(fields_with_aggregate_udf(&arg_fields, udaf))

.into_iter()

.map(|f| f.data_type().to_owned())

.collect::<Vec<_>>();

rresult!(vec_datatype_to_rvec_wrapped(&return_types))

}

}

Jefffrey · 2025-12-28T08:29:24Z

datafusion/expr/src/type_coercion/functions.rs

 ///
 /// 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")]


Personally I'm of the mind to just remove these functions completely

Jefffrey · 2025-12-28T08:32:41Z

datafusion/expr/src/type_coercion/functions.rs

 /// 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(


This function was only ever used in unit tests below 🤔

if its pub it still might be used elsewhere beyond DF engine

Jefffrey · 2025-12-28T08:32:55Z

datafusion/expr/src/type_coercion/functions.rs

-
-    matches!(
-        type_signature,
+    match type_signature {


Using match here makes it more obvious which variants are not well supported

Jefffrey · 2025-12-28T08:34:34Z

datafusion/expr/src/type_coercion/functions.rs

-                );
-            }
-        },
-        TypeSignature::OneOf(signatures) => signatures


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 SLT fix was needed

datafusion/ffi/src/udf/mod.rs

comphead · 2025-12-30T21:17:00Z

datafusion/optimizer/src/analyzer/type_coercion.rs

    }

-    let current_types = expressions
+    let current_fields = expressions


datafusion/optimizer/src/analyzer/type_coercion.rs

comphead · 2025-12-30T21:46:47Z

datafusion/expr/src/type_coercion/functions.rs

+}
+
+impl UDFCoercionExt for ScalarUDF {
+    fn name(&self) -> &str {


its bad the specialization feature is in nightly releases, so we could get rid of those parts

comphead

Thanks @Jefffrey

martin-g · 2025-12-31T04:43:50Z

datafusion/sqllogictest/test_files/errors.slt


 # WindowFunction wrong signature
-statement error DataFusion error: Error during planning: Failed to coerce arguments to satisfy a call to 'nth_value' function: coercion from Int32, Int64, Int64 to the signature OneOf\(\[Any\(0\), Any\(1\), Any\(2\)\]\) failed
+statement error DataFusion error: Error during planning: Internal error: Function 'nth_value' failed to match any signature


Is it really an Internal error ?
It is an error caused by the user's input.

This is what I aim to tackle in #19004

martin-g · 2025-12-31T04:47:39Z

datafusion/expr/src/type_coercion/functions.rs

            // Every signature failed, return the joined error
            if res.is_empty() {
-                internal_err!(
+                return internal_err!(


I think this should be a plan_err!()

This is what I aim to tackle in #19004

Co-authored-by: Oleks V <[email protected]>

Refactor duplicate code in type_coercion/functions.rs

48d5063

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) ffi Changes to the ffi crate labels Dec 28, 2025

make diff easier

9d922f4

Jefffrey commented Dec 28, 2025

View reviewed changes

Jefffrey added 2 commits December 28, 2025 19:04

clippy

bed669d

fix test

76e9a25

github-actions bot added the core Core DataFusion crate label Dec 28, 2025

Jefffrey marked this pull request as ready for review December 28, 2025 10:50

comphead reviewed Dec 30, 2025

View reviewed changes

datafusion/ffi/src/udf/mod.rs Outdated Show resolved Hide resolved

comphead reviewed Dec 30, 2025

View reviewed changes

datafusion/optimizer/src/analyzer/type_coercion.rs

}

let current_types = expressions

let current_fields = expressions

Copy link

Contributor

comphead Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

comphead reviewed Dec 30, 2025

View reviewed changes

datafusion/optimizer/src/analyzer/type_coercion.rs Outdated Show resolved Hide resolved

comphead reviewed Dec 30, 2025

View reviewed changes

comphead approved these changes Dec 30, 2025

View reviewed changes

martin-g reviewed Dec 31, 2025

View reviewed changes

Jefffrey and others added 3 commits December 31, 2025 16:04

Update datafusion/ffi/src/udf/mod.rs

5013d53

Co-authored-by: Oleks V <[email protected]>

Merge branch 'main' into refactor-type-coercion

0da838d

rename vars

4d93845

	unsafe extern "C" fn coerce_types_fn_wrapper(
	udaf: &FFI_AggregateUDF,
	arg_types: RVec<WrappedSchema>,
	) -> FFIResult<RVec<WrappedSchema>> {
	unsafe {
	let udaf = udaf.inner();

	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!(fields_with_aggregate_udf(&arg_fields, udaf))
	.into_iter()
	.map(\|f\| f.data_type().to_owned())
	.collect::<Vec<_>>();

	rresult!(vec_datatype_to_rvec_wrapped(&return_types))
	}
	}

Refactor duplicate code in type_coercion/functions.rs #19518

Are you sure you want to change the base?

Refactor duplicate code in type_coercion/functions.rs #19518

Uh oh!

Conversation

Jefffrey commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Refactor duplicate code in `type_coercion/functions.rs` #19518

Refactor duplicate code in `type_coercion/functions.rs` #19518

Jefffrey commented Dec 28, 2025 •

edited

Loading

Jefffrey Dec 28, 2025 •

edited

Loading

Jefffrey Dec 31, 2025 •

edited

Loading

Jefffrey Dec 31, 2025 •

edited

Loading