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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions datafusion/core/tests/sql/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1254,3 +1254,51 @@ async fn comparisons_with_null_lt() {
assert!(batch.columns()[0].is_null(1));
}
}

#[tokio::test]
async fn binary_mathematical_operator_with_null_lt() {
let ctx = SessionContext::new();

let cases = vec![
// 1. Integer and NULL
"select column1 + NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
"select column1 - NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
"select column1 * NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
"select column1 / NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
"select column1 % NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
// 2. Float and NULL
"select column2 + NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
"select column2 - NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
"select column2 * NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
"select column2 / NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
"select column2 % NULL from (VALUES (1, 2.3), (2, 5.4)) as t",
// ----
// ---- same queries, reversed argument order
// ----
// 3. NULL and Integer
"select NULL + column1 from (VALUES (1, 2.3), (2, 5.4)) as t",
"select NULL - column1 from (VALUES (1, 2.3), (2, 5.4)) as t",
"select NULL * column1 from (VALUES (1, 2.3), (2, 5.4)) as t",
"select NULL / column1 from (VALUES (1, 2.3), (2, 5.4)) as t",
"select NULL % column1 from (VALUES (1, 2.3), (2, 5.4)) as t",
// 4. NULL and Float
"select NULL + column2 from (VALUES (1, 2.3), (2, 5.4)) as t",
"select NULL - column2 from (VALUES (1, 2.3), (2, 5.4)) as t",
"select NULL * column2 from (VALUES (1, 2.3), (2, 5.4)) as t",
"select NULL / column2 from (VALUES (1, 2.3), (2, 5.4)) as t",
"select NULL % column2 from (VALUES (1, 2.3), (2, 5.4)) as t",
];

for sql in cases {
println!("Computing: {}", sql);

let mut actual = execute_to_batches(&ctx, sql).await;
assert_eq!(actual.len(), 1);

let batch = actual.pop().unwrap();
assert_eq!(batch.num_rows(), 2);
assert_eq!(batch.num_columns(), 1);
assert!(batch.columns()[0].is_null(0));
assert!(batch.columns()[0].is_null(1));
}
}
11 changes: 10 additions & 1 deletion datafusion/expr/src/binary_rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

if !is_numeric(lhs_type) || !is_numeric(rhs_type) {
if !both_numeric_or_null_and_numeric(lhs_type, rhs_type) {
return None;
};

Expand Down Expand Up @@ -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.

👍

match (lhs_type, rhs_type) {
(_, DataType::Null) => is_numeric(lhs_type),
(DataType::Null, _) => is_numeric(rhs_type),
_ => is_numeric(lhs_type) && is_numeric(rhs_type),
}
}

/// Coercion rules for dictionary values (aka the type of the dictionary itself)
fn dictionary_value_coercion(
lhs_type: &DataType,
Expand Down
22 changes: 12 additions & 10 deletions datafusion/physical-expr/src/expressions/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -701,16 +701,18 @@ 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()?,
)?))
if $RIGHT.is_null() {
Ok(Arc::new(new_null_array($LEFT.data_type(), $LEFT.len())))
} else {
let ll = $LEFT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
Ok(Arc::new(paste::expr! {[<$OP _scalar>]}(
&ll,
$RIGHT.try_into()?,
)?))
}
}};
}

Expand Down