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::coercible and port the relevant tests from Sail.
Fix TypeSignature::Coercible.

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-01-26T10:00:12Z

datafusion/expr/src/type_coercion/functions.rs

+
+/// Recursively traverses [`DataType::Dictionary`] to get the underlying value [`DataType`].
+/// For non-dictionary types, returns the default [`DataType`].
+fn base_dictionary_type_or_default_type(data_type: &DataType) -> DataType {


We can add dictionary support to datafusion/common/src/utils/mod.rs base_type

jayzhan211 · 2025-01-26T10:07:46Z

datafusion/sqllogictest/test_files/expr.slt

+50
+
+query I
+SELECT ascii(2)


In DuckDB, this doesn't work

D select ascii(2); Binder Error: No function matches the given name and argument types 'ascii(INTEGER_LITERAL)'. You might need to add explicit type casts. Candidate functions: ascii(VARCHAR) -> INTEGER LINE 1: select ascii(2); ^ D select ascii('2'); ┌────────────┐ │ ascii('2') │ │ int32 │ ├────────────┤ │ 50 │ └────────────┘

So does Postgres

postgres=# select ascii(2); ERROR: function ascii(integer) does not exist LINE 1: select ascii(2); ^ HINT: No function matches the given name and argument types. You might need to add explicit type casts. postgres=# select ascii('2'); ascii ------- 50 (1 row)

This used to work in DataFusion, but it is not consistent with other systems. I'm not sure whether we should introduce this behaviour back

The behavior is consistent with Spark and the tests also come from the Spark codebase (which we ported to Sail then here lol).

spark-sql (default)> SELECT ASCII(2); 50 Time taken: 0.952 seconds, Fetched 1 row(s) spark-sql (default)> SELECT ASCII('2'); 50 Time taken: 0.044 seconds, Fetched 1 row(s) spark-sql (default)>

jayzhan211 · 2025-01-26T10:09:06Z

datafusion/sqllogictest/test_files/expr.slt

@@ -571,8 +601,10 @@ select repeat('-1.2', arrow_cast(3, 'Int32'));
 ----
 -1.2-1.2-1.2

-query error DataFusion error: Error during planning: Internal error: Expect TypeSignatureClass::Native\(LogicalType\(Native\(Int64\), Int64\)\) but received Float64
+query T
 select repeat('-1.2', 3.2);


This should be error, the 2nd argument should be integer

Maybe the 2nd arg should be TypeSignatureClass::Integer instead of TypeSignatureClass::Native(logical_int64())? It's currently unimplemented with a TODO:

datafusion/datafusion/expr-common/src/signature.rs

Lines 216 to 218 in f775791

// TODO:

// Numeric

// Integer

Otherwise, it would seem strange to deviate from the behavior specified in the implementation of default_cast_forfor NativeType.

I do think that we should prioritize being permissive and general-purpose though as I was discussing here: #14296 (comment)

jayzhan211 · 2025-01-26T10:12:18Z

datafusion/optimizer/src/analyzer/type_coercion.rs

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


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

jayzhan211 · 2025-01-26T10:16:18Z

datafusion/expr/src/type_coercion/functions.rs

@@ -584,23 +544,7 @@ fn get_valid_types(
                match target_type_class {
                    TypeSignatureClass::Native(native_type) => {
                        let target_type = native_type.native();
-                        if &logical_type == target_type {


Does that mean others function that used Coercible String now also cast integer to string?

If it's TypeSignature::Coercible with a TypeSignatureClass::Native(logical_string()), then yes. Any function that specifies TypeSignature::Coercible with a TypeSignatureClass::Native should coerce according to the behavior implemented in the default_cast_for function for NativeType in order to be consistent.

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 #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

Open

21 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 added 3 commits January 25, 2025 23:33

find the common string type for TypeSignature::Coercible

437b83d

update coercible string

8fd9fb3

fix error msg and add tests for udfs

62b97c5

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 26, 2025

shehabgamin added 2 commits January 26, 2025 01:07

update docs to note that args are coercible string

46350c9

update expr test

3f0c870

github-actions bot added the documentation Improvements or additions to documentation label Jan 26, 2025

shehabgamin marked this pull request as ready for review January 26, 2025 09:45

shehabgamin requested a review from jayzhan211 January 26, 2025 09:45

jayzhan211 reviewed Jan 26, 2025

View reviewed changes

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 Jan 26, 2025

jayzhan211 Jan 26, 2025

shehabgamin Jan 26, 2025 •

edited

Loading

jayzhan211 Jan 26, 2025

shehabgamin Jan 26, 2025

jayzhan211 Jan 26, 2025

jayzhan211 Jan 26, 2025

shehabgamin Jan 26, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehabgamin Jan 26, 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

shehabgamin commented Jan 24, 2025 •

edited

Loading

jayzhan211 Jan 24, 2025 •

edited

Loading

shehabgamin Jan 26, 2025 •

edited

Loading