You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/24 19:19:00 UTC
[GitHub] [arrow-datafusion] WinkerDu opened a new pull request, #2610: binary mathematical operator work with `NULL`
WinkerDu opened a new pull request, #2610:
URL: https://github.com/apache/arrow-datafusion/pull/2610
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] WinkerDu commented on a diff in pull request #2610: binary mathematical operators work with `NULL`
Posted by GitBox <gi...@apache.org>.
WinkerDu commented on code in PR #2610:
URL: https://github.com/apache/arrow-datafusion/pull/2610#discussion_r882882003
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -701,16 +701,58 @@ macro_rules! compute_bool_op {
/// LEFT is array, RIGHT is scalar value
macro_rules! compute_op_scalar {
($LEFT:expr, $RIGHT:expr, $OP:ident, $DT:ident) => {{
- let ll = $LEFT
- .as_any()
- .downcast_ref::<$DT>()
- .expect("compute_op failed to downcast array");
- // generate the scalar function name, such as lt_scalar, from the $OP parameter
- // (which could have a value of lt) and the suffix _scalar
- Ok(Arc::new(paste::expr! {[<$OP _scalar>]}(
- &ll,
- $RIGHT.try_into()?,
- )?))
+ match $RIGHT {
Review Comment:
nice catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #2610: Support binary mathematical operators work with `NULL` literals
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2610:
URL: https://github.com/apache/arrow-datafusion/pull/2610#issuecomment-1138951941
Thanks @WinkerDu and @jackwener -- 🤗
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2610: binary mathematical operators work with `NULL`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2610:
URL: https://github.com/apache/arrow-datafusion/pull/2610#discussion_r880888491
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -701,16 +701,58 @@ macro_rules! compute_bool_op {
/// LEFT is array, RIGHT is scalar value
macro_rules! compute_op_scalar {
($LEFT:expr, $RIGHT:expr, $OP:ident, $DT:ident) => {{
- let ll = $LEFT
- .as_any()
- .downcast_ref::<$DT>()
- .expect("compute_op failed to downcast array");
- // generate the scalar function name, such as lt_scalar, from the $OP parameter
- // (which could have a value of lt) and the suffix _scalar
- Ok(Arc::new(paste::expr! {[<$OP _scalar>]}(
- &ll,
- $RIGHT.try_into()?,
- )?))
+ match $RIGHT {
Review Comment:
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
?
##########
datafusion/expr/src/binary_rule.rs:
##########
@@ -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 {
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] WinkerDu commented on pull request #2610: binary mathematical operators work with `NULL`
Posted by GitBox <gi...@apache.org>.
WinkerDu commented on PR #2610:
URL: https://github.com/apache/arrow-datafusion/pull/2610#issuecomment-1138886648
cc @alamb PTAL, thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #2610: Support binary mathematical operators work with `NULL` literals
Posted by GitBox <gi...@apache.org>.
alamb merged PR #2610:
URL: https://github.com/apache/arrow-datafusion/pull/2610
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jackwener commented on a diff in pull request #2610: binary mathematical operators work with `NULL`
Posted by GitBox <gi...@apache.org>.
jackwener commented on code in PR #2610:
URL: https://github.com/apache/arrow-datafusion/pull/2610#discussion_r882265217
##########
datafusion/expr/src/binary_rule.rs:
##########
@@ -282,7 +282,7 @@ fn mathematics_numerical_coercion(
use arrow::datatypes::DataType::*;
// error on any non-numeric type
Review Comment:
```suggestion
// error on any not numeric/null type
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org