Skip to content

Comments

fix not(null) with constant null#2144

Merged
xudong963 merged 1 commit intoapache:masterfrom
WinkerDu:master-not-with-null
Apr 9, 2022
Merged

fix not(null) with constant null#2144
xudong963 merged 1 commit intoapache:masterfrom
WinkerDu:master-not-with-null

Conversation

@WinkerDu
Copy link
Contributor

@WinkerDu WinkerDu commented Apr 3, 2022

Which issue does this PR close?

Closes #1191.

Rationale for this change

df not expression can not take NULL like

> select not(null);
Internal("NOT 'Literal { value: Utf8(NULL) }' can't be evaluated because the expression's type is Utf8, not boolean")

the correct output should be NULL

What changes are included in this PR?

NotExpr accepts NULL input presented as ScalarValue::Utf8(None)

Are there any user-facing changes?

NO.

@WinkerDu WinkerDu mentioned this pull request Apr 3, 2022
Closed
@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 4, 2022

cc @alamb @Dandandan @yjshen
plz have a review, thank you.

@Ted-Jiang
Copy link
Member

Ted-Jiang commented Apr 4, 2022

Is there any Null transform rule (doc) in datafusion?

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 4, 2022 •
edited
Loading

@Ted-Jiang I'm afraid there is not.
Maybe most of expressions (except for string expressions like concat, concat_ws, etc) should return NULL when accept NULL, correct me if wrong @alamb

@alamb alamb changed the title fix not(null) issue fix not(null) with constant null Apr 4, 2022
alamb
alamb reviewed Apr 4, 2022
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you for starting to take this on @WinkerDu ❤️

While the general purpose handling of null literals will likely be a more substantial change, I think this is a good step forward. If we can tighten up the checks a little I think we could merge this and improve things incrementally in follow on PRs.

alamb
alamb reviewed Apr 4, 2022
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

I wonder if it would be helpful to add a test in datafusion/core/tests/sql somewhere to show a query like SELECT null, not(null) working?

@WinkerDu WinkerDu force-pushed the master-not-with-null branch from a437f28 to 5553028 Compare April 8, 2022 16:49
@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 8, 2022

I wonder if it would be helpful to add a test in datafusion/core/tests/sql somewhere to show a query like SELECT null, not(null) working?

@alamb OK, will add more tests:

  1. SELECT not(true), not(false) = false, true
  2. SELECT null, not(null) = NULL, NULL
  3. SELECT NOT('hi'), type check error would raise.

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 8, 2022

@alamb PTAL, thank you.

alamb
alamb approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good to me -- thank you @WinkerDu

datafusion/physical-expr/src/expressions/not.rs
}
ColumnarValue::Scalar(scalar) => {
if scalar.is_null() {
return Ok(ColumnarValue::Scalar(ScalarValue::Boolean(None)));
Copy link
Contributor

@alamb alamb Apr 8, 2022

Choose a reason for hiding this comment

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

👍

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 9, 2022

cc @yjshen @xudong963 @liukun4515 @Dandandan PTAL, thank you!

xudong963
xudong963 approved these changes Apr 9, 2022
View reviewed changes
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

❯ select not null;
+----------------+
| NOT Utf8(NULL) |
+----------------+
|                |
+----------------+
1 row in set. Query took 0.003 seconds.
❯ select not(null);
+----------------+
| NOT Utf8(NULL) |
+----------------+
|                |
+----------------+
1 row in set. Query took 0.003 seconds.

I tested in my local, very good! @WinkerDu

@xudong963 xudong963 merged commit 2d90840 into apache:master Apr 9, 2022
@WinkerDu WinkerDu deleted the master-not-with-null branch April 17, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@xudong963 xudong963 xudong963 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

not() with NULL literals does not work: can't be evaluated because the expression's type is Utf8, not boolean

4 participants

@WinkerDu @Ted-Jiang @alamb @xudong963