Fix Type Coercion for UDF Arguments #14268

shehabgamin · 2025-01-24T08:38:02Z

Which issue does this PR close?

Rationale for this change

A bug was introduced in DataFusion v43.0.0 that affects type coercion for UDF arguments. Sail's tests uncovered several of these regressions, which required explicit casting in multiple areas as a workaround during the upgrade to DataFusion 43.0.0.

The regressions identified by Sail's tests include the following functions:

ascii
bit_length
contains
ends_with
starts_with
octet_length

Upon digging into the code, I discovered the following:

The above functions didn't have Signature::coercible.
Signature::coercible was incomplete. Coercion would only happen if logical_type == target_type, logical_type == NativeType::Null, or target_type.is_integer() && logical_type.is_integer().
1. It seems like this was unintentional because the doc comments specified behavior that was not consistent with the logic implemented:
  
  For example, Coercible(vec![logical_float64()]) accepts arguments like vec![Int32] or vec![Float32] since i32 and f32 can be cast to f64.

What changes are included in this PR?

Update functions from the list above to have Signature::user_defined

Are these changes tested?

Yes.

Are there any user-facing changes?

…egression-utf8

jayzhan211 · 2025-01-24T11:11:16Z

datafusion/functions/src/string/ascii.rs

+            signature: Signature::one_of(
+                vec![
+                    TypeSignature::String(1),
+                    TypeSignature::Coercible(vec![TypeSignatureClass::Native(


If we use coercible(string), we don't need string since it is a more strict rule.

That's what I initially did, but after testing on Sail, I discovered new test failures related to coercing input that's all String (e.g. func(Utf8, Utf8View)).

The plan is to port all the relevant tests from Sail into this PR!

@jayzhan211 You can find the test failures here if interested!
lakehq/sail@372e13b#diff-bb36d996163d98235e107f8203c9c24be34ef71c84f8afa4a420a8e483102e3e

Although it was not my intention to apply this pattern on single arg functions. I'll get that fixed!

The current design for coercion may still have room for improvement. It would be beneficial to represent the function signature in a simpler and more concise manner, rather than relying on complex combinations of multiple, similar signatures.

Agreed! I'll add that in.

shehabgamin · 2025-01-25T11:56:18Z

datafusion/optimizer/src/analyzer/type_coercion.rs

+    }
+
+    #[test]
+    fn test_ascii_expr() -> Result<()> {


I would have preferred to place the various UDF tests within their respective files, but I couldn't due to circular dependencies.

Ending up putting the tests in the .slt file, but figured we can still leave this test here.

We can remove this test if the purpose of the test is covered already in slt

…egression-utf8

jayzhan211 · 2025-02-03T13:42:45Z

@shehabgamin #14440 I come out a flexible version of Signature::CoercibleV2 (temporary name), it can replace Signature::Coercible, Signature::String, Signature::Numeric, Signature::Exact, Signature::Uniform and Signature::Comparable.

The most difference is that the implicit coercion is now defined by the user. This way any change for datafusion doesn't impact downstream projects unlike the existing signature like Signature::String or Signature::Coercible.

I probably don't have time to push it forward in the recent days, if you are interested in it you can work on it.

We can implement first version for the functions you mentioned, I believe the change makes more sense

alamb · 2025-02-03T15:36:41Z

FWIW I think @Omega359 hit the same "int no longer automatically coerces to String" in his application too: #14230 (comment)

shehabgamin · 2025-02-03T22:15:53Z

I'll work on this tonight @alamb @jayzhan211

…-43-regression-utf8

shehabgamin · 2025-02-04T05:10:44Z

@shehabgamin #14440 I come out a flexible version of Signature::CoercibleV2 (temporary name), it can replace Signature::Coercible, Signature::String, Signature::Numeric, Signature::Exact, Signature::Uniform and Signature::Comparable.

The most difference is that the implicit coercion is now defined by the user. This way any change for datafusion doesn't impact downstream projects unlike the existing signature like Signature::String or Signature::Coercible.

I probably don't have time to push it forward in the recent days, if you are interested in it you can work on it.

I think it makes sense to work on this for DataFusion 46!

We can implement first version for the functions you mentioned, I believe the change makes more sense

@jayzhan211 If I am understanding you correctly, Done! I added AnyNative and applied it to the functions mentioned.

CC @alamb @Omega359

jayzhan211 · 2025-02-04T05:50:54Z

@shehabgamin #14440 I come out a flexible version of Signature::CoercibleV2 (temporary name), it can replace Signature::Coercible, Signature::String, Signature::Numeric, Signature::Exact, Signature::Uniform and Signature::Comparable.
The most difference is that the implicit coercion is now defined by the user. This way any change for datafusion doesn't impact downstream projects unlike the existing signature like Signature::String or Signature::Coercible.
I probably don't have time to push it forward in the recent days, if you are interested in it you can work on it.

I think it makes sense to work on this for DataFusion 46!

We can implement first version for the functions you mentioned, I believe the change makes more sense

@jayzhan211 If I am understanding you correctly, Done! I added AnyNative and applied it to the functions mentioned.

CC @alamb @Omega359

I don't quite understand why we are adding more TypeSignatureClass here 😕

As we have discussed, we should avoid using old Coercible signature and also the TypeSignatureClass that is used in Coercible

shehabgamin · 2025-02-04T05:52:48Z

I may add a new TypeSignatureClass in this pr to expose default_cast_for so that it's accessible by downstream users

Not sure do we need this 🤔 since we might not need this in the future, but if this helps downstream project then it is fine to me to add it for now and remove it in the future

We can implement first version for the functions you mentioned, I believe the change makes more sense

I don't quite understand why we are adding more TypeSignatureClass here 😕

@jayzhan211 What did I miss here?

jayzhan211 · 2025-02-04T06:02:18Z

@jayzhan211 What did I miss here?

As we have discussed, we should avoid using old Coercible signature and also the TypeSignatureClass that is used in Coercible, because any change might impact downstream projects, although if we add new TypeSignatureClass like what you did will not, but adding more soon to remove things are not good idea.

We can implement first version for the functions you mentioned, I believe the change makes more sense

I think we can work directly on CoercibleV2 I mentioned for these functions

ascii
bit_length
contains
ends_with
starts_with
octet_length

shehabgamin · 2025-02-04T06:15:44Z

As we have discussed, we should avoid using old Coercible signature and also the TypeSignatureClass that is used in Coercible, because any change might impact downstream projects, although if we add new TypeSignatureClass like what you did will not, but adding more soon to remove things are not good idea

@jayzhan211 I thought you said it was okay to add a new signature if it helps downstream projects. See here:
#14268 (comment)

I reverted Native back to the original logic before this PR and then I added AnyNative.

I think we can work directly on CoercibleV2 I mentioned for these functions

IMO CoercibleV2 would be out of scope for this PR. It looks like @alamb would like to get this PR sorted out if possible for 45 (see here: #14008 (comment))

If I am understanding you correctly, you are okay with adding a new signature but not applying them to the UDFs in this PR? Should I apply User-defined coercion as you were mentioning earlier?

jayzhan211 · 2025-02-04T06:33:52Z

if it helps downstream projects

This is the point, I don't quite understand why adding signature for default_cast_to logic helps. I thought you want to solve the issues for these datafusion functions, so I don't know what others functions you want to solve and they are only possible if we have AnyNative.

Even if there is such case that we really need AnyNative, it seems strange to me to add in TypeSignatureClass but should be another new TypeSignature.

IMO CoercibleV2 would be out of scope for this PR

I think this is the solution to the issue we have, we need coercible like signature but not fixed logic exposed to the user since it makes any changes to it breaking change, while CoercibleV2 is not the case.

@shehabgamin First of all, what are the issues we are solving? Are these functions in datafusion?

ascii
bit_length
contains
ends_with
starts_with
octet_length

Are there others issues? Why AnyNative helps?

jayzhan211 · 2025-02-04T06:38:40Z

Btw, if your proposed signature doesn't used by any functions in datafusion. You should use TypeSignature::UserDefined it is used for any other customize logic for downstream.

shehabgamin · 2025-02-04T07:34:52Z

This is the point, I don't quite understand why adding signature for default_cast_to logic helps. I thought you want to solve the issues for these datafusion functions, so I don't know what others functions you want to solve and they are only possible if we have AnyNative.

@shehabgamin First of all, what are the issues we are solving? Are these functions in datafusion?
ascii
bit_length
contains
ends_with
starts_with
octet_length
Are there others issues? Why AnyNative helps?

@jayzhan211 The regression is that all these functions before DataFusion 43 would coerce:

Binary -> String
Int -> String

But now they no longer do.

I am trying to find some middle ground here. I am happy to implement any signature for the functions in this PR, but currently, default_cast_for is inaccessible for downstream users, adding AnyNative makes it accessible. Sail implements hundreds of custom UDFs, so it would be nice to have access to default_cast_for. We're not the only downstream project that has a need for flexible coercion like this (see here: #14230 (comment)).

Even if there is such case that we really need AnyNative, it seems strange to me to add in TypeSignatureClass but should be another new TypeSignature.

I'm not sure that AnyNative should be a new TypeSignature because TypeSignature::Coercible most accurately describes the behavior.

Btw, if your proposed signature doesn't used by any functions in datafusion. You should use TypeSignature::UserDefined it is used for any other customize logic for downstream.

The point is that default_cast_for is not accessible downstream. Should we just make default_cast_for a public function?

jayzhan211 · 2025-02-04T08:13:36Z

Making default_cast_for or any casting logic publicly accessible introduces the risk of additional regressions with every change. I believe it was one of our worst decisions, and I hope we can avoid repeating it in the future. 🙏🏻

Sail implements hundreds of custom UDFs

I guess this is the real issue, how do we have a solution that is easier to solve those UDFs

I don't have the best solution in my mind now, but list the possible solutions

The reason to not change TypeSignature::Coercible is to avoid another regression for others that use the existing logic.
The reason to not introduce TypeSignature::CoercibleV2 is because it requires tons of change for 100+ UDFs
The reason to not make default_cast_for public is what I mentioned above.

@shehabgamin Do you think we can write some utils function so we can make transferring to TypeSignature::CoercibleV2 easier?

shehabgamin · 2025-02-04T17:17:50Z

@jayzhan211 To keep it simple ill just remove AnyNative and use coerce_types so we don't block this PR any longer. We can have a larger discussion and align on goals afterwards!

CC @alamb @Omega359

shehabgamin · 2025-02-04T20:43:49Z

@jayzhan211 To keep it simple ill just remove AnyNative and use coerce_types so we don't block this PR any longer. We can have a larger discussion and align on goals afterwards!

CC @alamb @Omega359

Done, this should be good to merge now. AnyNative was removed, Native was reverted back to original logic, and for the 6 UDFs in the PR Signature::user_defined was used.

findepi · 2025-02-05T07:01:43Z

I fully recognize this creates behavior that diverges from PostgreSQL/DuckDB semantics for the various UDFs in this PR. However, there’s a critical distinction: System contracts vs. SQL dialect behavior.

[...]

The current approach keeps the door open for both strict and permissive use cases through explicit signature choices. With these changes, users can still achieve PostgreSQL/DuckDB behavior by defining UDFs with existing signatures while still leveraging broader coercion where appropriate.

The function coercions are not enough for building a tailored system. Relational operators also do coercions (the set operators: union, intersect, except).

In any case, for certain system designs -- those who take on responsibility of implementing their particular SQL dialect behavior before handing over the control over to DF core -- it's desirable to opt out from any coercion logic at all. @shehabgamin @linhr Given the Sail design, you might be interested in #12723.

jayzhan211 · 2025-02-05T09:30:01Z

datafusion/functions/src/string/ascii.rs

+        let arg_type = &arg_types[0];
+        let current_native_type: NativeType = arg_type.into();
+        let target_native_type = NativeType::String;
+        if current_native_type.is_integer()


should we support integer? It is not consistent with Postgres/DuckDB

Up to you and @alamb

Let's remove it

@jayzhan211 Forsure, let's make sure @alamb is okay with this too before I go ahead and make the change.

jayzhan211 · 2025-02-05T09:30:34Z

datafusion/functions/src/string/bit_length.rs

+        let arg_type = &arg_types[0];
+        let current_native_type: NativeType = arg_type.into();
+        let target_native_type = NativeType::String;
+        if current_native_type.is_integer()


same with this

datafusion/expr/src/type_coercion/functions.rs

jayzhan211 · 2025-02-06T12:29:13Z

datafusion/expr-common/src/signature.rs

-    // Numeric
-    // Integer
+    Numeric(LogicalTypeRef),
+    Integer(LogicalTypeRef),


@shehabgamin I found that we might not need LogicalTypeRef.
This is designed to accept all the Integer, so if the given type is integer, we keep it as it is.

If we want specific integer type, then we should use Native instead. Does this makes sense to you?

Numeric is the same

jayzhan211 · 2025-02-06T12:31:51Z

datafusion/expr/src/type_coercion/functions.rs

-
+                    }
+                    TypeSignatureClass::Integer(native_type) => {
+                        let target_type = native_type.native();


Suggested change

let target_type = native_type.native();

Ok(current_type.to_owned())

jayzhan211 · 2025-02-06T12:32:03Z

datafusion/expr/src/type_coercion/functions.rs

-                            return target_type.default_cast_for(current_type);
+                    }
+                    TypeSignatureClass::Numeric(native_type) => {
+                        let target_type = native_type.native();


Suggested change

let target_type = native_type.native();

Ok(current_type.to_owned())

Incorrect usage

jayzhan211 · 2025-02-06T13:19:54Z

datafusion/expr-common/src/signature.rs

-                    TypeSignatureClass::Native(l) => get_data_types(l.native()),
+                    TypeSignatureClass::Native(l)
+                    | TypeSignatureClass::Numeric(l)
+                    | TypeSignatureClass::Integer(l) => get_data_types(l.native()),


get_data_types is used only in get_possible_types

shehabgamin added 12 commits January 22, 2025 01:45

Fix DF 43 regression coerce ascii input to string

bcc0620

datafusion-testing submodule has new commits

a8539c1

Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…

4e4cb02

…egression-utf8

implicit cast int to string

eb61d49

Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…

a6f62a0

…egression-utf8

fix can_coerce_to and add tests

5afbfc0

update deprecation message for values exec

36a23b7

Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…

d2eadea

…egression-utf8

lint

944e0a3

coerce to string

2d77206

Adjust type signature

1a27626

fix comment

e714ba1

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions functions labels Jan 24, 2025

shehabgamin mentioned this pull request Jan 24, 2025

DataFusion Regression (Starting in v43): Type Coercion for UDF Arguments (X --> String) for Specified UDFs #14230

Open

jayzhan211 reviewed Jan 24, 2025

View reviewed changes

shehabgamin changed the title ~~Fix DF 43 Regression: Coerce Various Scalar Func Args to String~~ Fix DF 43 Coercion Regression: Improve TypeSignature::Coercible Jan 25, 2025

shehabgamin added 4 commits January 24, 2025 20:35

clean up clippy warnings

93d75b1

type signature coercible

5067223

bump pyo3

4d395e2

udf type coercion

ec8ccd1

github-actions bot added the common Related to common crate label Jan 25, 2025

shehabgamin changed the title ~~Fix DF 43 Coercion Regression: Improve TypeSignature::Coercible~~ Fix DF 43 Type Coercion Regression: Improve TypeSignature::Coercible Jan 25, 2025

shehabgamin changed the title ~~Fix DF 43 Type Coercion Regression: Improve TypeSignature::Coercible~~ Fix Type Coercion for UDF Arguments Jan 25, 2025

shehabgamin mentioned this pull request Jan 25, 2025

Release DataFusion 45.0.0 #14008

Closed

32 tasks

moving testing out of functions crate due to circular dependencies

d78877a

github-actions bot added the optimizer Optimizer rules label Jan 25, 2025

shehabgamin commented Jan 25, 2025

View reviewed changes

Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…

041e4ac

…egression-utf8

shehabgamin changed the base branch from main to branch-45 February 4, 2025 04:36

shehabgamin added 3 commits February 3, 2025 20:44

Merge branch 'branch-45' of github.com:lakehq/datafusion into sail-df…

f519d92

…-43-regression-utf8

revert native to original and add anynative

1f806b2

use anynative

25bcb58

use user defined rules instead

1d94e5c

jayzhan211 previously approved these changes Feb 5, 2025

View reviewed changes

jayzhan211 reviewed Feb 5, 2025

View reviewed changes

datafusion/expr/src/type_coercion/functions.rs Show resolved Hide resolved

jayzhan211 reviewed Feb 6, 2025

View reviewed changes

jayzhan211 mentioned this pull request Feb 7, 2025

Signature::Coercible with user defined implicit casting #14440

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Type Coercion for UDF Arguments #14268

Fix Type Coercion for UDF Arguments #14268

shehabgamin commented Jan 24, 2025 •

edited

Loading

jayzhan211 Jan 24, 2025

shehabgamin Jan 24, 2025

shehabgamin Jan 24, 2025

shehabgamin Jan 24, 2025

jayzhan211 Jan 24, 2025 •

edited

Loading

shehabgamin Jan 25, 2025

shehabgamin Jan 25, 2025

shehabgamin Jan 26, 2025

jayzhan211 Jan 26, 2025

jayzhan211 commented Feb 3, 2025 •

edited

Loading

alamb commented Feb 3, 2025

shehabgamin commented Feb 3, 2025

shehabgamin commented Feb 4, 2025 •

edited

Loading

jayzhan211 commented Feb 4, 2025 •

edited

Loading

shehabgamin commented Feb 4, 2025

jayzhan211 commented Feb 4, 2025 •

edited

Loading

shehabgamin commented Feb 4, 2025

jayzhan211 commented Feb 4, 2025 •

edited

Loading

jayzhan211 commented Feb 4, 2025 •

edited

Loading

shehabgamin commented Feb 4, 2025 •

edited

Loading

jayzhan211 commented Feb 4, 2025 •

edited

Loading

shehabgamin commented Feb 4, 2025 •

edited

Loading

shehabgamin commented Feb 4, 2025

findepi commented Feb 5, 2025

jayzhan211 Feb 5, 2025 •

edited

Loading

shehabgamin Feb 5, 2025

jayzhan211 Feb 5, 2025

shehabgamin Feb 6, 2025

jayzhan211 Feb 5, 2025

jayzhan211 Feb 6, 2025

jayzhan211 Feb 6, 2025

jayzhan211 Feb 6, 2025

jayzhan211 Feb 6, 2025

	let target_type = native_type.native();
	Ok(current_type.to_owned())

Fix Type Coercion for UDF Arguments #14268

Are you sure you want to change the base?

Fix Type Coercion for UDF Arguments #14268

Conversation

shehabgamin commented Jan 24, 2025 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 commented Feb 3, 2025 • edited Loading

alamb commented Feb 3, 2025

shehabgamin commented Feb 3, 2025

shehabgamin commented Feb 4, 2025 • edited Loading

jayzhan211 commented Feb 4, 2025 • edited Loading

shehabgamin commented Feb 4, 2025

jayzhan211 commented Feb 4, 2025 • edited Loading

shehabgamin commented Feb 4, 2025

jayzhan211 commented Feb 4, 2025 • edited Loading

jayzhan211 commented Feb 4, 2025 • edited Loading

shehabgamin commented Feb 4, 2025 • edited Loading

jayzhan211 commented Feb 4, 2025 • edited Loading

shehabgamin commented Feb 4, 2025 • edited Loading

shehabgamin commented Feb 4, 2025

findepi commented Feb 5, 2025

jayzhan211 Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehabgamin commented Jan 24, 2025 •

edited

Loading

jayzhan211 Jan 24, 2025 •

edited

Loading

jayzhan211 commented Feb 3, 2025 •

edited

Loading

shehabgamin commented Feb 4, 2025 •

edited

Loading

jayzhan211 commented Feb 4, 2025 •

edited

Loading

jayzhan211 commented Feb 4, 2025 •

edited

Loading

jayzhan211 commented Feb 4, 2025 •

edited

Loading

jayzhan211 commented Feb 4, 2025 •

edited

Loading

shehabgamin commented Feb 4, 2025 •

edited

Loading

jayzhan211 commented Feb 4, 2025 •

edited

Loading

shehabgamin commented Feb 4, 2025 •

edited

Loading

jayzhan211 Feb 5, 2025 •

edited

Loading