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

Support binary mathematical operators work with NULL literals #2610

Merged
merged 2 commits into from
May 26, 2022

Conversation

WinkerDu
Copy link
Contributor

@WinkerDu WinkerDu commented May 24, 2022

Which issue does this PR close?

Closes #2609 .

Rationale for this change

There is bug when binary mathematical operators +, -, *, /, % work with literal NULL

To reproduce

> select column1 + NULL from (VALUES (1, 2.3), (2, 5.4)) as t
Plan("'Int64 + Null' can't be evaluated because there isn't a common type to coerce the types to")

Postgres works like

# select column1 + NULL from (VALUES (1, 2.3), (2, 5.4)) as t;
 ?column? 
----------
         
         
(2 rows)

What changes are included in this PR?

  • Adjusts mathematics_numerical_coercion in binary_rule.rs, make it work well on condition that at most one side of lhs or rhs is NULL
  • Enhances compute_op_scalar macro in binary.rs to produce NULL array when scalar value is NULL

Are there any user-facing changes?

No.

@github-actions github-actions bot added the datafusion Changes in the datafusion crate label May 24, 2022
@WinkerDu WinkerDu changed the title binary mathematical operator work with NULL binary mathematical operators work with NULL May 24, 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.

Love it -- thank you @WinkerDu

Keep the code train moving 🚋

@@ -412,6 +412,15 @@ pub fn is_numeric(dt: &DataType) -> bool {
}
}

/// Determine if at least of one of lhs and rhs is numeric, and the other must be NULL or numeric
fn both_numeric_or_null_and_numeric(lhs_type: &DataType, rhs_type: &DataType) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

&ll,
$RIGHT.try_into()?,
)?))
match $RIGHT {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you could use ScalarValue::is_null() here rather than having to expand the macros out more

https://github.com/apache/arrow-datafusion/blob/master/datafusion/common/src/scalar.rs#L627

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch!

@@ -282,7 +282,7 @@ fn mathematics_numerical_coercion(
use arrow::datatypes::DataType::*;

// error on any non-numeric type
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// error on any non-numeric type
// error on any not numeric/null type

Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

LGTM after fix review.
Thanks @WinkerDu ❤️.

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions labels May 26, 2022
@WinkerDu
Copy link
Contributor Author

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

its-so-beautiful-crying-gif

@alamb alamb changed the title binary mathematical operators work with NULL Support binary mathematical operators work with NULL literals May 26, 2022
@alamb alamb merged commit 07574bd into apache:master May 26, 2022
@alamb
Copy link
Contributor

alamb commented May 26, 2022

Thanks @WinkerDu and @jackwener -- 🤗

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

* binary mathematical operator with NULL

* code clean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate datafusion Changes in the datafusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add, Minus, Multiply, divide, Modulo operator work with literal NULL
3 participants