Introduce Boolean Coercion #8331

jayzhan211 · 2023-11-27T12:51:53Z

Which issue does this PR close?

Ref #8302

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb

Thank you @jayzhan211 -- this is looking pretty close. I left some suggestions -- let me know what you think

alamb · 2023-11-27T22:41:47Z

datafusion/expr/src/type_coercion/binary.rs

@@ -353,6 +354,20 @@ fn string_temporal_coercion(
    }
 }

+/// Coerce `Boolean` to other larger types, like Numeric as `1` or String as "1"
+fn bool_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {
+    match (lhs_type, rhs_type) {


I think this also needs to check when rhs_type is a DataType::Boolean as well.

I would expect both of the following queries to work and return the same thing

sDataFusion CLI v33.0.0 ❯ select 1 = true; Error during planning: Cannot infer common argument type for comparison operation Int64 = Boolean ❯ select true = 1; +--------------------------+ | Boolean(true) = Int64(1) | +--------------------------+ | true | +--------------------------+ 1 row in set. Query took 0.006 seconds.

alamb · 2023-11-27T22:41:54Z

datafusion/sqllogictest/test_files/array.slt

-select make_array(1, 2, 3), make_array(1.0, 2.0, 3.0), make_array('h', 'e', 'l', 'l', 'o');
----
-[1, 2, 3] [1.0, 2.0, 3.0] [h, e, l, l, o]
+query ??????


Can you also add some tests for other uses of this logic (not just in make_array) such as comparisons

I notice that postgres doesn't handle boolean coercion

postgres=# select true = 1; ERROR: operator does not exist: boolean = integer LINE 1: select true = 1; ^ HINT: No operator matches the given name and argument types. You might need to add explicit type casts.

However, after this PR datafusion does:

DataFusion CLI v33.0.0 ❯ select true = 1; +--------------------------+ | Boolean(true) = Int64(1) | +--------------------------+ | true | +--------------------------+ 1 row in set. Query took 0.006 seconds.

Duckdb support boolean coercion, we can consider follow it. If it does not break the overall design, I think we can support it too.

datafusion/sqllogictest/test_files/union.slt

alamb · 2023-11-28T14:57:33Z

datafusion/physical-expr/src/expressions/case.rs

@@ -828,7 +828,8 @@ mod tests {
            None,
            schema.as_ref(),
        );
-        assert!(expr.is_err());
+        // expr is ok since boolean coercion is supported


I think we lost the negative coverage here -- can. you please change the test back to checking that this is an error?

Perhaps you could change it to

CASE WHEN a = 'foo' THEN timestamp(500ns) WHEN a = 'bar' THEN true END

As bool --> timestamp coercion isn't supported

alamb

Thanks @jayzhan211 -- this is looking very close. I think the code looks 👨‍🍳 👌 and we just need a few more changes to the tests to avoid losing negative coverage

Thanks for pushing this

alamb · 2023-11-28T14:57:50Z

datafusion/sqllogictest/test_files/scalar.slt

+
+# bool_coercion
+query BBBB
+select 1 == true, false == 0, 1.0 == true, false == 'false';


datafusion/sqllogictest/test_files/union.slt

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

jayzhan211 · 2023-11-29T00:17:18Z

datafusion/physical-expr/src/expressions/case.rs

@@ -799,69 +799,6 @@ mod tests {
        Ok(batch)
    }

-    #[test]


move to slt

jayzhan211 · 2023-11-29T00:17:31Z

datafusion/sql/tests/sql_integration.rs

@@ -2190,18 +2190,6 @@ fn union_with_aliases() {
    quick_test(sql, expected);
 }

-#[test]
-fn union_with_incompatible_data_types() {


move to slt

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

alamb

Looks good to me -- thank you @jayzhan211

@Dandandan / @andygrove / @viirya any concerns with supporting coercion from bool to other types? It seems like a good feature to me

jackwener · 2023-12-03T14:43:48Z

Boolean Coercion is a dangerous behavior. It will cause some strange behavior.

such as:

If there is some implicit type cast:
1. true = 'true'
2. true = 1

But true = 1 = '1' is conflict with true = 'true'

I suggest to we'd better to keep same behavior with pg.

jayzhan211 · 2023-12-04T00:04:34Z

Boolean Coercion is a dangerous behavior. It will cause some strange behavior.

such as:
If there is some implicit type cast:
1. true = 'true'
2. true = 1

But true = 1 = '1' is conflict with true = 'true'
I suggest to we'd better to keep same behavior with pg.

I agree it causes some confusing behavior. true is coerced to 1 or 'true' depends on other type.

query ?
select make_array(true, 1, 'true');
----
[true, 1, true]

query ?
select make_array(true, 'true', 1);
----
[true, true, 1]

query ?
select make_array(true, 1);
----
[1, 1]

Then, why do DuckDB introduce this 🤔

Per jakevin, let's not merge this until we sort out if it is a good idea

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

alamb reviewed Nov 27, 2023

View reviewed changes

alamb mentioned this pull request Nov 27, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 27, 2023 #8329

Closed

8 tasks

jayzhan211 marked this pull request as draft November 27, 2023 23:42

jayzhan211 marked this pull request as ready for review November 28, 2023 00:34

jayzhan211 requested a review from alamb November 28, 2023 00:34

github-actions bot added the optimizer Optimizer rules label Nov 28, 2023

jayzhan211 marked this pull request as draft November 28, 2023 13:04

github-actions bot added the physical-expr Physical Expressions label Nov 28, 2023

jayzhan211 mentioned this pull request Nov 28, 2023

Move Coercion for MakeArray to coerce_arguments_for_signature and introduce another one for ArrayAppend #8317

Merged

github-actions bot added the sql SQL Planner label Nov 28, 2023

jayzhan211 commented Nov 28, 2023

View reviewed changes

datafusion/sqllogictest/test_files/union.slt Show resolved Hide resolved

jayzhan211 marked this pull request as ready for review November 28, 2023 14:25

alamb reviewed Nov 28, 2023

View reviewed changes

jayzhan211 marked this pull request as draft November 29, 2023 00:09

jayzhan211 added 7 commits November 29, 2023 08:13

bool coercion

9fd8f76

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

fmt

570e475

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

address comment

720e925

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

fix ci

aefb39c

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

fix ci

2af4259

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

move test to sqllogictest

7171bd6

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

cleanup test

c14820d

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

jayzhan211 force-pushed the bool-coercion branch from 0c0a4be to c14820d Compare November 29, 2023 00:16

jayzhan211 commented Nov 29, 2023

View reviewed changes

datafusion/physical-expr/src/expressions/case.rs

@@ -799,69 +799,6 @@ mod tests {

Ok(batch)

}

#[test]

Copy link

Contributor Author

jayzhan211 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to slt

jayzhan211 commented Nov 29, 2023

View reviewed changes

jayzhan211 added 2 commits November 29, 2023 08:32

fix ci, remve multiline error

268b69b

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

union bool with integer

4b0bfbc

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

This comment was marked as outdated.

Sign in to view

jayzhan211 marked this pull request as ready for review November 29, 2023 01:25

jayzhan211 requested a review from alamb November 29, 2023 11:21

jayzhan211 mentioned this pull request Nov 29, 2023

Improve type coercion and casting #8302

Open

alamb previously approved these changes Dec 1, 2023

View reviewed changes

alamb marked this pull request as draft December 5, 2023 20:47

jayzhan211 closed this Mar 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Boolean Coercion #8331

Introduce Boolean Coercion #8331

jayzhan211 commented Nov 27, 2023

alamb left a comment

alamb Nov 27, 2023

alamb Nov 27, 2023

jayzhan211 Nov 27, 2023

alamb Nov 28, 2023

alamb left a comment

alamb Nov 28, 2023

jayzhan211 Nov 29, 2023

jayzhan211 Nov 29, 2023

This comment was marked as outdated.

alamb left a comment

jackwener commented Dec 3, 2023 •

edited

Loading

jayzhan211 commented Dec 4, 2023 •

edited

Loading

Introduce Boolean Coercion #8331

Introduce Boolean Coercion #8331

Conversation

jayzhan211 commented Nov 27, 2023

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?

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 27, 2023

Choose a reason for hiding this comment

alamb Nov 27, 2023

Choose a reason for hiding this comment

jayzhan211 Nov 27, 2023

Choose a reason for hiding this comment

alamb Nov 28, 2023

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 28, 2023

Choose a reason for hiding this comment

jayzhan211 Nov 29, 2023

Choose a reason for hiding this comment

jayzhan211 Nov 29, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

alamb left a comment

Choose a reason for hiding this comment

jackwener commented Dec 3, 2023 • edited Loading

jayzhan211 commented Dec 4, 2023 • edited Loading

jackwener commented Dec 3, 2023 •

edited

Loading

jayzhan211 commented Dec 4, 2023 •

edited

Loading