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-11479-9706742e65d531a13bf561aa9b638f6a6bc6a517 in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit 07508da83b9d909ab2e96f7249a48f41166d35fd Author: Matt Butrovich <[email protected]> AuthorDate: Tue Oct 13 20:57:55 2026 +0000 perf(arrow-arith): don't build an error for each decimal `div` value that overflows when scaled (#11479) # Which issue does this PR close? - Part of #11421. # Rationale for this change @andygrove [found](https://github.com/apache/arrow-rs/pull/11427#discussion_r4232910930) that decimal `div` builds an error for every value whose scaled dividend overflows, and then discards it. `decimal_op` scales each dividend with [`mul_checked`](https://github.com/apache/arrow-rs/blob/c07655f8e986834f2aebf9de46ba1c02ac58d32a/arrow-arith/src/numeric.rs#L1141-L1143), which formats an `ArrowError` message on overflow, and the `Err(_)` arm falls back to `scaled_div` without using it. In the overflow cases of the #11427 benchmarks, the formatting took about half the time. The #11421 kernel PRs will report before and after numbers from these cases, so this cost should be gone first. # What changes are included in this PR? `decimal_op` scales the dividend with `num_traits::CheckedMul::checked_mul`, which returns `None` on overflow, so the fallback to `scaled_div` no longer builds an error. The arithmetic and the results are the same. The `CheckedMul` impl for `i256` gains `#[inline]`. It is not generic, so without the attribute `arrow-arith` called it out of line, which made the `Decimal256` cases that don't overflow 2% to 6% slower than `main`. The inherent `i256::checked_mul` it wraps, which `mul_checked` calls, is already `#[inline]`. # Are these changes tested? A new test divides values on each side of the boundary where scaling by 10^6 overflows, for all four decimal types and both divisor signs, so each value takes the direct path or the `scaled_div` fallback. The test passes on `main` and with this change. This depends on the benchmarks in #11427. On an Apple M5 Max, with the `decimal_division` benchmark from #11427 added locally, in us per call: | Type | Case | Rows | `main` | This PR | Ratio | |---|---|---|---|---|---| | `Decimal32` | 3 by 4 digits | 1024, no nulls | 2.15 | 1.69 | 0.78x | | `Decimal32` | 3 by 4 digits | 1024, every fifth null | 1.84 | 1.65 | 0.90x | | `Decimal32` | 6 by 4 digits, overflows when scaled | 1024, no nulls | 68.94 | 37.06 | 0.54x | | `Decimal64` | 3 by 4 digits | 1024, no nulls | 2.02 | 1.62 | 0.80x | | `Decimal64` | 3 by 4 digits | 1024, every fifth null | 1.72 | 1.52 | 0.88x | | `Decimal64` | 12 by 4 digits | 1024, no nulls | 2.00 | 1.62 | 0.81x | | `Decimal64` | 15 by 4 digits, overflows when scaled | 1024, no nulls | 70.54 | 37.46 | 0.53x | | `Decimal128` | 10 by 4 digits | 1024, no nulls | 4.16 | 3.52 | 0.85x | | `Decimal128` | 10 by 4 digits | 1024, every fifth null | 3.59 | 3.12 | 0.87x | | `Decimal128` | 30 by 4 digits | 1024, no nulls | 5.61 | 5.32 | 0.95x | | `Decimal128` | 30 by 21 digits | 1024, no nulls | 9.45 | 9.22 | 0.98x | | `Decimal128` | 34 by 4 digits, overflows when scaled | 1024, no nulls | 124.78 | 68.35 | 0.55x | | `Decimal256` | 30 by 4 digits | 1024, no nulls | 22.53 | 22.07 | 0.98x | | `Decimal256` | 30 by 4 digits | 1024, every fifth null | 18.28 | 18.04 | 0.99x | | `Decimal256` | 60 by 4 digits | 1024, no nulls | 31.82 | 30.52 | 0.96x | | `Decimal256` | 60 by 40 digits | 1024, no nulls | 49.86 | 48.80 | 0.98x | | `Decimal256` | 72 by 4 digits, overflows when scaled | 1024, no nulls | 294.44 | 133.18 | 0.45x | <details><summary>Small arrays</summary> | Type | Rows | `main` | This PR | Ratio | |---|---|---|---|---| | `Decimal32` | 128 | 0.32 | 0.27 | 0.84x | | `Decimal32` | 16 | 0.08 | 0.08 | 0.95x | | `Decimal32` | 1 | 0.045 | 0.045 | 1.01x | | `Decimal64` | 128 | 0.30 | 0.25 | 0.82x | | `Decimal64` | 16 | 0.07 | 0.07 | 0.99x | | `Decimal64` | 1 | 0.045 | 0.045 | 1.00x | | `Decimal128` | 128 | 0.58 | 0.49 | 0.85x | | `Decimal128` | 16 | 0.11 | 0.11 | 0.96x | | `Decimal128` | 1 | 0.048 | 0.047 | 0.98x | | `Decimal256` | 128 | 2.88 | 2.85 | 0.99x | | `Decimal256` | 16 | 0.41 | 0.42 | 1.01x | | `Decimal256` | 1 | 0.078 | 0.075 | 0.97x | </details> Once #11427 merges, I'll run the benchmark bot on this PR for arm64 and x86_64 numbers. # 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: Jeffrey Vo <[email protected]> --- arrow-arith/src/numeric.rs | 48 +++++++++++++++++++++++++++++++++++++----- arrow-buffer/src/bigint/mod.rs | 1 + 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/arrow-arith/src/numeric.rs b/arrow-arith/src/numeric.rs index d1d0c6435c..ea2a51a6e4 100644 --- a/arrow-arith/src/numeric.rs +++ b/arrow-arith/src/numeric.rs @@ -28,7 +28,7 @@ use arrow_array::types::*; use arrow_array::*; use arrow_buffer::{ArrowNativeType, IntervalDayTime, IntervalMonthDayNano}; use arrow_schema::{ArrowError, DataType, IntervalUnit, TimeUnit}; -use num_traits::ToPrimitive; +use num_traits::{CheckedMul, ToPrimitive}; use crate::arity::{binary, try_binary}; @@ -1026,7 +1026,10 @@ fn decimal_op<T: DecimalType>( l_s: bool, r: &dyn Array, r_s: bool, -) -> Result<ArrayRef, ArrowError> { +) -> Result<ArrayRef, ArrowError> +where + T::Native: CheckedMul, +{ let l = l.as_primitive::<T>(); let r = r.as_primitive::<T>(); @@ -1138,9 +1141,9 @@ fn decimal_op<T: DecimalType>( l_s, r, r_s, - match l.mul_checked(l_mul) { - Ok(scaled) => scaled.div_checked(r.mul_checked(r_mul)?), - Err(_) => scaled_div::<T>(l, r, mul_pow), + match l.checked_mul(&l_mul) { + Some(scaled) => scaled.div_checked(r.mul_checked(r_mul)?), + None => scaled_div::<T>(l, r, mul_pow), } ) .with_precision_and_scale(result_precision, result_scale)? @@ -1719,6 +1722,41 @@ mod tests { check::<Decimal256Type>(); } + #[test] + fn test_decimal_div_scaling_overflow_boundary() { + // `div` scales `l` by 10^6, which overflows just past `MAX / 10^6` and `MIN / 10^6`. + // Dividing by 10^6 returns each value, on either side of the boundary. + fn check<T: DecimalType>() { + let one = T::Native::ONE; + let pow = T::Native::usize_as(1_000_000); + let max = T::Native::MAX_TOTAL_ORDER.div_wrapping(pow); + let min = T::Native::MIN_TOTAL_ORDER.div_wrapping(pow); + let values = [ + max, + max.add_wrapping(one), + min, + min.sub_wrapping(one), + T::Native::MAX_TOTAL_ORDER, + T::Native::MIN_TOTAL_ORDER.add_wrapping(one), + ]; + let a = PrimitiveArray::<T>::from_iter_values(values) + .with_precision_and_scale(T::MAX_PRECISION, 2) + .unwrap(); + for (divisor, negate) in [(pow, false), (pow.neg_wrapping(), true)] { + let b = PrimitiveArray::<T>::from_iter_values([divisor]) + .with_precision_and_scale(T::MAX_PRECISION, 2) + .unwrap(); + let result = div(&a, &Scalar::new(b)).unwrap(); + let expected = values.map(|v| if negate { v.neg_wrapping() } else { v }); + assert_eq!(result.as_primitive::<T>().values().as_ref(), expected); + } + } + check::<Decimal32Type>(); + check::<Decimal64Type>(); + check::<Decimal128Type>(); + check::<Decimal256Type>(); + } + #[test] fn test_decimal256_div_wide_intermediate() { // Dividing two scale-37 values needs l * 10^41, which is 79 digits and does not diff --git a/arrow-buffer/src/bigint/mod.rs b/arrow-buffer/src/bigint/mod.rs index d9c16087d5..823ec7d28d 100644 --- a/arrow-buffer/src/bigint/mod.rs +++ b/arrow-buffer/src/bigint/mod.rs @@ -1203,6 +1203,7 @@ impl CheckedDiv for i256 { } impl CheckedMul for i256 { + #[inline] fn checked_mul(&self, v: &i256) -> Option<Self> { (*self).checked_mul(*v) }
