Fix `Coalesce` casting logic to follows what Postgres and DuckDB do. Introduce signature that do non-comparison coercion #10268

jayzhan211 · 2024-04-27T23:42:06Z

Which issue does this PR close?

Closes #10261
Closes #10241
Part of #10507

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

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

datafusion/expr/src/signature.rs

@@ -92,14 +92,19 @@ pub enum TypeSignature {
    /// A function such as `concat` is `Variadic(vec![DataType::Utf8, DataType::LargeUtf8])`
    Variadic(Vec<DataType>),
    /// One or more arguments of an arbitrary but equal type.
-    /// DataFusion attempts to coerce all argument types to match the first argument's type
+    /// DataFusion attempts to coerce all argument types to match to the common type with comparision coercion.
    ///
    /// # Examples
    /// Given types in signature should be coercible to the same final type.
    /// A function such as `make_array` is `VariadicEqual`.
    ///
    /// `make_array(i32, i64) -> make_array(i64, i64)`
    VariadicEqual,


datafusion/expr/src/signature.rs

jayzhan211 · 2024-04-27T23:47:35Z

datafusion/expr/src/type_coercion/functions.rs

-        // Note that not all rules in `comparison_coercion` can be reused here.
-        // For example, all numeric types can be coerced into Utf8 for comparison,
-        // but not for function arguments.
-        _ => comparison_binary_numeric_coercion(type_into, type_from).and_then(


This logic is introduced in #9459, so I think it is safe to remove together with this PR

datafusion/sqllogictest/test_files/functions.slt

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

datafusion/expr/src/type_coercion/functions.rs

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

alamb · 2024-05-21T20:20:42Z

In general I am concerned about the potential downstream effects of this change. I don't fully understand them

What I would ideally like to do is to run the influxdb_iox regression tests with this change and see what happens.

I am not sure I have the time to do that in the next week -- maybe @appletreeisyellow does 🤔

alamb · 2024-05-21T20:21:26Z

Basically I worry this is just changing behavior rather than fixing a bug and will result in churn for no benefit downstream. I may be mis understanding the change and rationale however

jayzhan211 · 2024-05-21T23:42:39Z

I believe the coercion rule is quite messy as it currently stands. It would be more understandable and maintainable to move the coercion rule from coerced_from to each individual function. This way, it would be clear which coercion rule applies to each function or signature. Having a single function handle multiple coercion rules makes the code harder to reason about and could cause downstream issues, as you have pointed out. I've also filed an issue to track this at @10507.

Would it be better to start by removing the rule from coerce_from and moving it to the specific function or signature?

appletreeisyellow · 2024-05-22T04:09:37Z

I am not sure I have the time to do that in the next week -- maybe @appletreeisyellow does 🤔

@alamb I'm happy to coordinate with our performance team and run an influxdb_iox regression tests if needed

alamb · 2024-05-22T15:40:39Z

I don't think this needs a performance test -- I was thinking just the main test suite

alamb · 2024-05-22T15:41:09Z

I believe the coercion rule is quite messy as it currently stands. It would be more understandable and maintainable to move the coercion rule from coerced_from to each individual function. This way, it would be clear which coercion rule applies to each function or signature. Having a single function handle multiple coercion rules makes the code harder to reason about and could cause downstream issues, as you have pointed out. I've also filed an issue to track this at @10507.

Would it be better to start by removing the rule from coerce_from and moving it to the specific function or signature?

Hi @jayzhan211 -- I am sorry I haven't had a chance to devote more time to this and review. I will try and find some time in the next few days

appletreeisyellow · 2024-05-22T17:06:01Z

I don't think this needs a performance test -- I was thinking just the main test suite

I was thinking performance regression 😅 Got it! I can put up an Influxdata test against this branch, sometime this afternoon

alamb

Update here is that @appletreeisyellow did run the internal influx db tests against this branch and we didn't find any problems. Not that it is required that we don't have to change our code, but I have found that our test suite has a bit broader coverage and thus the fact it doesn't have problems suggests to me this PR is less likely to have knock on effects

I had another look at this PR this morning and Other than the changes in select.slt I think it looks great to me

Thanks again @jayzhan211

alamb · 2024-05-25T10:04:59Z

datafusion/sqllogictest/test_files/select.slt

@@ -1473,7 +1473,7 @@ DROP TABLE t;

 # related to https://github.com/apache/datafusion/issues/8814
 statement ok
-create table t(x int, y int) as values (1,1), (2,2), (3,3), (0,0), (4,0);
+create table t(x bigint, y bigint) as values (1,1), (2,2), (3,3), (0,0), (4,0);


Could you please keep the existing test and add a new test that uses bigint so it is clearer what behavior is changing?

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

alamb · 2024-05-25T13:09:56Z

datafusion/sqllogictest/test_files/joins.slt

@@ -1778,7 +1778,7 @@ AS VALUES
 ('BB', 6, 1);

 query TII
-select col1, col2, coalesce(sum_col3, 0) as sum_col3
+select col1, col2, coalesce(sum_col3, arrow_cast(0, 'UInt64')) as sum_col3


maybe also we can change this test back too

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

alamb · 2024-05-26T10:04:00Z

Looks good, and existing tests are passing 👍 🚀

jayzhan211 · 2024-05-26T12:26:05Z

Thanks @alamb and @appletreeisyellow

…Introduce signature that do non-comparison coercion (apache#10268) * remove casting for coalesce Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * crate only visibility Signed-off-by: jayzhan211 <[email protected]> * polish comment Signed-off-by: jayzhan211 <[email protected]> * improve test Signed-off-by: jayzhan211 <[email protected]> * backup Signed-off-by: jayzhan211 <[email protected]> * introduce new signautre for coalesce Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * ignore err msg Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * fix doc Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * switch to type_resolution coercion Signed-off-by: jayzhan211 <[email protected]> * fix i64 and u64 case Signed-off-by: jayzhan211 <[email protected]> * add more tests Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * add null case Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * fix Signed-off-by: jayzhan211 <[email protected]> * rename to type_union_resolution Signed-off-by: jayzhan211 <[email protected]> * add comment Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * fix test Signed-off-by: jayzhan211 <[email protected]> * add comment Signed-off-by: jayzhan211 <[email protected]> * rm test Signed-off-by: jayzhan211 <[email protected]> * cleanup since rebase Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * fix msg Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * rm pure_string_coercion Signed-off-by: jayzhan211 <[email protected]> * rm duplicate Signed-off-by: jayzhan211 <[email protected]> * change type in select.slt Signed-off-by: jayzhan211 <[email protected]> * fix slt Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]>

alamb · 2024-05-27T12:23:19Z

Thank you for sticking with it @jayzhan211 -- so much is going on!@

…Introduce signature that do non-comparison coercion (apache#10268) * remove casting for coalesce Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * crate only visibility Signed-off-by: jayzhan211 <[email protected]> * polish comment Signed-off-by: jayzhan211 <[email protected]> * improve test Signed-off-by: jayzhan211 <[email protected]> * backup Signed-off-by: jayzhan211 <[email protected]> * introduce new signautre for coalesce Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * ignore err msg Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * fix doc Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * switch to type_resolution coercion Signed-off-by: jayzhan211 <[email protected]> * fix i64 and u64 case Signed-off-by: jayzhan211 <[email protected]> * add more tests Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * add null case Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * fix Signed-off-by: jayzhan211 <[email protected]> * rename to type_union_resolution Signed-off-by: jayzhan211 <[email protected]> * add comment Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * fix test Signed-off-by: jayzhan211 <[email protected]> * add comment Signed-off-by: jayzhan211 <[email protected]> * rm test Signed-off-by: jayzhan211 <[email protected]> * cleanup since rebase Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * add more test Signed-off-by: jayzhan211 <[email protected]> * fix msg Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * rm pure_string_coercion Signed-off-by: jayzhan211 <[email protected]> * rm duplicate Signed-off-by: jayzhan211 <[email protected]> * change type in select.slt Signed-off-by: jayzhan211 <[email protected]> * fix slt Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 added 5 commits April 27, 2024 23:04

remove casting for coalesce

c79156f

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

add more test

a36e6b2

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

add more test

bf16c92

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

crate only visibility

407e3c7

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

polish comment

03b9162

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

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Apr 27, 2024

jayzhan211 commented Apr 27, 2024

View reviewed changes

datafusion/expr/src/signature.rs Outdated Show resolved Hide resolved

jayzhan211 commented Apr 27, 2024

View reviewed changes

datafusion/sqllogictest/test_files/functions.slt Outdated Show resolved Hide resolved

improve test

4abf29d

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

jayzhan211 closed this Apr 28, 2024

jayzhan211 added 3 commits April 28, 2024 12:17

backup

4965e8d

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

introduce new signautre for coalesce

81f0235

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

cleanup

bae996c

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

jayzhan211 reopened this Apr 28, 2024

github-actions bot added physical-expr Physical Expressions optimizer Optimizer rules labels Apr 28, 2024

jayzhan211 changed the title ~~Avoid casting for coalesce, introduce signature VariadicEqualOrNull for coalesce.~~ Fix Coalesce casting logic that follows Postgres and DuckDB. Introduce signature that do non-comparison coercion Apr 28, 2024

cleanup

ddf9b1c

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

github-actions bot removed the optimizer Optimizer rules label Apr 28, 2024

jayzhan211 added 3 commits April 28, 2024 14:04

ignore err msg

c2799ea

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

fmt

2574896

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

fix doc

6a17e57

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

jayzhan211 commented Apr 28, 2024

View reviewed changes

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

cleanup

4cba8c5

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

github-actions bot removed the physical-expr Physical Expressions label Apr 28, 2024

jayzhan211 added 2 commits April 28, 2024 15:25

add more test

f1cfb8d

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

switch to type_resolution coercion

d2e83d3

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

jayzhan211 marked this pull request as draft May 14, 2024 08:23

jayzhan211 marked this pull request as ready for review May 14, 2024 11:27

jayzhan211 marked this pull request as draft May 14, 2024 11:31

jayzhan211 added 5 commits May 14, 2024 19:37

Merge remote-tracking branch 'upstream/main' into fix-coelesce

6a8fe6f

Merge remote-tracking branch 'upstream/main' into fix-coelesce

20e618e

fmt

4153593

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

rm pure_string_coercion

030a519

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

rm duplicate

5b797d5

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

jayzhan211 marked this pull request as ready for review May 14, 2024 13:18

jayzhan211 mentioned this pull request May 14, 2024

Strengthen TypeSignature and Coercion rule. #10507

Open

alamb approved these changes May 25, 2024

View reviewed changes

change type in select.slt

b954479

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

alamb reviewed May 25, 2024

View reviewed changes

fix slt

829b5a2

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

alamb approved these changes May 26, 2024

View reviewed changes

alamb merged commit 0836500 into apache:main May 26, 2024
23 checks passed

alamb mentioned this pull request May 26, 2024

Fix compilation "comparison_binary_numeric_coercion not found" #10677

Merged

jayzhan211 mentioned this pull request Oct 4, 2024

Apply type_union_resolution to array and values #12753

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `Coalesce` casting logic to follows what Postgres and DuckDB do. Introduce signature that do non-comparison coercion #10268

Fix `Coalesce` casting logic to follows what Postgres and DuckDB do. Introduce signature that do non-comparison coercion #10268

jayzhan211 commented Apr 27, 2024 •

edited

Loading

This comment was marked as outdated.

jayzhan211 Apr 27, 2024

alamb May 1, 2024

alamb commented May 21, 2024

alamb commented May 21, 2024

jayzhan211 commented May 21, 2024

appletreeisyellow commented May 22, 2024

alamb commented May 22, 2024

alamb commented May 22, 2024

appletreeisyellow commented May 22, 2024 •

edited

Loading

alamb left a comment

alamb May 25, 2024

alamb May 25, 2024

alamb commented May 26, 2024

jayzhan211 commented May 26, 2024

alamb commented May 27, 2024

Fix Coalesce casting logic to follows what Postgres and DuckDB do. Introduce signature that do non-comparison coercion #10268

Fix Coalesce casting logic to follows what Postgres and DuckDB do. Introduce signature that do non-comparison coercion #10268

Conversation

jayzhan211 commented Apr 27, 2024 • 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?

This comment was marked as outdated.

jayzhan211 Apr 27, 2024

Choose a reason for hiding this comment

alamb May 1, 2024

Choose a reason for hiding this comment

alamb commented May 21, 2024

alamb commented May 21, 2024

jayzhan211 commented May 21, 2024

appletreeisyellow commented May 22, 2024

alamb commented May 22, 2024

alamb commented May 22, 2024

appletreeisyellow commented May 22, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb May 25, 2024

Choose a reason for hiding this comment

alamb May 25, 2024

Choose a reason for hiding this comment

alamb commented May 26, 2024

jayzhan211 commented May 26, 2024

alamb commented May 27, 2024

Fix `Coalesce` casting logic to follows what Postgres and DuckDB do. Introduce signature that do non-comparison coercion #10268

Fix `Coalesce` casting logic to follows what Postgres and DuckDB do. Introduce signature that do non-comparison coercion #10268

jayzhan211 commented Apr 27, 2024 •

edited

Loading

appletreeisyellow commented May 22, 2024 •

edited

Loading