Decimal Mul/Div kernels - #9221
Open
mhk197 wants to merge 7 commits into
Open
Conversation
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Merging this PR will degrade performance by 0.68%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | compress_fsst[(10000, 64, 8)] |
9.6 ms | 10.8 ms | -10.92% |
| ⚡ | Simulation | chunked_dict_primitive_canonical_into[u32, (1000, 100, 100)] |
1.6 ms | 1.5 ms | +10.74% |
| 🆕 | Simulation | mul_decimal_i64_nonnull |
N/A | 1.5 ms | N/A |
| 🆕 | Simulation | div_decimal_i128_nullable |
N/A | 13.1 ms | N/A |
| 🆕 | Simulation | div_decimal_i64_nonnull |
N/A | 3.6 ms | N/A |
| 🆕 | Simulation | mul_decimal_i128_nullable |
N/A | 5.3 ms | N/A |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mk/decimal-mul-div (5087468) with mk/decimal-scalar-mul-div (a2ecde0)
Footnotes
-
43 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
mhk197
marked this pull request as ready for review
August 6, 2026 00:21
gatesn
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds native
MulandDivexecution for decimal arrays, completing the operator set that #8724 started with Add/Sub. Both operators were previously rejected withnumeric operator {op} is not yet supported for decimal arrays.Stacked on #8888 — base is
mk/decimal-scalar-mul-div, so review that one first. #8888 defines the Arrow decimal arithmetic rules (scalar::decimal_numeric_result_dtype) and implements them for scalars; this PR implements the array kernels against the same function rather than restating the formulas, so the scalar and array paths cannot drift:p + 1s2p + 12sp + s + 4s + 4Execution
Lanes run at a working width from
decimal_numeric_work_dtype, then narrow to the result's own storage width. The two widths are not always the same: Mul's intermediate is the result itself, but Div scales the dividend by10^result_scalebefore dividing and so needs room forp + |result_scale|digits. A negative result scale scales the divisor instead.DecimalOpConstants<W>hoists the per-execution constants — the result-precision bounds and the two division scale factors — out of the lane loop.Every lane is checked at the working width.
DecimalArraydoes not validate its stored values against the declared precision, so an out-of-precision value can reach a kernel and must not be able to overflow it;test_decimal_value_outside_working_width_errorshas depended on that behaviour since #8724. Overflowing the result precision on a valid lane is an error, as is division by zero; invalid lanes never error.Notes for review
checked_lanes/LaneZipAPI from Unify numeric compute kernels with vortex-compute iteration functions #8939 rather than the index-based loops this branch originally used, so the previousunsafe { get_unchecked }lane access is gone.GuaranteedDecimalMulfast path that skipped the checked multiply and the bounds compare when2p <= MAX_PRECISION. That was unsound — it assumed in-precision inputs, which nothing enforces — and is removed. It could return a value past the declared result precision, and could overflow the working width outright (panicking in debug, wrapping in release) on a valueDecimalArrayhappily stores. Both cases are pinned by regression tests.compute::conformance::binary_numeric) now exercises Mul and Div for every decimal encoding, not just Add/Sub.Checks
cargo nextest run -p vortex-array— 3200 passedcargo nextest run --workspace— 7408 passed, 560 skippedcargo clippy -p vortex-array --all-targetscargo +nightly fmt --all --check