mbutrovich opened a new pull request, #11530: URL: https://github.com/apache/arrow-rs/pull/11530
# Which issue does this PR close? - Part of #11451. - Part of #11421. # Rationale for this change See #11451. This is its second item, the fast path for operands that fit in `i128`. The path applies where `i256` holds the product of two `Decimal128` values before a rescale, as in `multiply_fixed_point` and Comet's [`div_round_half_up`](https://github.com/apache/datafusion-comet/blob/8d0bb01a5a99924024e07c1722e6eddd9260e167/native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs#L123-L146), and to `Decimal256` columns whose values have at most 38 digits. # What changes are included in this PR? `i256::div_rem` divides the magnitudes with one `u128` division when both are below 2^128, and with the long division otherwise. The checks before it and the sign handling after it don't change. This covers every pair of operands that fits in `i128`, and `i128::MIN / -1` needs no special case because the magnitudes are taken first. # Are these changes tested? `test_div_rem_powers_of_ten` and `test_i256_fuzz` in `arrow-buffer/src/bigint/mod.rs` cover both sides of the new boundary, with dividends such as `i256::from_parts(u128::MAX, 0)` and `i256::from_parts(0, 1)`, both signs, and every power of ten up to 10^76 as the divisor. These numbers are from an Apple M5 Max with rustc 1.99.0 and criterion with a 1 s warm-up and 2 s of measurement. #11528 is applied on both sides, because it changes how often `make_downscaler` calls `div_rem`. Each column is a build setting, and each value is the speedup over `main` with no nulls: | Case | Default profile | Thin LTO, 1 codegen unit | Fat LTO, 1 codegen unit | |---|---|---|---| | `cast_decimal`: `Decimal256` (76, 10) to (76, 2), 38 digits | 1.64x | 1.33x | 1.33x | | `cast_decimal`: `Decimal256` (41, 20) to `Decimal128` (38, 6), 30 digits | 1.62x | 1.48x | 1.44x | | `decimal_division`: `decimal256_scalar/div_30_digits_by_4_digits` | 1.17x | 1.18x | 1.17x | | `decimal_division`: `decimal256_scalar/rem_30_digits_by_4_digits` | 1.18x | 1.17x | 1.17x | | `cast_decimal`: `Decimal256` (76, 10) to (76, 2), 60 digits | 0.97x | 1.02x | 1.00x | | `cast_decimal`: 48-digit fallible casts | 0.97x to 0.98x | 1.00x to 1.01x | 1.00x to 1.01x | | `cast_decimal`: 42-digit fallible casts | 0.96x to 0.97x | 0.99x to 1.11x | 0.94x to 0.96x | | `decimal_division`: `div` and `rem` of 60 digits by 4 digits | 0.98x, 0.99x | 0.93x, 0.93x | 0.93x, 0.92x | | `decimal_division`: `div` and `rem` of 60 digits by 40 digits | 1.00x, 1.00x | 0.99x, 1.00x | 0.97x, 0.97x | | `i256`: `i256_div_rem` | 0.98x | 1.00x | 0.99x | The first four cases take the new path. The thin LTO baseline of the 38-digit cast without nulls was disturbed, so that cell uses the case with nulls, which costs the same. Wide values pay for the check before the long division. With LTO that costs about 2 ns per division, which shows only when the divisor fits in 64 bits and the long division is short, as in the 60-digit by 4-digit cases. Forcing loop alignment with `-C llvm-args=-align-loops=64` didn't change it. The precomputed divisors in #11421 would take `Decimal256` `/` and `%` by a scalar off this path. `multiply_fixed_point` also takes the new path, but #11526 changes its per-row divisions, so its numbers are left out until that 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]
