Skip to content

Comments

Numeric, String, Boolean comparisons with literal NULL#2481

Merged
yjshen merged 1 commit intoapache:masterfrom
WinkerDu:master-binary-expr-null
May 11, 2022
Merged

Numeric, String, Boolean comparisons with literal NULL#2481
yjshen merged 1 commit intoapache:masterfrom
WinkerDu:master-binary-expr-null

Conversation

@WinkerDu
Copy link
Contributor

@WinkerDu WinkerDu commented May 7, 2022 •
edited
Loading

Which issue does this PR close?

Closes #1179 and #2482 .

Rationale for this change

This pr aims to solve Numeric, String, Boolean comparisons (Binary Expressions) with literal NULL
For now, DF would fail when running the following SQLs:

> select * from (VALUES (1, 'foo' ,2.3), (2, 'bar', 5.4)) as t;
+---------+---------+---------+
| column1 | column2 | column3 |
+---------+---------+---------+
| 1       | foo     | 2.3     |
| 2       | bar     | 5.4     |
+---------+---------+---------+
2 rows in set. Query took 0.004 seconds.
> select column1 < null from (VALUES (1, 'foo' ,2.3), (2, 'bar', 5.4)) as t;
Plan("'Int64 < Utf8' can't be evaluated because there isn't a common type to coerce the types to")
> select column2 < null from (VALUES (1, 'foo' ,2.3), (2, 'bar', 5.4)) as t;
ArrowError(ExternalError(Internal("compute_utf8_op_scalar for 'lt' failed to cast literal value NULL")))
> select column3 < null from (VALUES (1, 'foo' ,2.3), (2, 'bar', 5.4)) as t;
Plan("'Float64 < Utf8' can't be evaluated because there isn't a common type to coerce the types to")

What changes are included in this PR?

  • Introduces null_coercion to comparison_eq_coercion and comparison_order_coercion
  • Enhances binary expression macros to support NULL scalar value comparing with array, like binary_array_op_dyn_scalar

Are there any user-facing changes?

No.

@WinkerDu
Copy link
Contributor Author

WinkerDu commented May 8, 2022

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

yjshen
yjshen reviewed May 9, 2022
View reviewed changes
datafusion/physical-expr/src/expressions/binary.rs
stringify!($OP),
)))
// when the $RIGHT is a NULL, generate a NULL array of $OP_TYPE
Ok(Arc::new(new_null_array($OP_TYPE, $LEFT.len())))
Copy link
Member

@yjshen yjshen May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  1. I think we should respect SortOptions here for ScalaValue.
  2. Question: how about the NULL != column case? Do we have an expression normalizer that asserts Nulls often coming as the right-hand operand for a binary comparison?

Copy link
Contributor

@alamb alamb May 9, 2022

Choose a reason for hiding this comment

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

@yjshen what do you mean SortOptions? I don't think these expressions are used directly in ORDER BY clauses -- they are used instead for expressions like a < b

Adding some more tests in sql/expr.rs for NULL < column and NULL > column sounds like a good idea.

Copy link
Member

@yjshen yjshen May 9, 2022

Choose a reason for hiding this comment

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

After checking Postgres and MySQL, I found I'm wrong on the expected behavior while comparing columns with NULL using < > ..., they will return NULL in all cases. I was expecting to apply null first or null last while comparing column with NULL, so always true or always false at the first time.

Copy link
Contributor Author

@WinkerDu WinkerDu May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @alamb @yjshen
I agree this macro is just used in expression evaluate, maybe we can ignore SortOptions in this pr.
I'll look into this case NULL != column carefully and add more cases in ut.
Besides, I am super busy these days, I'll do it ASAP 🤟

Copy link
Contributor

@alamb alamb May 11, 2022

Choose a reason for hiding this comment

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

I will spend some time writing the NULL != column

Copy link
Contributor

@alamb alamb May 11, 2022

Choose a reason for hiding this comment

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

Follow on PR: #2510 turns out there was a bug!

alamb
alamb approved these changes May 9, 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.

This looks great to me -- thank you @WinkerDu

I think we should make sure @yjshen is satisfied with the answers about SortOptions and a few more tests would be good. But otherwise 👍

Very nice

datafusion/physical-expr/src/expressions/binary.rs
stringify!($OP),
)))
// when the $RIGHT is a NULL, generate a NULL array of $OP_TYPE
Ok(Arc::new(new_null_array($OP_TYPE, $LEFT.len())))
Copy link
Contributor

@alamb alamb May 9, 2022

Choose a reason for hiding this comment

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

@yjshen what do you mean SortOptions? I don't think these expressions are used directly in ORDER BY clauses -- they are used instead for expressions like a < b

Adding some more tests in sql/expr.rs for NULL < column and NULL > column sounds like a good idea.

yjshen
yjshen approved these changes May 11, 2022
View reviewed changes
Copy link
Member

@yjshen yjshen left a comment

Choose a reason for hiding this comment

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

Thanks @WinkerDu. I think the current behavior that always yields null while comparing with null is consistent with other systems, so no more efforts are required on this.

It would be nice to guarantee expression normalization exists and always have scalar value as the right operand after normalization. I think this goes beyond the scope of this PR and we could do it as follow-ups.

Thanks for your perseverance and nice work!

@yjshen yjshen merged commit ac35f34 into apache:master May 11, 2022
alamb
alamb reviewed May 11, 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.

Agree with @yjshen -- thank you @WinkerDu

datafusion/physical-expr/src/expressions/binary.rs
stringify!($OP),
)))
// when the $RIGHT is a NULL, generate a NULL array of $OP_TYPE
Ok(Arc::new(new_null_array($OP_TYPE, $LEFT.len())))
Copy link
Contributor

@alamb alamb May 11, 2022

Choose a reason for hiding this comment

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

I will spend some time writing the NULL != column

@alamb alamb mentioned this pull request May 11, 2022
Merged
@WinkerDu WinkerDu mentioned this pull request May 20, 2022
Closed
ovr pushed a commit to cube-js/arrow-datafusion that referenced this pull request Aug 15, 2022
@WinkerDu @ovr
Numeric, String, Boolean comparisons with literal NULL (apache#2481)
ddeda51
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request May 11, 2023
@WinkerDu @MazterQyou
Numeric, String, Boolean comparisons with literal NULL (apache#2481)
924813d
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request May 11, 2023
@WinkerDu @MazterQyou
Numeric, String, Boolean comparisons with literal NULL (apache#2481)
1a26be2
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Jun 9, 2023
@WinkerDu @MazterQyou
Numeric, String, Boolean comparisons with literal NULL (apache#2481)
78fd3a0
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Jun 9, 2023
@WinkerDu @MazterQyou
Numeric, String, Boolean comparisons with literal NULL (apache#2481)
b5de6f5
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Jan 19, 2024
@WinkerDu @MazterQyou
Numeric, String, Boolean comparisons with literal NULL (apache#2481)
de90292
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Feb 5, 2024
@WinkerDu @MazterQyou
Numeric, String, Boolean comparisons with literal NULL (apache#2481)
c91541d
MazterQyou pushed a commit to cube-js/arrow-datafusion that referenced this pull request Feb 5, 2024
@WinkerDu @MazterQyou
Numeric, String, Boolean comparisons with literal NULL (apache#2481)
a096a47
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

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

Comparisons (BinaryExprs) to literal NULL don't work

3 participants

@WinkerDu @alamb @yjshen