Skip to content

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.

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

}

/// 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!

) -> Option<DataType> {
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 Changes to the physical-expr crates 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 logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates

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