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.

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

Jump to bottom

Fix is_distinct from for float NaN values #5446

Merged
merged 2 commits into from
Mar 3, 2023
Merged

Fix is_distinct from for float NaN values #5446

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80e25d3
impl is_distinct for floats
comphead Mar 2, 2023
c6430d0
impl is_distinct for floats.comments
comphead Mar 2, 2023
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/sqllogictests/test_files/select.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,29 @@ select '1' from foo order by column1;
# foo distinct order by
statement error DataFusion error: Error during planning: For SELECT DISTINCT, ORDER BY expressions column1 must appear in select list
select distinct '1' from foo order by column1;

# distincts for float nan
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a good test to add to https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests/test_files/pg_compat so they are automatically compared with postgres as part of CI (kudos to @melgenek for help there)

https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/pg_compat/pg_compat_simple.slt perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ive duplicated in both select.slt and pg_compat_simple.slt Reason for that is PG doesn't support double datatype

query BBBBBBBBBBBBBBBBB
select
'nan'::double is distinct from 'nan'::double v1,
'nan'::double is not distinct from 'nan'::double v2,
'nan'::double is not distinct from null v3,
'nan'::double is distinct from null v4,
null is distinct from 'nan'::double v5,
null is not distinct from 'nan'::double v6,
'nan'::float is distinct from 'nan'::float v7,
'nan'::float is not distinct from 'nan'::float v8,
'nan'::float is not distinct from null v9,
'nan'::float is distinct from null v10,
null is distinct from 'nan'::float v11,
null is not distinct from 'nan'::float v12,
1::float is distinct from 2::float v13,
'nan'::float is distinct from 1::float v14,
'nan'::float is not distinct from 1::float v15,
1::float is not distinct from null v16,
1::float is distinct from null v17
;
----
false true false true true false false true false true true false true true false false true


59 changes: 44 additions & 15 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 @@ -63,11 +63,12 @@ use kernels::{
use kernels_arrow::{
add_decimal_dyn_scalar, add_dyn_decimal, divide_decimal_dyn_scalar,
divide_dyn_opt_decimal, is_distinct_from, is_distinct_from_bool,
is_distinct_from_decimal, is_distinct_from_null, is_distinct_from_utf8,
is_not_distinct_from, is_not_distinct_from_bool, is_not_distinct_from_decimal,
is_not_distinct_from_null, is_not_distinct_from_utf8, modulus_decimal,
modulus_decimal_scalar, multiply_decimal_dyn_scalar, multiply_dyn_decimal,
subtract_decimal_dyn_scalar, subtract_dyn_decimal,
is_distinct_from_decimal, is_distinct_from_f32, is_distinct_from_f64,
is_distinct_from_null, is_distinct_from_utf8, is_not_distinct_from,
is_not_distinct_from_bool, is_not_distinct_from_decimal, is_not_distinct_from_f32,
is_not_distinct_from_f64, is_not_distinct_from_null, is_not_distinct_from_utf8,
modulus_decimal, modulus_decimal_scalar, multiply_decimal_dyn_scalar,
multiply_dyn_decimal, subtract_decimal_dyn_scalar, subtract_dyn_decimal,
};

use arrow::datatypes::{DataType, Schema, TimeUnit};
Expand Down Expand Up @@ -184,16 +185,44 @@ macro_rules! compute_decimal_op {
}};
}

macro_rules! compute_f32_op {
($LEFT:expr, $RIGHT:expr, $OP:ident, $DT:ident) => {{
let ll = $LEFT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast left side array");
let rr = $RIGHT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast right side array");
Ok(Arc::new(paste::expr! {[<$OP _f32>]}(ll, rr)?))
}};
}

macro_rules! compute_f64_op {
($LEFT:expr, $RIGHT:expr, $OP:ident, $DT:ident) => {{
let ll = $LEFT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast left side array");
let rr = $RIGHT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast right side array");
Ok(Arc::new(paste::expr! {[<$OP _f64>]}(ll, rr)?))
}};
}

