mbutrovich opened a new pull request, #11479: URL: https://github.com/apache/arrow-rs/pull/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_arithmetic` 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. -- 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]
