Add proper support for `null` literal by introducing `ScalarValue::Null` by WinkerDu · Pull Request #2364 · apache/datafusion

WinkerDu · 2022-04-28T02:55:10Z

Which issue does this PR close?

Closes #2363 .

Rationale for this change

To solve Null constants issues listed in #1184 , and since /apache/arrow-rs#1572 Null casted from and to most of types in arrow-rs kernel, it's reasonable that introduce Null type to df for type coercion.

What changes are included in this PR?

Introduce ScalarValue::Null type to df

Are there any user-facing changes?

No.

WinkerDu · 2022-04-28T03:08:05Z

For someone reviews this pr,
since arrow = { version = "12" } doesn't contain patch /apache/arrow-rs#1572, thanks for @alamb suggestion, I create winkerdu/test_null_cast in my private repo like:

git checkout master -b winkerdu/test_null_cast
git reset --hard dbc47e030c3038d40dbef39fbf3b39ae41f9e98a                 # prepare for version 12.0.0 release
git cherry-pick b50cc737efa2a4f2b1e27c0f3da3bc0403c6a2b6                 # see https://github.com/apache/arrow-rs/pull/1572

I convert this pr to draft for code review. It's ok to go if new arrow-rs version release

WinkerDu · 2022-04-28T04:02:54Z

cc @alamb @andygrove @yjshen @xudong963 Please have a review, thank you

alamb

Looking very @WinkerDu 👍 I think this is a great step. I left some comments for your consideration. Very very cool

cc @jimexist who was working with null constants a while ago and might be interested i this work

alamb · 2022-04-28T20:20:00Z

Cargo.toml

 codegen-units = 1
 lto = true
+
+[patch.crates-io]


BTW arrow-rs 13.0.0 with these changes should be released sometime early next wee

alamb · 2022-04-28T20:20:11Z

datafusion/common/src/scalar.rs

 #[derive(Clone)]
 pub enum ScalarValue {
+    /// represents null
+    Null,


datafusion/common/src/scalar.rs

alamb · 2022-04-28T20:26:48Z

datafusion/common/src/scalar.rs

                eq_array_primitive!(array, index, IntervalMonthDayNanoArray, val)
            }
            ScalarValue::Struct(_, _) => unimplemented!(),
+            ScalarValue::Null => false,


alamb · 2022-04-28T20:28:29Z

