mbutrovich opened a new pull request, #11528: URL: https://github.com/apache/arrow-rs/pull/11528
# Which issue does this PR close? - Closes #11454. - Part of #11421. # Rationale for this change See #11454. # What changes are included in this PR? `ArrowNativeTypeOp` gains `div_rem_wrapping` and `div_rem_checked`, which return `(quotient, remainder)`. Their default implementations call `div_*` and `mod_*`. `i256` overrides them with `i256::wrapping_div_rem` and `i256::checked_div_rem` from #11425, and `div_rem_checked` returns the same errors as `div_checked`. `native_type_op!` takes optional trailing items so the `i256` impl can carry the overrides. The names follow the `*_wrapping` and `*_checked` suffixes of the other `ArrowNativeTypeOp` methods. `checked_div_rem` would share a name with `i256::checked_div_rem` but return `Result` instead of `Option`. `make_downscaler` and `scaled_div` call the new methods. `ArrowNativeType` is sealed, so adding trait methods with defaults doesn't break downstream code. # Are these changes tested? `test_i256_div_rem` and `test_i256_div_rem_wrapping_zero_panics` in `arrow-array/src/arithmetic.rs` check the `i256` overrides against `div_*` and `mod_*`, including the errors. The existing tests cover both callers with `Decimal256` and negative values, such as `test_cast_decimal_to_decimal_round` and `test_decimal256_div_wide_intermediate`. On an Apple M5 Max with rustc 1.99.0, the `decimal_downscale` cases of the `cast_decimal` benchmark from #11426 gave, in us per 8192 values with no nulls: | Case | `main` | This PR | |---|---|---| | `Decimal256` (76, 10) to (76, 2), 38 digits | 178.9 | 104.4 | | `Decimal256` (76, 10) to (76, 2), 60 digits | 272.1 | 154.0 | | `Decimal256` (41, 20) to `Decimal128` (38, 6), 30 digits | 263.0 | 153.4 | | `Decimal256` (76, 20) to `Decimal128` (38, 6), 48 digits, fallible | 460.4 | 294.9 | | `Decimal256` (76, 40) to `Decimal128` (38, 6), 42 digits, fallible | 380.5 | 227.7 | The cases with nulls and the strict casts changed by the same amount. `Decimal128` keeps the default methods, and its cases changed by 3% or less, within the difference between repeated runs. In the `decimal_division` benchmark from #11427, `decimal256_scalar/div_72_digits_by_4_digits`, which reaches `scaled_div`, went from 295 to 284 us per 1024 values, the mean of two alternating runs each. Most of its time goes to the per-digit loop and to the error that #11479 stops building. I'll run the benchmark bot on this PR for arm64 and x86_64 numbers. # Are there any significant user-facing changes? `ArrowNativeTypeOp` has two new methods with default implementations. ## 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]
