Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
10 changes: 10 additions & 0 deletions datafusion/core/tests/sql/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,16 @@ async fn test_struct_literals() -> Result<()> {
Ok(())
}

#[tokio::test]
async fn binary_bitwise_shift() -> Result<()> {
test_expression!("2 << 10", "2048");
test_expression!("2048 >> 10", "2");
test_expression!("2048 << NULL", "NULL");
test_expression!("2048 >> NULL", "NULL");

Ok(())
}

#[tokio::test]
async fn test_interval_expressions() -> Result<()> {
// day nano intervals
Expand Down
16 changes: 11 additions & 5 deletions datafusion/expr/src/binary_rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ pub fn binary_operator_data_type(
| Operator::IsDistinctFrom
| Operator::IsNotDistinctFrom => Ok(DataType::Boolean),
// bitwise operations return the common coerced type
Operator::BitwiseAnd | Operator::BitwiseOr => Ok(result_type),
Operator::BitwiseAnd
| Operator::BitwiseOr
| Operator::BitwiseShiftLeft
| Operator::BitwiseShiftRight => Ok(result_type),
// math operations return the same value as the common coerced type
Operator::Plus
| Operator::Minus
Expand All @@ -76,9 +79,10 @@ pub fn coerce_types(
) -> Result<DataType> {
// This result MUST be compatible with `binary_coerce`
let result = match op {
Operator::BitwiseAnd | Operator::BitwiseOr => {
bitwise_coercion(lhs_type, rhs_type)
}
Operator::BitwiseAnd
| Operator::BitwiseOr
| Operator::BitwiseShiftRight
| Operator::BitwiseShiftLeft => bitwise_coercion(lhs_type, rhs_type),
Operator::And | Operator::Or => match (lhs_type, rhs_type) {
// logical binary boolean operators can only be evaluated in bools
(DataType::Boolean, DataType::Boolean) => Some(DataType::Boolean),
Expand Down Expand Up @@ -135,12 +139,14 @@ pub fn coerce_types(
fn bitwise_coercion(left_type: &DataType, right_type: &DataType) -> Option<DataType> {
use arrow::datatypes::DataType::*;

if !is_numeric(left_type) || !is_numeric(right_type) {
if !both_numeric_or_null_and_numeric(left_type, right_type) {
Copy link
Contributor Author

@ovr ovr Aug 14, 2022

Choose a reason for hiding this comment

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

image

To be similar with default SQL behaviour for null.

return None;
}

if left_type == right_type && !is_dictionary(left_type) {
return Some(left_type.clone());
}

// TODO support other data type
match (left_type, right_type) {
(Int64, _) | (_, Int64) => Some(Int64),
Expand Down
6 changes: 6 additions & 0 deletions datafusion/expr/src/operator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ pub enum Operator {
BitwiseAnd,
/// Bitwise or, like `|`
BitwiseOr,
/// Bitwise right, like `>>`
BitwiseShiftRight,
/// Bitwise right, like `<<`
BitwiseShiftLeft,
/// String concat
StringConcat,
}
Expand Down Expand Up @@ -101,6 +105,8 @@ impl fmt::Display for Operator {
Operator::IsNotDistinctFrom => "IS NOT DISTINCT FROM",
Operator::BitwiseAnd => "&",
Operator::BitwiseOr => "|",
Operator::BitwiseShiftRight => ">>",
Operator::BitwiseShiftLeft => "<<",
Operator::StringConcat => "||",
};
write!(f, "{}", display)
Expand Down
56 changes: 55 additions & 1 deletion datafusion/physical-expr/src/expressions/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ use arrow::compute::kernels::comparison::{

use adapter::{eq_dyn, gt_dyn, gt_eq_dyn, lt_dyn, lt_eq_dyn, neq_dyn};
use arrow::compute::kernels::concat_elements::concat_elements_utf8;
use kernels::{bitwise_and, bitwise_and_scalar, bitwise_or, bitwise_or_scalar};
use kernels::{
bitwise_and, bitwise_and_scalar, bitwise_or, bitwise_or_scalar, bitwise_shift_left,
bitwise_shift_left_scalar, bitwise_shift_right, bitwise_shift_right_scalar,
};
use kernels_arrow::{
add_decimal, add_decimal_scalar, divide_decimal, divide_decimal_scalar,
eq_decimal_scalar, gt_decimal_scalar, gt_eq_decimal_scalar, is_distinct_from,
Expand Down Expand Up @@ -740,6 +743,12 @@ impl BinaryExpr {
),
Operator::BitwiseAnd => bitwise_and_scalar(array, scalar.clone()),
Operator::BitwiseOr => bitwise_or_scalar(array, scalar.clone()),
Operator::BitwiseShiftRight => {
bitwise_shift_right_scalar(array, scalar.clone())
}
Operator::BitwiseShiftLeft => {
bitwise_shift_left_scalar(array, scalar.clone())
}
// if scalar operation is not supported - fallback to array implementation
_ => None,
};
Expand Down Expand Up @@ -850,6 +859,8 @@ impl BinaryExpr {
}
Operator::BitwiseAnd => bitwise_and(left, right),
Operator::BitwiseOr => bitwise_or(left, right),
Operator::BitwiseShiftRight => bitwise_shift_right(left, right),
Operator::BitwiseShiftLeft => bitwise_shift_left(left, right),
Operator::StringConcat => {
binary_string_array_op!(left, right, concat_elements)
}
Expand Down Expand Up @@ -2481,6 +2492,34 @@ mod tests {
Ok(())
}

#[test]
fn bitwise_shift_array_test() -> Result<()> {
let input = Arc::new(Int32Array::from(vec![Some(2), None, Some(10)])) as ArrayRef;
let modules =
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest a test for when the modules is Null (you cover NULL for the input already)

Arc::new(Int32Array::from(vec![Some(2), Some(4), Some(8)])) as ArrayRef;
let mut result = bitwise_shift_left(input.clone(), modules.clone())?;

let expected = Int32Array::from(vec![Some(8), None, Some(2560)]);
assert_eq!(result.as_ref(), &expected);

result = bitwise_shift_right(result.clone(), modules.clone())?;
assert_eq!(result.as_ref(), &input);

Ok(())
}

#[test]
fn bitwise_shift_array_overflow_test() -> Result<()> {
let input = Arc::new(Int32Array::from(vec![Some(2)])) as ArrayRef;
let modules = Arc::new(Int32Array::from(vec![Some(100)])) as ArrayRef;
let result = bitwise_shift_left(input.clone(), modules.clone())?;

let expected = Int32Array::from(vec![Some(32)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@liukun4515 liukun4515 Aug 13, 2022

Choose a reason for hiding this comment

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

2<<100 =>> 2<<(100%bit_width(i32)) =>> 2 << 4?

Copy link
Member

Choose a reason for hiding this comment

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

i am surprised that this is rotational shifting

Copy link
Contributor

Choose a reason for hiding this comment

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

i am surprised that this is rotational shifting

me too...

assert_eq!(result.as_ref(), &expected);

Ok(())
}

#[test]
fn bitwise_scalar_test() -> Result<()> {
let left = Arc::new(Int32Array::from(vec![Some(12), None, Some(11)])) as ArrayRef;
Expand All @@ -2494,4 +2533,19 @@ mod tests {
assert_eq!(result.as_ref(), &expected);
Ok(())
}

#[test]
fn bitwise_shift_scalar_test() -> Result<()> {
let input = Arc::new(Int32Array::from(vec![Some(2), None, Some(4)])) as ArrayRef;
let module = ScalarValue::from(10i32);
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, here a test for null handling might be good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of doing unit test here, I did a simple SQL test which test the whole pipeline, and I found that DF doesn't handle nulls for bitwise operators.

Fixed in:

810f4b0

let mut result = bitwise_shift_left_scalar(&input, module.clone()).unwrap()?;

let expected = Int32Array::from(vec![Some(2048), None, Some(4096)]);
assert_eq!(result.as_ref(), &expected);

result = bitwise_shift_right_scalar(&result, module).unwrap()?;
assert_eq!(result.as_ref(), &input);

Ok(())
}
}
Loading