Skip to content

Commit

Permalink
Decimal multiply kernel should not cause precision loss
Browse files Browse the repository at this point in the history
  • Loading branch information
viirya committed Mar 24, 2023
1 parent 26b8377 commit 8effc59
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 10 deletions.
6 changes: 3 additions & 3 deletions datafusion/physical-expr/src/expressions/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4004,9 +4004,9 @@ mod tests {

// multiply: decimal array multiply int32 array
let expect = Arc::new(create_decimal_array(
&[Some(15129), None, Some(15006), Some(15376)],
21,
2,
&[Some(1512900), None, Some(1500600), Some(1537600)],
38,
4,
)) as ArrayRef;
apply_arithmetic_op(
&schema,
Expand Down
19 changes: 12 additions & 7 deletions datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ use arrow::compute::{
};
use arrow::datatypes::Decimal128Type;
use arrow::{array::*, datatypes::ArrowNumericType, downcast_dictionary_array};
use arrow_schema::DataType;
use arrow_schema::{DataType, DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE};
use datafusion_common::cast::as_decimal128_array;
use datafusion_common::{DataFusionError, Result};
use std::cmp::min;
use std::sync::Arc;

// Simple (low performance) kernels until optimized kernels are added to arrow
Expand Down Expand Up @@ -370,12 +371,15 @@ pub(crate) fn multiply_dyn_decimal(
left: &dyn Array,
right: &dyn Array,
) -> Result<ArrayRef> {
let (precision, scale) = get_precision_scale(left)?;

let divide = 10_i128.pow(scale as u32);
let (left_precision, left_scale) = get_precision_scale(left)?;
let (right_precision, right_scale) = get_precision_scale(right)?;
let product_precision = min(
left_precision + right_precision + 1,
DECIMAL128_MAX_PRECISION,
);
let product_scale = min(left_scale + right_scale, DECIMAL128_MAX_SCALE);
let array = multiply_dyn(left, right)?;
let array = divide_scalar_dyn::<Decimal128Type>(&array, divide)?;
decimal_array_with_precision_scale(array, precision, scale)
decimal_array_with_precision_scale(array, product_precision, product_scale)
}

pub(crate) fn divide_dyn_opt_decimal(
Expand Down Expand Up @@ -526,7 +530,8 @@ mod tests {
// multiply
let result = multiply_dyn_decimal(&left_decimal_array, &right_decimal_array)?;
let result = as_decimal128_array(&result)?;
let expect = create_decimal_array(&[Some(15), None, Some(15), Some(15)], 25, 3);
let expect =
create_decimal_array(&[Some(15129), None, Some(15006), Some(15252)], 38, 6);
assert_eq!(&expect, result);
let result = multiply_decimal_dyn_scalar(&left_decimal_array, 10)?;
let result = as_decimal128_array(&result)?;
Expand Down

0 comments on commit 8effc59

Please sign in to comment.