Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix not(null) with constant null #2144

Merged
merged 1 commit into from
Apr 9, 2022

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
Copy link
Contributor Author

WinkerDu commented Apr 4, 2022

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

@Ted-Jiang
Copy link
Member

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

@WinkerDu
Copy link
Contributor Author

WinkerDu commented Apr 4, 2022

@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
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.

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.

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

@@ -86,6 +86,16 @@ impl PhysicalExpr for NotExpr {
)))
}
ColumnarValue::Scalar(scalar) => {
if scalar.is_null() {
return Ok(ColumnarValue::Scalar(ScalarValue::Boolean(None)));
Copy link
Contributor

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!

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
Labels
datafusion Changes in the datafusion crate
Projects
None yet
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