mbutrovich opened a new pull request, #12080: URL: https://github.com/apache/arrow-rs/pull/12080
# Which issue does this PR close? - Closes #11529. - Part of #11421. # Rationale for this change See #11529. This PR takes the narrower option from the issue and changes only the `decimal_op` closures, because moving the error out of line in `ArrowNativeTypeOp` slowed integer division with nulls. # What changes are included in this PR? - `div` and `rem` of a decimal array by a valid scalar scale the scalar once. When the scaled divisor fits and is neither 0 nor -1, no value can fail on the divisor, so the closure has no per-value checks of it. In a release build for aarch64, LLVM then inlines the closure into the loop of `try_unary` for `Decimal32`, `Decimal64` and `Decimal128`. The `Decimal256` closure is still called out of line, and `i256` division dominates its cost. - A null scalar, a scalar on the left, a divisor of 0 or -1, and a divisor that overflows when scaled take the existing code, so results and errors don't change. - `rem` builds the error for a value that overflows when scaled in a `#[cold]` function. - 98ff083356 adds `rem_rescaled` cases to the `decimal_division` benchmark, so the benchmark bot can compare against it. In those cases the divisor has a larger scale than the values, so `rem` scales each value by 100. # Are these changes tested? `test_decimal_div_rem_by_scalar` in `arrow-arith/src/numeric.rs` compares division by a scalar with division by an array that repeats it, which takes `try_binary`, for every decimal type. Bot runs against 98ff083356: [Neoverse-V2](), [Xeon](), and [Xeon with `-C target-cpu=x86-64-v3`](). <details><summary>Local times in us per 1024 values</summary> These are from an Apple M5 Max with rustc 1.99.1, the default bench profile, `-C llvm-args=-align-all-functions=7 -C llvm-args=-align-all-nofallthru-blocks=7` on both sides, and criterion with a 1 s warm-up and 3 s of measurement. Each row is the first `no_nulls` case of its group. The `mixed_nulls` and `rem_rescaled` cases changed by about as much. | Case | `main` | This PR | Speedup | |---|---|---|---| | `decimal32_scalar/div_3_digits_by_4_digits` | 1.71 | 0.59 | 2.9x | | `decimal32_scalar/rem_9_digits_by_4_digits` | 1.64 | 0.55 | 3.0x | | `decimal64_scalar/div_3_digits_by_4_digits` | 1.69 | 0.65 | 2.6x | | `decimal64_scalar/rem_18_digits_by_4_digits` | 1.68 | 0.58 | 2.9x | | `decimal128_scalar/div_10_digits_by_4_digits` | 3.41 | 2.91 | 1.2x | | `decimal128_scalar/rem_10_digits_by_4_digits` | 4.02 | 2.10 | 1.9x | | `decimal256_scalar/div_30_digits_by_4_digits` | 19.9 | 12.5 | 1.6x | | `decimal256_scalar/rem_30_digits_by_4_digits` | 18.8 | 12.5 | 1.5x | `main` varied from 3.4 to 6.5 us on `Decimal128` `div` across runs of the same binary, while this PR stayed at 2.9 to 3.0 us. The rows above use `main`'s fastest run. The `div` cases whose values overflow when scaled, such as `div_6_digits_by_4_digits` for `Decimal32`, didn't change, because `scaled_div` dominates them. </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. -- 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]
