From c38c0a644a668085f34cc63b85eaaaa688ce3c02 Mon Sep 17 00:00:00 2001 From: "mingmwang@ebay.com" Date: Thu, 11 May 2023 12:58:02 +0800 Subject: [PATCH 1/3] refine decimal multiply, avoid cast to wider type --- datafusion/expr/src/type_coercion/binary.rs | 21 +++++++++---------- .../src/expressions/binary/kernels_arrow.rs | 18 +--------------- 2 files changed, 11 insertions(+), 28 deletions(-) diff --git a/datafusion/expr/src/type_coercion/binary.rs b/datafusion/expr/src/type_coercion/binary.rs index fa912b777ad73..962434d65233d 100644 --- a/datafusion/expr/src/type_coercion/binary.rs +++ b/datafusion/expr/src/type_coercion/binary.rs @@ -514,7 +514,7 @@ pub fn coercion_decimal_mathematics_type( left_decimal_type, right_decimal_type, ), - Operator::Multiply | Operator::Divide | Operator::Modulo => { + Operator::Divide | Operator::Modulo => { get_wider_decimal_type(left_decimal_type, right_decimal_type) } _ => None, @@ -946,7 +946,7 @@ mod tests { &left_decimal_type, &right_decimal_type, ); - assert_eq!(DataType::Decimal128(20, 4), result.unwrap()); + assert_eq!(None, result); let result = decimal_op_mathematics_type(&op, &left_decimal_type, &right_decimal_type); assert_eq!(DataType::Decimal128(31, 7), result.unwrap()); @@ -1232,7 +1232,7 @@ mod tests { mathematics_op: Operator, expected_lhs_type: Option, expected_rhs_type: Option, - expected_coerced_type: DataType, + expected_coerced_type: Option, expected_output_type: DataType, ) { // The coerced types for lhs and rhs, if any of them is not decimal @@ -1245,8 +1245,7 @@ mod tests { // The coerced type of decimal math expression, applied during expression evaluation let coerced_type = - coercion_decimal_mathematics_type(&mathematics_op, &lhs_type, &rhs_type) - .unwrap(); + coercion_decimal_mathematics_type(&mathematics_op, &lhs_type, &rhs_type); assert_eq!(coerced_type, expected_coerced_type); // The output type of decimal math expression @@ -1263,7 +1262,7 @@ mod tests { Operator::Plus, None, None, - DataType::Decimal128(11, 2), + Some(DataType::Decimal128(11, 2)), DataType::Decimal128(11, 2), ); @@ -1273,7 +1272,7 @@ mod tests { Operator::Plus, Some(DataType::Decimal128(10, 0)), None, - DataType::Decimal128(13, 2), + Some(DataType::Decimal128(13, 2)), DataType::Decimal128(13, 2), ); @@ -1283,7 +1282,7 @@ mod tests { Operator::Minus, Some(DataType::Decimal128(10, 0)), None, - DataType::Decimal128(13, 2), + Some(DataType::Decimal128(13, 2)), DataType::Decimal128(13, 2), ); @@ -1293,7 +1292,7 @@ mod tests { Operator::Multiply, Some(DataType::Decimal128(10, 0)), None, - DataType::Decimal128(12, 2), + None, DataType::Decimal128(21, 2), ); @@ -1303,7 +1302,7 @@ mod tests { Operator::Divide, Some(DataType::Decimal128(10, 0)), None, - DataType::Decimal128(12, 2), + Some(DataType::Decimal128(12, 2)), DataType::Decimal128(23, 11), ); @@ -1313,7 +1312,7 @@ mod tests { Operator::Modulo, Some(DataType::Decimal128(10, 0)), None, - DataType::Decimal128(12, 2), + Some(DataType::Decimal128(12, 2)), DataType::Decimal128(10, 2), ); diff --git a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs index 2852d617bf13a..765588593c62b 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs @@ -469,24 +469,8 @@ pub(crate) fn multiply_decimal_dyn_scalar( result_type: &DataType, ) -> Result { let (precision, scale) = get_precision_scale(result_type)?; - - let op_type = decimal_op_mathematics_type( - &Operator::Multiply, - left.data_type(), - left.data_type(), - ) - .unwrap(); - let (_, op_scale) = get_precision_scale(&op_type)?; - let array = multiply_scalar_dyn::(left, right)?; - - if op_scale > scale { - let div = 10_i128.pow((op_scale - scale) as u32); - let array = divide_scalar_dyn::(&array, div)?; - decimal_array_with_precision_scale(array, precision, scale) - } else { - decimal_array_with_precision_scale(array, precision, scale) - } + decimal_array_with_precision_scale(array, precision, scale) } pub(crate) fn divide_decimal_dyn_scalar( From 61c8bb21fd7302d7c3218c1e4c0fc764dd406718 Mon Sep 17 00:00:00 2001 From: "mingmwang@ebay.com" Date: Thu, 11 May 2023 13:29:21 +0800 Subject: [PATCH 2/3] fix clippy --- .../physical-expr/src/expressions/binary/kernels_arrow.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs index 765588593c62b..e856f20a5be2d 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs @@ -35,9 +35,7 @@ use arrow_schema::DataType; use datafusion_common::cast::{as_date32_array, as_date64_array, as_decimal128_array}; use datafusion_common::scalar::{date32_add, date64_add}; use datafusion_common::{DataFusionError, Result, ScalarValue}; -use datafusion_expr::type_coercion::binary::decimal_op_mathematics_type; use datafusion_expr::ColumnarValue; -use datafusion_expr::Operator; use std::cmp::min; use std::sync::Arc; @@ -686,7 +684,9 @@ pub(crate) fn modulus_decimal_dyn_scalar( #[cfg(test)] mod tests { + use datafusion_expr::Operator; use super::*; + use datafusion_expr::type_coercion::binary::decimal_op_mathematics_type; fn create_decimal_array( array: &[Option], From b021d49fd267b5d8a0dcdb9a37b4c76c04afc355 Mon Sep 17 00:00:00 2001 From: "mingmwang@ebay.com" Date: Thu, 11 May 2023 13:34:30 +0800 Subject: [PATCH 3/3] fix fmt --- .../physical-expr/src/expressions/binary/kernels_arrow.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs index e856f20a5be2d..90fca17157886 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs @@ -684,9 +684,9 @@ pub(crate) fn modulus_decimal_dyn_scalar( #[cfg(test)] mod tests { - use datafusion_expr::Operator; use super::*; use datafusion_expr::type_coercion::binary::decimal_op_mathematics_type; + use datafusion_expr::Operator; fn create_decimal_array( array: &[Option],