[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3690: Allow precision loss on multiplying decimal arrays

["tustvold (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org