Introduce TypeSignature::Comparable and update `NullIf` signature by jayzhan211 · Pull Request #13356 · apache/datafusion

jayzhan211 · 2024-11-11T12:40:48Z

Which issue does this PR close?

Part of #13301

Closes #.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

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

jayzhan211 · 2024-11-13T00:52:07Z

@Omega359 ptal

Omega359

I have concerns about the signature change causing queries that used to work to now fail. I am not certain that the signature change for nullif is for the better for that reason.

With respect to the TypeSignature::Boolean I left some comments.

Omega359 · 2024-11-13T01:23:43Z

datafusion/expr/src/type_coercion/functions.rs

            return Ok(t);
        }
    } else {
+        // TODO: Deprecate this branch after all signatures are well-supported (aka coercion are happend already)


Suggested change

// TODO: Deprecate this branch after all signatures are well-supported (aka coercion are happend already)

// TODO: Deprecate this branch after all signatures are well-supported (aka coercion has happened already)

Omega359 · 2024-11-13T01:24:09Z

datafusion/expr/src/type_coercion/functions.rs

+        TypeSignature::Boolean(number) => {
+            function_length_check(current_types.len(), *number)?;
+
+            // Find common boolean type amongs given types


Suggested change

// Find common boolean type amongs given types

// Find common boolean type amongst the given types

Omega359 · 2024-11-13T01:29:54Z

datafusion/expr/src/type_coercion/functions.rs

+            function_length_check(current_types.len(), *number)?;
+
+            // Find common boolean type amongs given types
+            let mut valid_type = current_types.first().unwrap().to_owned();


Unless I'm mistaken there is only one possible valid type here - Boolean doesn't have multiple types, does it? If so, I don't see the need for this variable nor the code below the for loop. valid_type must be DataType::Boolean, no?

This is to ensure the given types are all boolean, if it isn't we should return error here. If we got null, we return boolean

Omega359 · 2024-11-13T01:30:19Z

datafusion/functions/src/core/nullif.rs

-                2,
-                SUPPORTED_NULLIF_TYPES.to_vec(),
+            signature: Signature::one_of(
+                // Hack: String is at the beginning so the return type is String if both args are Nulls


Not sure I'd call that a hack tbh

Omega359 · 2024-11-13T01:34:19Z

datafusion/sqllogictest/test_files/nullif.slt

+----
+1
+
+query error DataFusion error: Error during planning: Internal error: Failed to match any signature, errors: Error during planning: The signature expected NativeType::String but received NativeType::Int64


This used to work, I just ran this locally against v43. I can't see a reason why this should no longer be supported.

select nullif(2, 'a');

This query failed in Postgres, I think we should return error for this query

Omega359 · 2024-11-13T01:35:07Z

datafusion/sqllogictest/test_files/nullif.slt

+# TODO: support numeric string
+# This query success in Postgres and DuckDB
+query error DataFusion error: Error during planning: Internal error: Failed to match any signature, errors: Error during planning: The signature expected NativeType::String but received NativeType::Int64
+select nullif(2, '1');


This also used to work.

query T select nullif(2, '1'); ---- 2

Interesting that the type is text vs number but still, it did work.

This is quite a hard issue to fix, since we can't tell the difference between Unknown type and String type currently

This query pass in main too but it should not.

query T select nullif('1'::varchar, 2); ---- 1

The change did have regression on nullif(2, '1'), but also fix nullif('1'::varchar, 2)

Similar issue in #13240 (comment)

I don't have strong opinions against this PR because it fixes nullif('1'::varchar, 2) and refines the TypeSignature. We could file a ticket for it and address it in a follow-up.

Tracked in #13285

Duckdb can run this query

D select nullif('2'::varchar, 2); ┌─────────────────────────────────┐ │ nullif(CAST('2' AS VARCHAR), 2) │ │ varchar │ ├─────────────────────────────────┤ │ │ └─────────────────────────────────┘

😕

To me what is important is not strictly what other systems support - those may/could be a guide to what datafusion will support - but whether the provided arguments to a signature can be losslessly converted to what the signature accepts and whether it logically makes sense to do so.

I personally would rather be lenient for what is accepted and do casting/coercion as required than to be strict and push the onus onto the user to do that. That's just me though, I don't know if that is the general consensus of the community. Perhaps we should file a discussion ticket with the options and decide?

I just double checked on this branch and it seems good to me (supports things reasonably)

Running `target/debug/datafusion-cli` DataFusion CLI v43.0.0 > select nullif(2, '1'); +----------------------------+ | nullif(Int64(2),Utf8("1")) | +----------------------------+ | 2 | +----------------------------+ 1 row(s) fetched. Elapsed 0.027 seconds. > select nullif('1'::varchar, 2); +----------------------------+ | nullif(Utf8("1"),Int64(2)) | +----------------------------+ | 1 | +----------------------------+ 1 row(s) fetched. Elapsed 0.007 seconds.

Signed-off-by: jayzhan211 <[email protected]>

Weijun-H · 2024-11-13T12:20:42Z

datafusion/expr-common/src/signature.rs

+            TypeSignature::String(arg_count) => get_data_types(&NativeType::String)
+                .into_iter()
+                .map(|dt| vec![dt; *arg_count])
+                .collect::<Vec<_>>(),


Weijun-H · 2024-11-15T03:15:29Z

datafusion/sqllogictest/test_files/nullif.slt

+# TODO: support numeric string
+# This query success in Postgres and DuckDB
+query error DataFusion error: Error during planning: Internal error: Failed to match any signature, errors: Error during planning: The signature expected NativeType::String but received NativeType::Int64
+select nullif(2, '1');


I don't have strong opinions against this PR because it fixes nullif('1'::varchar, 2) and refines the TypeSignature. We could file a ticket for it and address it in a follow-up.

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-11-15T09:09:09Z

I revert to the previous function's behaviour since it makes more sense to me

jayzhan211 · 2024-11-21T00:03:44Z

This is ready for review too

alamb

Thank you @jayzhan211 -- I think this looks like a nice improvement.

I have some suggestions on improving the documentation to be more explicit about what TypeSignature::Comparable means but I also think we could implement that as a follow on

THank you @Omega359 and @Weijun-H for the reviews

alamb · 2024-11-21T21:51:16Z

datafusion/expr/src/type_coercion/functions.rs

+            function_length_check(current_types.len(), *num)?;
+            let mut target_type = current_types[0].to_owned();
+            for data_type in current_types.iter().skip(1) {
+                if let Some(dt) = comparison_coercion_numeric(&target_type, data_type) {


It seems like TypeSignature::Comparable has different rules that, for example, = or < (it prefers numeric)

I think this is fine, but should probably be documented better / more explicitly as it might be expected

Also the coercion / defaulting to Utf8 might also be a good thing to document

alamb · 2024-11-21T21:53:58Z

datafusion/sqllogictest/test_files/nullif.slt

+# TODO: support numeric string
+# This query success in Postgres and DuckDB
+query error DataFusion error: Error during planning: Internal error: Failed to match any signature, errors: Error during planning: The signature expected NativeType::String but received NativeType::Int64
+select nullif(2, '1');


I just double checked on this branch and it seems good to me (supports things reasonably)

Running `target/debug/datafusion-cli` DataFusion CLI v43.0.0 > select nullif(2, '1'); +----------------------------+ | nullif(Int64(2),Utf8("1")) | +----------------------------+ | 2 | +----------------------------+ 1 row(s) fetched. Elapsed 0.027 seconds. > select nullif('1'::varchar, 2); +----------------------------+ | nullif(Utf8("1"),Int64(2)) | +----------------------------+ | 1 | +----------------------------+ 1 row(s) fetched. Elapsed 0.007 seconds.

alamb

Thank you @jayzhan211 -- I think this looks like a nice improvement.

I have some suggestions on improving the documentation to be more explicit about what TypeSignature::Comparable means but I also think we could implement that as a follow on

THank you @Omega359 and @Weijun-H for the reviews

alamb · 2024-11-21T21:55:08Z

datafusion/expr-common/src/signature.rs

    /// arguments like `vec![DataType::Int32]` or `vec![DataType::Float32]`
    /// since i32 and f32 can be casted to f64
    Coercible(Vec<LogicalTypeRef>),
+    /// The number of arguments that are comparable


Would it be possible to define what comparable means in this context?

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

jayzhan211 · 2024-11-22T03:20:16Z

Thanks @alamb @Omega359 @Weijun-H

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Nov 11, 2024

jayzhan211 added 2 commits November 11, 2024 20:44

simplify signature for nullif

c6ef8a5

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

add possible types

ffd02c4

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

jayzhan211 force-pushed the nullif branch from 0794802 to ffd02c4 Compare November 11, 2024 12:53

jayzhan211 marked this pull request as ready for review November 11, 2024 13:21

This was referenced Nov 13, 2024

Add utf8view support in nullif #13380

Merged

Add support for utf8view to nvl function #13382

Merged

Merge branch 'main' of github.com:apache/datafusion into nullif

784018f

jayzhan211 requested a review from Weijun-H November 13, 2024 00:51

Merge branch 'main' of github.com:apache/datafusion into nullif

ca87f2c

Omega359 reviewed Nov 13, 2024

View reviewed changes

jayzhan211 added the regression Something that used to work no longer does label Nov 13, 2024

typo

95f09c4

Signed-off-by: jayzhan211 <[email protected]>

Weijun-H approved these changes Nov 15, 2024

View reviewed changes

jayzhan211 marked this pull request as draft November 15, 2024 07:11

jayzhan211 marked this pull request as ready for review November 15, 2024 07:53

jayzhan211 marked this pull request as draft November 15, 2024 08:40

numeric string

60496e5

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 changed the title ~~Introduce TypeSignature::Boolean and update NullIf signature~~ Introduce TypeSignature::Comparable and update NullIf signature Nov 15, 2024

jayzhan211 removed the regression Something that used to work no longer does label Nov 15, 2024

jayzhan211 marked this pull request as ready for review November 15, 2024 09:45

jayzhan211 requested review from Omega359 and Weijun-H November 15, 2024 11:24

alamb approved these changes Nov 21, 2024

View reviewed changes

alamb reviewed Nov 21, 2024

View reviewed changes

jayzhan211 added 2 commits November 22, 2024 09:03

add doc for signature

8d0b73f

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

add doc for signature

e4316a2

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

jayzhan211 merged commit 2f150f6 into apache:main Nov 22, 2024

jayzhan211 deleted the nullif branch November 22, 2024 03:20

	// TODO: Deprecate this branch after all signatures are well-supported (aka coercion are happend already)
	// TODO: Deprecate this branch after all signatures are well-supported (aka coercion has happened already)

	// Find common boolean type amongs given types
	// Find common boolean type amongst the given types

Comments

Conversation

jayzhan211 commented Nov 11, 2024 • 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!

jayzhan211 commented Nov 13, 2024

Uh oh!

Omega359 left a comment

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!

Omega359 Nov 13, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Omega359 Nov 13, 2024 • 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!

jayzhan211 Nov 13, 2024 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Nov 15, 2024

Uh oh!

jayzhan211 commented Nov 21, 2024

Uh oh!

alamb left a comment

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!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Nov 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jayzhan211 commented Nov 11, 2024 •

edited

Loading

Omega359 Nov 13, 2024 •

edited

Loading

Omega359 Nov 13, 2024 •

edited

Loading

jayzhan211 Nov 13, 2024 •

edited

Loading