datafusion/core/src/sql/planner.rs

                        SQLExpr::Value(Value::SingleQuotedString(s)) => Ok(lit(s)),
                        SQLExpr::Value(Value::Null) => {
-                            Ok(Expr::Literal(ScalarValue::Utf8(None)))
+                            Ok(Expr::Literal(ScalarValue::Null))


Well this explains a lot of odd type coercion errors I have been seeing

alamb · 2022-04-28T20:30:09Z

datafusion/expr/src/binary_rule.rs


+/// coercion rules from NULL type. Since NULL can be casted to most of types in arrow,
+/// either lhs or rhs is NULL, if NULL can be casted to type of the other side, the coecion is valid.
+fn null_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {


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

WinkerDu · 2022-04-29T18:52:48Z

cc @alamb PTAL, thank you ❤️

alamb · 2022-04-30T13:54:53Z

This looks great @WinkerDu -- thank you -- I am working to get arrow 13 -- see #2382. It should only be a few more days now.

WinkerDu · 2022-05-02T12:58:01Z

This looks great @WinkerDu -- thank you -- I am working to get arrow 13 -- see #2382. It should only be a few more days now.

Thanks @alamb look forward to it !

WinkerDu · 2022-05-04T04:07:48Z

Since #2382 is merged, I switch this pr to open status for code reviewing

WinkerDu · 2022-05-04T06:21:55Z

cc @alamb @andygrove @yjshen @xudong963
Please have a review, thank you

alamb

Looking very good @WinkerDu -- thank you!

I think there is a small issue with null = null but it should be fairly easy to solve.

Thanks again!

alamb · 2022-05-04T13:43:31Z

datafusion/core/tests/sql/expr.rs

-        "| 999                                                                                          |",
-        "+----------------------------------------------------------------------------------------------+",
+        "+----------------------------------------------------------------------------------------+",
+        "| CASE WHEN #t1.c1 = Utf8(\"a\") THEN Int64(1) WHEN NULL THEN Int64(2) ELSE Int64(999) END |",


This is the key difference here. Very nice 👍

alamb · 2022-05-04T13:45:27Z

datafusion/core/tests/sql/joins.rs

            INNER JOIN (SELECT null AS id2) t2 ON id1 = id2";

-    let expected = vec!["++", "++"];
+    let expected = vec![


This answer is not correct -- there should be no rows that match.

This is because the join should produce rows where id1 = id2 evaluates to true

However, null = null evaluates to null 🤯

Here is the query in postgres:

alamb=# SELECT * FROM (SELECT null AS id1) t1 INNER JOIN (SELECT null AS id2) t2 ON id1 = id2 alamb-# ; id1 | id2 -----+----- (0 rows)

alamb · 2022-05-04T13:48:04Z

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

+pub fn eq_null(left: &NullArray, _right: &NullArray) -> Result<BooleanArray> {
+    let length = left.len();
+    make_boolean_array(length, false)
+}


I don't think this is correct -- specifically I think the resulting boolean array should be full of nulls not false

Perhaps something like:

std::iter::repeat(left.len(), None).collect()

andygrove · 2022-05-04T21:12:40Z

This is looking great. Thanks @WinkerDu! I don't have any additional comments beyond what @alamb has already raised

WinkerDu · 2022-05-05T20:05:26Z

cc @alamb PTAL, thank you

alamb

Awesome -- thank you so much @WinkerDu -- this is great

alamb · 2022-05-06T17:24:19Z

datafusion/core/tests/sql/joins.rs

            INNER JOIN (SELECT null AS id2) t2 ON id1 = id2";

-    let expected = vec!["++", "++"];
+    #[rustfmt::skip]


alamb · 2022-05-06T17:25:03Z

🎉 we are going to have real null support 😍

WinkerDu · 2022-05-07T04:33:36Z

Thank you all @alamb @andygrove

…ll` (apache#2364) * introduce null * fix fmt

github-actions bot added the datafusion label Apr 28, 2022

WinkerDu marked this pull request as draft April 28, 2022 03:12

WinkerDu mentioned this pull request Apr 28, 2022

(NOT FOR MERGING) Patch datafusion to use preview of arrow-rs #2269

Closed

alamb reviewed Apr 28, 2022

View reviewed changes

alamb changed the title ~~introduce ScalarValue::Null to df~~ Add proper support for null literal by introducing ScalarValue::Null Apr 28, 2022

WinkerDu force-pushed the master-introduce-null branch from 501add5 to 0100277 Compare May 4, 2022 04:02

WinkerDu marked this pull request as ready for review May 4, 2022 04:08

alamb reviewed May 4, 2022

View reviewed changes

introduce null

13a1601

WinkerDu force-pushed the master-introduce-null branch from 0100277 to 13a1601 Compare May 5, 2022 18:55

fix fmt

109bc0e

alamb approved these changes May 6, 2022

View reviewed changes

alamb merged commit fcc35e8 into apache:master May 6, 2022

This was referenced May 7, 2022

Adds unit test cases of mathematical expressions working with null literal #2478

Merged

Improve expressions to completely support NULL constants #2483

Open

ovr pushed a commit to cube-js/arrow-datafusion that referenced this pull request Aug 15, 2022

Add proper support for null literal by introducing `ScalarValue::Nu…

bfb1e78

…ll` (apache#2364) * introduce null * fix fmt

ovr pushed a commit to cube-js/arrow-datafusion that referenced this pull request Aug 15, 2022

Add proper support for null literal by introducing `ScalarValue::Nu…

35d2daf

…ll` (apache#2364) * introduce null * fix fmt

ovr pushed a commit to cube-js/arrow-datafusion that referenced this pull request Aug 15, 2022

Add proper support for null literal by introducing `ScalarValue::Nu…

551a0e5

…ll` (apache#2364) * introduce null * fix fmt

ovr pushed a commit to cube-js/arrow-datafusion that referenced this pull request Aug 15, 2022

Add proper support for null literal by introducing `ScalarValue::Nu…

a5bea4b

…ll` (apache#2364) * introduce null * fix fmt

MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Jun 9, 2023

Add proper support for null literal by introducing `ScalarValue::Nu…

99fdae5

…ll` (apache#2364) * introduce null * fix fmt

MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Jun 9, 2023

Add proper support for null literal by introducing `ScalarValue::Nu…

eae4e05

…ll` (apache#2364) * introduce null * fix fmt

MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Jan 19, 2024

Add proper support for null literal by introducing `ScalarValue::Nu…

e30d7d0

…ll` (apache#2364) * introduce null * fix fmt

MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Feb 5, 2024

Add proper support for null literal by introducing `ScalarValue::Nu…

13ad9bd

…ll` (apache#2364) * introduce null * fix fmt

MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Feb 5, 2024

Add proper support for null literal by introducing `ScalarValue::Nu…

a78e521

…ll` (apache#2364) * introduce null * fix fmt

Comments

Conversation

WinkerDu commented Apr 28, 2022 • 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 there any user-facing changes?

Uh oh!

WinkerDu commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WinkerDu commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

WinkerDu commented Apr 29, 2022

Uh oh!

alamb commented Apr 30, 2022

Uh oh!

WinkerDu commented May 2, 2022

Uh oh!

WinkerDu commented May 4, 2022

Uh oh!

WinkerDu commented May 4, 2022

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!

Choose a reason for hiding this comment

Uh oh!

andygrove commented May 4, 2022

Uh oh!

WinkerDu commented May 5, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented May 6, 2022

Uh oh!

WinkerDu commented May 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

WinkerDu commented Apr 28, 2022 •

edited

Loading

WinkerDu commented Apr 28, 2022 •

edited

Loading

WinkerDu commented Apr 28, 2022 •

edited

Loading