tustvold commented on code in PR #3690:
URL: https://github.com/apache/arrow-rs/pull/3690#discussion_r1103851353
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -1241,6 +1243,74 @@ pub fn multiply_dyn_checked(
}
}
+/// Perform `left * right` operation on two decimal arrays. If either left or
right value is
+/// null then the result is also null.
+///
+/// This performs decimal multiplication which allows precision loss if an
exact representation
+/// is not possible for the result. In the case, the result will be rounded.
+pub fn multiply_decimal(
+ left: &PrimitiveArray<Decimal128Type>,
+ right: &PrimitiveArray<Decimal128Type>,
+) -> Result<ArrayRef, ArrowError> {
+ let precision = left.precision();
+ let mut product_scale = left.scale() + right.scale();
+
+ math_checked_op(left, right, |a, b| {
+ let a = i256::from_i128(a);
+ let b = i256::from_i128(b);
+
+ a.checked_mul(b)
Review Comment:
FWIW this will be exceptionally slow, it currently performs two memory
allocations :sweat_smile:
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -1241,6 +1243,74 @@ pub fn multiply_dyn_checked(
}
}
+/// Perform `left * right` operation on two decimal arrays. If either left or
right value is
+/// null then the result is also null.
+///
+/// This performs decimal multiplication which allows precision loss if an
exact representation
+/// is not possible for the result. In the case, the result will be rounded.
+pub fn multiply_decimal(
+ left: &PrimitiveArray<Decimal128Type>,
+ right: &PrimitiveArray<Decimal128Type>,
+) -> Result<ArrayRef, ArrowError> {
+ let precision = left.precision();
+ let mut product_scale = left.scale() + right.scale();
+
+ math_checked_op(left, right, |a, b| {
Review Comment:
This might be a stupid question, I know very little about decimal
arithmetic, but if you are just going to truncate the result why do you need to
perform multiplication as the larger type?
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -1241,6 +1243,74 @@ pub fn multiply_dyn_checked(
}
}
+/// Perform `left * right` operation on two decimal arrays. If either left or
right value is
+/// null then the result is also null.
+///
+/// This performs decimal multiplication which allows precision loss if an
exact representation
+/// is not possible for the result. In the case, the result will be rounded.
+pub fn multiply_decimal(
+ left: &PrimitiveArray<Decimal128Type>,
+ right: &PrimitiveArray<Decimal128Type>,
+) -> Result<ArrayRef, ArrowError> {
+ let precision = left.precision();
+ let mut product_scale = left.scale() + right.scale();
+
+ math_checked_op(left, right, |a, b| {
+ let a = i256::from_i128(a);
+ let b = i256::from_i128(b);
+
+ a.checked_mul(b)
+ .map(|mut a| {
+ // Round the value if an exact representation is not possible.
+ // ref: java.math.BigDecimal#doRound
+ let mut digits = a.to_string().len() as i8;
Review Comment:
`log10` perhaps?
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -1241,6 +1243,74 @@ pub fn multiply_dyn_checked(
}
}
+/// Perform `left * right` operation on two decimal arrays. If either left or
right value is
+/// null then the result is also null.
+///
+/// This performs decimal multiplication which allows precision loss if an
exact representation
+/// is not possible for the result. In the case, the result will be rounded.
+pub fn multiply_decimal(
+ left: &PrimitiveArray<Decimal128Type>,
+ right: &PrimitiveArray<Decimal128Type>,
+) -> Result<ArrayRef, ArrowError> {
+ let precision = left.precision();
+ let mut product_scale = left.scale() + right.scale();
+
+ math_checked_op(left, right, |a, b| {
+ let a = i256::from_i128(a);
+ let b = i256::from_i128(b);
+
+ a.checked_mul(b)
+ .map(|mut a| {
+ // Round the value if an exact representation is not possible.
+ // ref: java.math.BigDecimal#doRound
+ let mut digits = a.to_string().len() as i8;
+ let mut diff = digits - (Decimal128Type::MAX_PRECISION as i8);
+
+ while diff > 0 {
+ let divisor = i256::from_i128(10).pow_wrapping(diff as
u32);
+ a = divide_and_round::<Decimal256Type>(a, divisor);
+ product_scale -= diff;
Review Comment:
I'm not sure this is correct, as it won't "recompute" the values that have
already been evaluated?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
