This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-11526-bece8ca54ed1f776b2b3a1c4cb663ac4f8dd83dc in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit ebd7a591d27aa6895aef7ff096795b03ffbee291 Author: Matt Butrovich <[email protected]> AuthorDate: Wed Oct 14 01:35:59 2026 +0000 perf(arrow-arith): divide once per row in `multiply_fixed_point` (#11526) # Which issue does this PR close? - Closes #11453. - Part of #11421. # Rationale for this change See #11453. # What changes are included in this PR? `divide_and_round` takes the divisor and returns a closure that computes `half` and `half_neg` once and divides each row with `i256::wrapping_div_rem` from #11425. `multiply_fixed_point` and `multiply_fixed_point_checked` build the closure before calling `binary` and `try_binary`. The results don't change. # Are these changes tested? `test_decimal_multiply_fixed_point_rounding` in `arrow-arith/src/arithmetic.rs` covers rounding of negative products, ties and both null paths, which the existing tests of `multiply_fixed_point` and `multiply_fixed_point_checked` don't reach. It passes on `main` and with this change. The benchmark bot ran the `decimal128_multiply_fixed_point` cases of the `decimal_division` benchmark from #11427 on [x86_64](https://github.com/apache/arrow-rs/pull/11526#issuecomment-6129725592) (Xeon Platinum 8581C) and [arm64](https://github.com/apache/arrow-rs/pull/11526#issuecomment-6129729047) (Neoverse-V2). <img width="1600" height="740" alt="multiply-fixed-point-bot" src="https://github.com/user-attachments/assets/cca8f76e-27c3-4427-8c41-859cd635c03b"; /> <details><summary>Numbers behind the chart</summary> In us per 1024 values: | Case | x86_64: `main` | This PR | arm64: `main` | This PR | |---|---|---|---|---| | 15 digits, scale 10 to scale 6 | 45.2 | 16.4 | 73.2 | 29.8 | | 24 digits, scale 10 to scale 6 | 44.7 | 15.3 | 104.6 | 45.0 | | 21 digits, scale 20 to scale 6 | 84.1 | 37.0 | 96.2 | 40.4 | </details> <details><summary>Apple M5 Max</summary> With rustc 1.99.0, in us per 1024 values: | Case | `main` | This PR | |---|---|---| | 15 digits, scale 10 to scale 6 | 43.6 | 19.1 | | 24 digits, scale 10 to scale 6 | 61.1 | 29.4 | | 21 digits, scale 20 to scale 6 | 53.0 | 23.5 | </details> # Are there any significant user-facing changes? No. ## AI Disclosure I wrote this PR with help from Claude but understand and support the core approach, implementation, and test coverage. Co-authored-by: Andrew Lamb <[email protected]> --- arrow-arith/src/arithmetic.rs | 73 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 59 insertions(+), 14 deletions(-) diff --git a/arrow-arith/src/arithmetic.rs b/arrow-arith/src/arithmetic.rs index 170bd203dc..c8ac46d041 100644 --- a/arrow-arith/src/arithmetic.rs +++ b/arrow-arith/src/arithmetic.rs @@ -25,7 +25,6 @@ use crate::arity::*; use arrow_array::types::*; use arrow_array::*; -use arrow_buffer::ArrowNativeType; use arrow_buffer::i256; use arrow_schema::*; use std::cmp::min; @@ -114,12 +113,13 @@ pub fn multiply_fixed_point_checked( .with_precision_and_scale(precision, required_scale); } + let round = divide_and_round(divisor); let product = try_binary::<_, _, _, Decimal128Type>(left, right, |a, b| { let a = i256::from_i128(a); let b = i256::from_i128(b); let mut mul = a.wrapping_mul(b); - mul = divide_and_round::<Decimal256Type>(mul, divisor); + mul = round(mul); mul.to_i128().ok_or_else(|| { ArrowError::ArithmeticOverflow(format!("Overflow happened on: {a:?} * {b:?}")) }) @@ -158,30 +158,32 @@ pub fn multiply_fixed_point( .with_precision_and_scale(precision, required_scale); } + let round = divide_and_round(divisor); let product = binary::<_, _, _, Decimal128Type>(left, right, |a, b| { let a = i256::from_i128(a); let b = i256::from_i128(b); let mut mul = a.wrapping_mul(b); - mul = divide_and_round::<Decimal256Type>(mul, divisor); + mul = round(mul); mul.as_i128() })?; product.with_precision_and_scale(precision, required_scale) } -/// Divide a decimal native value by given divisor and round the result. -fn divide_and_round<I: DecimalType>(input: I::Native, div: I::Native) -> I::Native { - let d = input.div_wrapping(div); - let r = input.mod_wrapping(div); - - let half = div.div_wrapping(I::Native::from_usize(2).unwrap()); +/// Returns a function that divides a value by `div` and rounds the result half away from zero +fn divide_and_round(div: i256) -> impl Fn(i256) -> i256 { + let half = div.div_wrapping(i256::from_i128(2)); let half_neg = half.neg_wrapping(); - // Round result - match input >= I::Native::ZERO { - true if r >= half => d.add_wrapping(I::Native::ONE), - false if r <= half_neg => d.sub_wrapping(I::Native::ONE), - _ => d, + move |input| { + let (d, r) = input.wrapping_div_rem(div); + + // Round result + match input >= i256::ZERO { + true if r >= half => d.add_wrapping(i256::ONE), + false if r <= half_neg => d.sub_wrapping(i256::ONE), + _ => d, + } } } @@ -340,4 +342,47 @@ mod tests { "1234567890.0000000000000000000000000000" ); } + + #[test] + fn test_decimal_multiply_fixed_point_rounding() { + // Products below, at and above half for each sign, rounded from scale 1 to scale 0 + let a = Decimal128Array::from(vec![ + Some(14), + Some(15), + Some(16), + Some(-14), + Some(-15), + Some(-16), + Some(25), + Some(-25), + None, + ]) + .with_precision_and_scale(38, 1) + .unwrap(); + let b = Decimal128Array::from(vec![1, 1, 1, 1, 1, 1, -1, -1, 1]) + .with_precision_and_scale(38, 0) + .unwrap(); + let expected = Decimal128Array::from(vec![ + Some(1), + Some(2), + Some(2), + Some(-1), + Some(-2), + Some(-2), + Some(-3), + Some(3), + None, + ]) + .with_precision_and_scale(38, 0) + .unwrap(); + + // The slice without the null covers the paths without nulls + for (a, b, expected) in [ + (a.clone(), b.clone(), expected.clone()), + (a.slice(0, 8), b.slice(0, 8), expected.slice(0, 8)), + ] { + assert_eq!(multiply_fixed_point(&a, &b, 0).unwrap(), expected); + assert_eq!(multiply_fixed_point_checked(&a, &b, 0).unwrap(), expected); + } + } }