macro_rules! compute_null_op {
($LEFT:expr, $RIGHT:expr, $OP:ident, $DT:ident) => {{
let ll = $LEFT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
.expect("compute_op failed to downcast left side array");
let rr = $RIGHT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
.expect("compute_op failed to downcast right side array");
Ok(Arc::new(paste::expr! {[<$OP _null>]}(&ll, &rr)?))
}};
}
Expand All @@ -204,11 +233,11 @@ macro_rules! compute_utf8_op {
let ll = $LEFT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
.expect("compute_op failed to downcast left side array");
let rr = $RIGHT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
.expect("compute_op failed to downcast right side array");
Ok(Arc::new(paste::expr! {[<$OP _utf8>]}(&ll, &rr)?))
}};
}
Expand All @@ -219,7 +248,7 @@ macro_rules! compute_utf8_op_scalar {
let ll = $LEFT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
.expect("compute_op failed to downcast left side array");
if let ScalarValue::Utf8(Some(string_value)) = $RIGHT {
Ok(Arc::new(paste::expr! {[<$OP _utf8_scalar>]}(
&ll,
Expand Down Expand Up @@ -318,7 +347,7 @@ macro_rules! compute_op_scalar {
let ll = $LEFT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
.expect("compute_op failed to downcast left side array");
Ok(Arc::new(paste::expr! {[<$OP _scalar>]}(
&ll,
$RIGHT.try_into()?,
Expand Down Expand Up @@ -392,11 +421,11 @@ macro_rules! compute_op {
let ll = $LEFT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
.expect("compute_op failed to downcast left side array");
let rr = $RIGHT
.as_any()
.downcast_ref::<$DT>()
.expect("compute_op failed to downcast array");
.expect("compute_op failed to downcast right side array");
Ok(Arc::new($OP(&ll, &rr)?))
}};
// invoke unary operator
Expand Down Expand Up @@ -540,8 +569,8 @@ macro_rules! binary_array_op {
DataType::UInt16 => compute_op!($LEFT, $RIGHT, $OP, UInt16Array),
DataType::UInt32 => compute_op!($LEFT, $RIGHT, $OP, UInt32Array),
DataType::UInt64 => compute_op!($LEFT, $RIGHT, $OP, UInt64Array),
DataType::Float32 => compute_op!($LEFT, $RIGHT, $OP, Float32Array),
DataType::Float64 => compute_op!($LEFT, $RIGHT, $OP, Float64Array),
DataType::Float32 => compute_f32_op!($LEFT, $RIGHT, $OP, Float32Array),
DataType::Float64 => compute_f64_op!($LEFT, $RIGHT, $OP, Float64Array),
DataType::Utf8 => compute_utf8_op!($LEFT, $RIGHT, $OP, StringArray),
DataType::Timestamp(TimeUnit::Nanosecond, _) => {
compute_op!($LEFT, $RIGHT, $OP, TimestampNanosecondArray)
Expand Down
130 changes: 108 additions & 22 deletions datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,22 +62,13 @@ pub(crate) fn is_distinct_from<T>(
where
T: ArrowNumericType,
{
let left_data = left.data();
let right_data = right.data();
let array_len = left_data.len().min(right_data.len());

let left_values = left.values();
let right_values = right.values();

let distinct = arrow_buffer::MutableBuffer::collect_bool(array_len, |i| {
left_data.is_null(i) != right_data.is_null(i) || left_values[i] != right_values[i]
});

let array_data = ArrayData::builder(arrow_schema::DataType::Boolean)
.len(array_len)
.add_buffer(distinct.into());

Ok(BooleanArray::from(unsafe { array_data.build_unchecked() }))
distinct(
left,
right,
|left_value, right_value, left_isnull, right_isnull| {
left_isnull != right_isnull || left_value != right_value
},
)
}

pub(crate) fn is_not_distinct_from<T>(
Expand All @@ -87,25 +78,120 @@ pub(crate) fn is_not_distinct_from<T>(
where
T: ArrowNumericType,
{
let left_data = left.data();
let right_data = right.data();
let array_len = left_data.len().min(right_data.len());
distinct(
left,
right,
|left_value, right_value, left_isnull, right_isnull| {
!(left_isnull != right_isnull || left_value != right_value)
},
)
}

fn distinct<
T,
F: FnMut(
<T as ArrowPrimitiveType>::Native,
<T as ArrowPrimitiveType>::Native,
bool,
bool,
) -> bool,
>(
left: &PrimitiveArray<T>,
right: &PrimitiveArray<T>,
mut op: F,
) -> Result<BooleanArray>
where
T: ArrowNumericType,
{
let left_values = left.values();
let right_values = right.values();
let left_data = left.data();
let right_data = right.data();

let array_len = left_data.len().min(right_data.len());
let distinct = arrow_buffer::MutableBuffer::collect_bool(array_len, |i| {
!(left_data.is_null(i) != right_data.is_null(i)
|| left_values[i] != right_values[i])
op(
left_values[i],
right_values[i],
left_data.is_null(i),
right_data.is_null(i),
)
});

let array_data = ArrayData::builder(arrow_schema::DataType::Boolean)
.len(array_len)
.add_buffer(distinct.into());

Ok(BooleanArray::from(unsafe { array_data.build_unchecked() }))
}

pub(crate) fn is_distinct_from_f32(
left: &Float32Array,
right: &Float32Array,
) -> Result<BooleanArray> {
distinct(
left,
right,
|left_value, right_value, left_isnull, right_isnull| {
left_isnull != right_isnull
|| left_value.is_nan() != right_value.is_nan()
|| (!left_value.is_nan()
&& !right_value.is_nan()
&& left_value != right_value)
},
)
}

pub(crate) fn is_not_distinct_from_f32(
left: &Float32Array,
right: &Float32Array,
) -> Result<BooleanArray> {
distinct(
left,
right,
|left_value, right_value, left_isnull, right_isnull| {
!(left_isnull != right_isnull
|| left_value.is_nan() != right_value.is_nan()
|| (!left_value.is_nan()
&& !right_value.is_nan()
&& left_value != right_value))
},
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me quite some time to see that the only difference here is that there is a ! in front of the entire expression. Would it be possible to factor out the duplication so the difference between the kernels are clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)
}

pub(crate) fn is_distinct_from_f64(
left: &Float64Array,
right: &Float64Array,
) -> Result<BooleanArray> {
distinct(
left,
right,
|left_value, right_value, left_isnull, right_isnull| {
left_isnull != right_isnull
|| left_value.is_nan() != right_value.is_nan()
|| (!left_value.is_nan()
&& !right_value.is_nan()
&& left_value != right_value)
},
)
}

pub(crate) fn is_not_distinct_from_f64(
left: &Float64Array,
right: &Float64Array,
) -> Result<BooleanArray> {
distinct(
left,
right,
|left_value, right_value, left_isnull, right_isnull| {
!(left_isnull != right_isnull
|| left_value.is_nan() != right_value.is_nan()
|| (!left_value.is_nan()
&& !right_value.is_nan()
&& left_value != right_value))
},
)
}

pub(crate) fn is_distinct_from_utf8<OffsetSize: OffsetSizeTrait>(
left: &GenericStringArray<OffsetSize>,
right: &GenericStringArray<OffsetSize>,
Expand Down