Skip to content

fix comparison of decimal array with null scalar #3567

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.

Sign up for GitHub

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

Merged
alamb merged 1 commit into from
Sep 22, 2022
Merged

fix comparison of decimal array with null scalar #3567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 26 additions & 0 deletions datafusion/core/tests/sql/decimal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,3 +811,29 @@ async fn sql_abs_decimal() -> Result<()> {
assert_batches_eq!(expected, &actual);
Ok(())
}

#[tokio::test]
async fn decimal_null_scalar_array_comparison() -> Result<()> {
let ctx = SessionContext::new();
let sql = "select a < null from (values (1.1::decimal)) as t(a)";
let actual = execute_to_batches(&ctx, sql).await;
assert_eq!(1, actual.len());
assert_eq!(1, actual[0].num_columns());
assert_eq!(1, actual[0].num_rows());
assert!(actual[0].column(0).is_null(0));
assert_eq!(&DataType::Boolean, actual[0].column(0).data_type());
Ok(())
}

#[tokio::test]
async fn decimal_null_array_scalar_comparison() -> Result<()> {
let ctx = SessionContext::new();
let sql = "select null <= a from (values (1.1::decimal)) as t(a);";
let actual = execute_to_batches(&ctx, sql).await;
assert_eq!(1, actual.len());
assert_eq!(1, actual[0].num_columns());
assert_eq!(1, actual[0].num_rows());
assert!(actual[0].column(0).is_null(0));
assert_eq!(&DataType::Boolean, actual[0].column(0).data_type());
Ok(())
}
32 changes: 26 additions & 6 deletions datafusion/physical-expr/src/expressions/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,33 @@ impl std::fmt::Display for BinaryExpr {
}
}

macro_rules! compute_decimal_op_dyn_scalar {
($LEFT:expr, $RIGHT:expr, $OP:ident, $OP_TYPE:expr) => {{
let ll = $LEFT.as_any().downcast_ref::<Decimal128Array>().unwrap();
if let ScalarValue::Decimal128(Some(_), _, _) = $RIGHT {
Ok(Arc::new(paste::expr! {[<$OP _decimal_scalar>]}(
ll,
$RIGHT.try_into()?,
)?))
} else {
// when the $RIGHT is a NULL, generate a NULL array of $OP_TYPE type
Ok(Arc::new(new_null_array($OP_TYPE, $LEFT.len())))
}
}};
}

macro_rules! compute_decimal_op_scalar {
($LEFT:expr, $RIGHT:expr, $OP:ident, $DT:ident) => {{
let ll = $LEFT.as_any().downcast_ref::<$DT>().unwrap();
Ok(Arc::new(paste::expr! {[<$OP _decimal_scalar>]}(
ll,
$RIGHT.try_into()?,
)?))
let ll = $LEFT.as_any().downcast_ref::<Decimal128Array>().unwrap();
if let ScalarValue::Decimal128(Some(_), _, _) = $RIGHT {
Ok(Arc::new(paste::expr! {[<$OP _decimal_scalar>]}(
ll,
$RIGHT.try_into()?,
)?))
} else {
Copy link
Contributor

@alamb alamb Sep 21, 2022

Choose a reason for hiding this comment

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

Should we assert here that $RIGHT is still a decimal ? As written the code could convert ScalarValue::Int8(Some(4)) into NULL, for example

But I don't know if such a ScalarValue could be passed

Copy link
Contributor Author

@kmitchener kmitchener Sep 21, 2022

Choose a reason for hiding this comment

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

I don't think we need to. Those 2 decimal macros are used only in 1 place each and include "decimal" in the name -- it'd be a clear mistake to use it for any other datatype. This area could use some refactoring, but I wanted to get this in and the kernels moved to arrow-rs then maybe revisit this whole module (famous last words, right?).

Copy link
Contributor

@alamb alamb Sep 22, 2022

Choose a reason for hiding this comment

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

famous last words maybe, but a reasonable plan nonetheless ;)

// when the $RIGHT is a NULL, generate a NULL array of LEFT's datatype
Ok(Arc::new(new_null_array($LEFT.data_type(), $LEFT.len())))
}
}};
}

Expand Down Expand Up @@ -642,7 +662,7 @@ macro_rules! binary_array_op_dyn_scalar {

let result: Result<Arc<dyn Array>> = match right {
ScalarValue::Boolean(b) => compute_bool_op_dyn_scalar!($LEFT, b, $OP, $OP_TYPE),
ScalarValue::Decimal128(..) => compute_decimal_op_scalar!($LEFT, right, $OP, Decimal128Array),
ScalarValue::Decimal128(..) => compute_decimal_op_dyn_scalar!($LEFT, right, $OP, $OP_TYPE),
Copy link
Contributor

@alamb alamb Sep 21, 2022

Choose a reason for hiding this comment

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

❤️ you are starting to figure out the thicket that is binary.rs 👍

Copy link
Contributor Author

@kmitchener kmitchener Sep 21, 2022

Choose a reason for hiding this comment

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

Oh yeah :) and what a pleasure it is

ScalarValue::Utf8(v) => compute_utf8_op_dyn_scalar!($LEFT, v, $OP, $OP_TYPE),
ScalarValue::LargeUtf8(v) => compute_utf8_op_dyn_scalar!($LEFT, v, $OP, $OP_TYPE),
ScalarValue::Binary(v) => compute_binary_op_dyn_scalar!($LEFT, v, $OP, $OP_TYPE),
Expand Down