Signature::Coercible with user defined implicit casting #14440

jayzhan211 · 2025-02-03T13:37:38Z

Which issue does this PR close?

Part of Epic: Simplify functions signature with LogicalType #13301.
It would be helpful for Fix Type Coercion for UDF Arguments #14268

Rationale for this change

Inspired from Fix Type Coercion for UDF Arguments #14268 from @shehabgamin we used to define some implicit coercion inside datafusion which turns out it is not flexible and doesn't work for all the use case. We turn it explicitly and customizable by default.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jayzhan211 · 2025-02-03T13:40:04Z

datafusion/expr-common/src/signature.rs

+
+#[derive(Debug, Clone, Eq, PartialOrd, Hash)]
+pub struct ParameterType {
+    pub param_type: LogicalTypeRef,


param_type: target type of function signature
allowed_casts: implicit coercion allowed to cast to target type.

For example,
param_type: string
allowed_casts: binary, int

Valid: All are casted to string func(string) func(binary) func(int) Invalid: func(float or other types)

I am worried that Vec<Vec<ParameterType>> might become challenging to reason about. It would also be confusing about when one use this new signature rather than Signature::Coercable 🤔

Given this seems very similar to Signature::Coercable, and Signature::Coercable mirrors what we want pretty well, could add some new information there on the allowed coercions rather than an entire new type signature. Something like extending Coercable with rules could be used to coerce to the target type:

pub enum TypeSignature { ... Coercible(Vec<Coercion>), ... }

Where Coercion looks like

struct Coercion { desired_type: TypeSignatureClass, allowed_casts: ... // also includes an option for User Defined? }

🤔

I would like to have a breaking change to Signature::Coercible too, the only concern is whether this cause regression or large impact to downstream

@shehabgamin If we replace CoercibleV2 with Signature::Coercible would it be a large change we should be concerned to?

If not, I'll replace Signature::Coercible with Signature::CoercibleV2.

Signed-off-by: Jay Zhan <[email protected]>

jayzhan211 · 2025-02-07T01:55:22Z

datafusion/functions/src/string/repeat.rs

                vec![
-                    TypeSignatureClass::Native(logical_string()),
-                    TypeSignatureClass::Native(logical_int64()),


In v1 version, any integer is casted to the defined NativeType's default casted type which is i64 in this case.

I do like how much more explicit the new formualtion is

jayzhan211 · 2025-02-07T01:55:39Z

datafusion/functions/src/string/repeat.rs

+                    // Accept all integer types but cast them to i64
+                    Coercion {
+                        desired_type: TypeSignatureClass::Native(logical_int64()),
+                        allowed_casts: vec![TypeSignatureClass::Integer],


Without this, i32 is rejected.

jayzhan211 · 2025-02-07T02:00:08Z

datafusion/functions/src/string/ascii.rs

+            signature: Signature::coercible_v2(
+                vec![Coercion {
+                    desired_type: TypeSignatureClass::Native(logical_string()),
+                    allowed_casts: vec![TypeSignatureClass::Native(logical_binary())],


Coercing binary to string is now easily customizable

Signed-off-by: Jay Zhan <[email protected]>

alamb · 2025-02-07T19:17:17Z

I will try and review this carefully over the weekend

Maybe @shehabgamin has some time to take a look too

shehabgamin · 2025-02-08T08:25:54Z

I will try and review this carefully over the weekend

Maybe @shehabgamin has some time to take a look too

I will review over the weekend as well!

alamb · 2025-02-14T16:54:34Z

I took the liberty of merging up from main to get the CI to pass again.

I hope to spend a bit longer today messing around with potential changes to Coercion for your consideration

alamb · 2025-02-14T16:56:19Z

Modifying Signature::Coercible alone won't achieve what @shehabgamin wants because, regardless of the approach, they would still need to:

Fork the entire function

Modify its signature

Register it in the function map

I was hoping it might be possible to make a 'wrapper' function that only overrides the signature but passes everything else in

Though now that I type this, perhaps what is really needed is user defined coercion for operators (like = or other comparisons 🤔 )

alamb · 2025-02-14T16:57:09Z

To support int->string in coercion,

This change is all you need

                    Coercion::new_exact(TypeSignatureClass::Native(logical_string())),

                    Coercion::new_implicit(TypeSignatureClass::Native(logical_string()), vec![TypeSignatureClass::Integer], NativeType::String),

Maybe we could make this a builder style API too -- I hope to mess around with it later todya

shehabgamin · 2025-02-15T02:33:30Z

@jayzhan211 I will re-review by tomorrow EOD, exciting progress!

alamb · 2025-02-15T10:03:54Z

I ran out of time yesterday and I am out next week so I am not likely to be able to make much progress here.

Sorry about that -- maybe one of you will have a chance to do something

goldmedal

Thanks for working on this, @jayzhan211. The new behavior of coercion signature makes sense to me.

I found I can't access Coercion and ImplicitCoercion outside the crate.
I believe we need to publicly use them in datafusion/expr/src/lib.rs.
Maybe we can just set it as below for all struct in signature.

pub use datafusion_expr_common::signature::*;

datafusion/expr-common/src/signature.rs

jayzhan211 · 2025-02-16T01:20:54Z

Thanks for working on this, @jayzhan211. The new behavior of coercion signature makes sense to me.

I found I can't access Coercion and ImplicitCoercion outside the crate. I believe we need to publicly use them in datafusion/expr/src/lib.rs. Maybe we can just set it as below for all struct in signature.
pub use datafusion_expr_common::signature::*;

If you can access Signature, no reason you can't access for Coercion. I think you should import datafuison-expr-common instead since Signature is re-exported in datafusion-expr for backward compatibility, but not for Coercion and ImplicitCoercion

goldmedal · 2025-02-16T04:03:04Z

If you can access Signature, no reason you can't access for Coercion. I think you should import datafuison-expr-common instead since Signature is re-exported in datafusion-expr for backward compatibility, but not for Coercion and ImplicitCoercion

I see. I tried to use Coercion in the example crate that only uses the datafusion dependency.
So, I can access Siganture by use datafusion::logical_expr::Signature because logical_expr is exported from datafusion-expr.
I found datafuison-expr-common hasn't been re-exported in datafusion. Maybe we can re-export it. Just like what we did for datafusion_physical_expr_common.

datafusion/datafusion/core/src/lib.rs

Line 751 in 587277f

pub mod physical_expr_common {

pub mod logical_expr_common {
    pub use datafusion_expr_common::*;
}

Then, user can only uses datafusion dependency without datafusion-expr-common. WDYT?

goldmedal

Thanks @jayzhan211. It looks great to me.

I was hoping it might be possible to make a 'wrapper' function that only overrides the signature but passes everything else in

@alamb's idea looks great, but I think it may be another issue 🤔 Maybe we can focus on the behavior of coercible signature improvement in this PR.

shehabgamin

@jayzhan211 Well done!!! 🔥

My approval doesn't actually do anything, but I approved it anyway lol.

Side note: Is this logic from the old PR needed?

datafusion/datafusion/expr/src/type_coercion/functions.rs

Lines 610 to 626 in 1d94e5c

    
           // Following the behavior of `TypeSignature::String`, we find the common string type. 
        
           let string_indices: Vec<_> = target_types.iter().enumerate() 
        
               .filter(|(_, t)| { 
        
                   matches!(t, TypeSignatureClass::Native(n) if n.native() == &NativeType::String) 
        
               }) 
        
               .map(|(i, _)| i) 
        
               .collect(); 
        
           if !string_indices.is_empty() { 
        
               let mut coerced_string_type = new_types[string_indices[0]].to_owned(); 
        
               for &i in string_indices.iter().skip(1) { 
        
                   coerced_string_type = 
        
                       find_common_string_type(&coerced_string_type, &new_types[i])?; 
        
               } 
        
               for i in string_indices { 
        
                   new_types[i] = coerced_string_type.clone(); 
        
               } 
        
           }

jayzhan211 · 2025-02-17T00:57:31Z

Side note: Is this logic from the old PR needed?

Probably not

alamb · 2025-02-17T11:56:01Z

Thanks @jayzhan211 @shehabgamin and @goldmedal 🚀

jayzhan211 · 2025-02-17T13:18:26Z

Thanks @alamb @shehabgamin @goldmedal

findepi · 2025-02-18T10:29:21Z

datafusion/expr-common/src/signature.rs

    /// Target coerce types in order
-    pub fn coercible(
-        target_types: Vec<TypeSignatureClass>,


Is this a breaking change?

Yes, label is added already

findepi · 2025-02-18T10:37:33Z

datafusion/expr-common/src/signature.rs

+    /// Coercion that only accepts arguments exactly matching the desired type.
+    Exact {
+        /// The required type for the argument
+        desired_type: TypeSignatureClass,
+    },
+
+    /// Coercion that accepts the desired type and can implicitly coerce from other types.
+    Implicit {
+        /// The primary desired type for the argument
+        desired_type: TypeSignatureClass,
+        /// Rules for implicit coercion from other types
+        implicit_coercion: ImplicitCoercion,
+    },


An implementor of a function `foo(s) wants to say:

foo(s) accepts s being "a string" (eg via logical type NativeType::String or old school DataType::Utf8)

so the query SELECT foo(s) should succeed when s is a string or s is coercible to string.

Neither Coercion option gives that.
In theory Coercion::Implicit gives that, but alas, it requires the function implementor to know the coercion rules of the engine, to enumerate the "allowed source types".

How is it supposed to be used?

I don't quite understand the question.

so the query SELECT foo(s) should succeed when s is a string or s is coercible to string.

This is true for this signature. For exact, string succeed, for implicit, string and other defined allow source types succeed

Great, but the burden to find "allowed source types" is on the function implementor. This is backwards.

to find "allowed source types" is on the function implementor.

This is what we need, we don't have single rule that fits all the use case

I think this structure (Coercion as an enum) allows for introducing another variant if we need additional rules / ways to express coercion.

So in my mind this is a step forward, even if we still have more work to do

Let's consider example of substr(s, i) function.
The call to substr should succeed for i being any integer type coercible to UInt64 or Int64.

You’ve defined that the second argument of substr can be any integer type coercible to Int64. Isn’t this part of the function definition? By doing so, the function knows what coercion is needed. However, it's not enough to just make this definition possible. If we want to allow coercion from a string integer type or if we only expect coercion to Int32, those options should be possible as well. Given this, we can't decide how coercion should happen without being informed by the user or the function definition

For example, integer types should be coercible to broader integer types the same way regardless whether it's in context of UNION ALL, EXCEPT, a function call, or an operator.

For internal DataFusion use cases, this rule works well. However, from an extensibility perspective, restricting coercion rules reduces flexibility. What if they don't want broader integer type for some specific workflow because they know the max value is in i32? We should prioritize maximum flexibility while also providing built-in options for general use cases to ensure ease of use.

#14296

If we begin allowing user-defined coercion rules for other operations, not just functions, users would need to define coercion rules for operations like binary comparisons as well. In other hand, what you are suggesting could limit the extensibility of coercion rules within DataFusion.

However, while we aim to provide extensibility, basic rules—such as converting an integer to a broader integer type—can be implemented as default rules in DataFusion, as long as they align well with the internal logic of the system.

It seems to me like we are perhaps not talking about the same things here.

Maybe someone can write up some example / test case that shows what feature is lacking. I suspect that will get us to a resolution the quickest

It is not a step forward. We removed ability for a function f(s) to express the most basic need: "f takes s being a type X, so the call succeeds for any type X' coercible to X"

This is, i believe, what the TypeSignature::Coercible(logical type) was doing. (At least this is what it should be doing by the name of it.)

@findepi This was the behavior before DataFusion 43. Starting in DataFusion 43, this was no longer the case. My old PR #14268 attempted to fix this regression. I agree that the naming does not match the behavior, and there is an extensive discussion in the PR I linked regarding that.

@jayzhan211 @alamb @findepi The relevant comment from my old PR: #14268 (comment).

With that in mind, this PR is still a significant step forward from the new behavior introduced in DataFusion 43. However, more work is needed to define functions that are explicitly documented, have clear and consistent naming, and sufficiently meet internal expectations while respecting system contracts and remaining flexible for downstream use cases.

It is not a step forward. We removed ability for a function f(s) to express the most basic need: "f takes s being a type X, so the call succeeds for any type X' coercible to X"
This is, i believe, what the TypeSignature::Coercible(logical type) was doing. (At least this is what it should be doing by the name of it.)

@findepi This was the behavior before DataFusion 43. Starting in DataFusion 43, this was no longer the case. My old PR #14268 attempted to fix this regression. I agree that the naming does not match the behavior, and there is an extensive discussion in the PR I linked regarding that.

@jayzhan211 @alamb @findepi The relevant comment from my old PR: #14268 (comment).

With that in mind, this PR is still a significant step forward from the new behavior introduced in DataFusion 43. However, more work is needed to define functions that are explicitly documented, have clear and consistent naming, and sufficiently meet internal expectations while respecting system contracts and remaining flexible for downstream use cases.

THanks @shehabgamin -- the writeup makes sense to me at a high level but I can't fully understand the practical implications / usecase.

In the past when we have had such issues, one thing we have done is to write up an example / usecase in https://github.com/apache/datafusion/tree/main/datafusion-examples that tries to show what is needed.

Perhaps in this case, someone could add an example to https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/advanced_udf.rs that shows how to create a function with the desired coercion behavior (or if that is not possible, shows why not)

I think that would make the principles of the #14268 (comment) clearer, at least to me, and we would have a specific example to evaluate potential additional improvements against

findepi · 2025-02-18T10:39:14Z

datafusion/functions/src/datetime/date_part.rs

@@ -95,24 +96,29 @@ impl DatePartFunc {
            signature: Signature::one_of(
                vec![
                    TypeSignature::Coercible(vec![
-                        TypeSignatureClass::Native(logical_string()),
-                        TypeSignatureClass::Timestamp,
+                        Coercion::new_exact(TypeSignatureClass::Native(logical_string())),


the function used to accept anything coercible to "logical string"
now it accepts anything that is "logical string"

Is this a breaking change?

the function used to accept anything coercible to "logical string"

This is actually incorrect so we fix this

See #14268 for what is happening

See #14268 for what is happening

Is #14268 still relevant? I'm not sure what I should be doing with that PR after this.

the function used to accept anything coercible to "logical string" now it accepts anything that is "logical string"

Is this a breaking change?

Anything coercible to "logical string" was behavior before DataFusion 43. Starting at DataFusion 43 the behavior changed to anything that is "logical string". #14268 attempted to change this back to anything coercible to "logical string", although I'm not sure if anything in there is relevant anymore after this PR.

github-actions bot added logical-expr Logical plan and expressions functions labels Feb 3, 2025

jayzhan211 commented Feb 3, 2025

View reviewed changes

jayzhan211 mentioned this pull request Feb 3, 2025

Fix Type Coercion for UDF Arguments #14268

Closed

jayzhan211 force-pushed the signature-v2 branch from 23953dd to 806c6a6 Compare February 6, 2025 13:46

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

jayzhan211 marked this pull request as draft February 7, 2025 00:38

jayzhan211 added 7 commits February 7, 2025 08:38

coerciblev2

386c7ed

Signed-off-by: Jay Zhan <[email protected]>

repeat

579ffef

Signed-off-by: Jay Zhan <[email protected]>

fix possible types

104da43

replace all coerciblev1

aae48ff

cleanup

f585136

remove specialize logic

bad7348

comment

c99e986

jayzhan211 force-pushed the signature-v2 branch from 806c6a6 to c99e986 Compare February 7, 2025 01:49

github-actions bot added the common Related to common crate label Feb 7, 2025

jayzhan211 changed the title ~~Draft: coercible signature~~ Signature::Coercible with user defined implicit casting Feb 7, 2025

jayzhan211 commented Feb 7, 2025

View reviewed changes

jayzhan211 added 2 commits February 7, 2025 10:15

err msg

07f97d0

ci escape

da84394

jayzhan211 marked this pull request as ready for review February 7, 2025 03:57

jayzhan211 added 3 commits February 7, 2025 20:23

rm coerciblev1

e0a889e

Signed-off-by: Jay Zhan <[email protected]>

fmt

7a78a6d

rename

2f8c2ad

jayzhan211 added the api change Changes the API exposed to users of the crate label Feb 7, 2025

jayzhan211 requested a review from alamb February 7, 2025 13:04

jayzhan211 and others added 3 commits February 14, 2025 21:23

Merge branch 'main' of github.com:apache/datafusion into signature-v2

6ce2c8c

fmt

56838b7

Merge remote-tracking branch 'apache/main' into signature-v2

6ceb9a2

goldmedal reviewed Feb 15, 2025

View reviewed changes

datafusion/expr-common/src/signature.rs Show resolved Hide resolved

datafusion/expr-common/src/signature.rs Show resolved Hide resolved

alamb mentioned this pull request Feb 15, 2025

feat: Add ScalarUDF support in FFI crate #14579

Merged

3 tasks

jayzhan211 added 2 commits February 16, 2025 09:46

doc

055d8d0

upd doc

be575e3

goldmedal approved these changes Feb 16, 2025

View reviewed changes

shehabgamin approved these changes Feb 16, 2025

View reviewed changes

goldmedal mentioned this pull request Feb 16, 2025

Minor: Re-export datafusion_expr_common crate #14696

Merged

alamb mentioned this pull request Feb 16, 2025

alamb's review queue #14698

Open

jayzhan211 merged commit 71f9d0c into apache:main Feb 17, 2025
26 checks passed

jayzhan211 deleted the signature-v2 branch February 17, 2025 13:18

findepi reviewed Feb 18, 2025

View reviewed changes

	// Following the behavior of `TypeSignature::String`, we find the common string type.
	let string_indices: Vec<_> = target_types.iter().enumerate()
	.filter(\|(_, t)\| {
	matches!(t, TypeSignatureClass::Native(n) if n.native() == &NativeType::String)
	})
	.map(\|(i, _)\| i)
	.collect();
	if !string_indices.is_empty() {
	let mut coerced_string_type = new_types[string_indices[0]].to_owned();
	for &i in string_indices.iter().skip(1) {
	coerced_string_type =
	find_common_string_type(&coerced_string_type, &new_types[i])?;
	}
	for i in string_indices {
	new_types[i] = coerced_string_type.clone();
	}
	}

Signature::Coercible with user defined implicit casting #14440

Signature::Coercible with user defined implicit casting #14440

Conversation

jayzhan211 commented Feb 3, 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

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

alamb commented Feb 7, 2025

shehabgamin commented Feb 8, 2025

alamb commented Feb 14, 2025

alamb commented Feb 14, 2025

alamb commented Feb 14, 2025

shehabgamin commented Feb 15, 2025

alamb commented Feb 15, 2025

goldmedal left a comment

Choose a reason for hiding this comment

jayzhan211 commented Feb 16, 2025

goldmedal commented Feb 16, 2025

goldmedal left a comment

Choose a reason for hiding this comment

shehabgamin left a comment • edited Loading

Choose a reason for hiding this comment

jayzhan211 commented Feb 17, 2025

alamb commented Feb 17, 2025

jayzhan211 commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Feb 18, 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 Feb 19, 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 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

shehabgamin Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

shehabgamin Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

jayzhan211 commented Feb 3, 2025 •

edited

Loading

shehabgamin left a comment •

edited

Loading

jayzhan211 Feb 18, 2025 •

edited

Loading

jayzhan211 Feb 19, 2025 •

edited

Loading

jayzhan211 Feb 18, 2025 •

edited

Loading

shehabgamin Feb 18, 2025 •

edited

Loading

shehabgamin Feb 18, 2025 •

edited

Loading