Skip to content

Add native decimal arithmetic kernels#8722

Closed
mhk197 wants to merge 4 commits into
mk/decimal-scalar-fixed-pointfrom
mk/decimal-arithmetic
Closed

Add native decimal arithmetic kernels#8722
mhk197 wants to merge 4 commits into
mk/decimal-scalar-fixed-pointfrom
mk/decimal-arithmetic

Conversation

@mhk197

@mhk197 mhk197 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Adds array-level Add/Sub/Mul/Div over decimal arrays. Previously decimal arithmetic was rejected at two gates (Binary::return_dtype requires a primitive lhs; execute_numeric starts with PType::try_from), and there is no Arrow fallback — it simply errored.

Stacked on #8721 (the scalar Mul/Div fix), which the conformance tests compare against.

Semantics: same-dtype fixed-point, truncating toward zero

Both operands must share one DecimalDType (as compare requires since #8661; coerce_args/least_supertype already coerce to a common decimal dtype), and the result keeps that dtype with unioned nullability. On stored integers at scale s, precision p:

Op Stored result
Add/Sub a ± b, checked
Mul a × b rescaled from 2s back to s, truncating toward zero
Div (a · 10^s) / b (s < 0: a / (b · 10^{-s})), integer truncation

plus a fits_in_precision range check. Overflow, precision violations, and division by zero error only on valid lanes; invalid lanes never error — matching the primitive kernel. SQL-style type-growth semantics (DataFusion/Spark) compose on top of these kernels via input casts in the engine conversion layers, planned as a follow-up that needs no kernel change.

Implementation

  • numeric.rs is split into numeric/{mod,primitive,decimal,tests}.rs, mirroring the compare split in Implement compare in vortex instead of falling back to arrow #8661. The generic checked lane loops move to numeric/mod.rs with bounds relaxed to T: Default so i256 qualifies.
  • numeric/decimal.rs follows compare/decimal.rs: operands canonicalize through execute::<DecimalArray> (so Chunked, DecimalByteParts, Dict, … work with no kernel registration) or extract constants, then widen once to a working type and run one-pass checked lanes.
  • The working width is chosen per-op so in-precision inputs cannot spuriously overflow intermediates: Add/Sub stay at the input width (the precision check subsumes width overflow), Mul needs 2p digits (+|s| for negative scales), Div p + |s|, capped at i256. Near the 76-digit cap a raw product that overflows i256 errors even if the rescaled result would fit — documented limitation, only reachable for p > 38 Mul.
  • The result keeps the working width; DecimalData explicitly supports wider-than-necessary storage, and this avoids a narrowing pass. Happy to add a BigCast narrowing pass if preferred.
  • The binary-numeric conformance harness now accepts decimals, checking array results element-wise against DecimalScalar::checked_binary_numeric for constants 0, 1, -1, 10^s, MAX_BY_PRECISION[p] in both operand orders; wired into the decimal, chunked, and decimal-byte-parts compute tests.
  • widened_buffer moves from compare/decimal.rs to arrays/decimal so compare and numeric share it.

Testing

  • Unit tests: truncation toward zero (incl. negatives), mixed storage widths, null-lane error suppression, precision-stricter-than-width (60+60 at p=2 in i8), constants on either side (incl. nullable/null/out-of-width constants), i256 working width, i256-intermediate-overflow at p=76, negative scales, empty, constant folding.
  • Conformance fixtures: 8 decimal cases + chunked-decimal + 3 DecimalByteParts cases.
  • Benchmarks: add_decimal_i64, add_decimal_i128_nullable, mul_decimal_i64 (widens to i128), div_decimal_i128_constant in binary_ops.rs.
  • cargo nextest run -p vortex-array (3130 passed), -p vortex-decimal-byte-parts (25 passed), cargo clippy --all-targets --all-features, cargo test --doc -p vortex-array, cargo +nightly fmt --all.

🤖 Generated with Claude Code

mhk197 added 4 commits July 10, 2026 10:33
Mechanical move mirroring the compare split: dtype-generic lane machinery
stays in numeric/mod.rs (bounds relaxed to T: Default so wider decimal types
can reuse it), primitive-specific code moves to numeric/primitive.rs, tests
to numeric/tests.rs.

Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Add/Sub/Mul/Div over decimal arrays sharing a decimal dtype, executed as
fixed-point arithmetic at the shared scale (truncating toward zero) in a
working width wide enough that in-precision inputs cannot spuriously overflow
intermediates. Overflow, precision violations, and division by zero error
only on valid lanes, matching primitive semantics. widened_buffer moves to
arrays/decimal so compare and numeric share it.

Signed-off-by: Matt Katz <mhkatz97@gmail.com>
The conformance harness now accepts decimal arrays, checking array results
element-wise against DecimalScalar::checked_binary_numeric for representative
constants. Wire it into the decimal, chunked, and decimal-byte-parts compute
tests, and add decimal cases to the binary_ops benchmark.

Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
@codspeed-hq

codspeed-hq Bot commented Jul 10, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚡ 3 improved benchmarks
❌ 2 regressed benchmarks
✅ 1625 untouched benchmarks
🆕 4 new benchmarks
⏩ 49 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation rebuild_naive 91.3 µs 105.3 µs -13.37%
Simulation bitwise_not_vortex_buffer_mut[128] 244.4 ns 273.6 ns -10.66%
Simulation encode_varbin[(10000, 2)] 977.1 µs 839.2 µs +16.42%
Simulation chunked_varbinview_into_canonical[(100, 100)] 307.4 µs 271.7 µs +13.12%
Simulation eq_i64_constant 298.4 µs 267.8 µs +11.42%
🆕 Simulation add_decimal_i128_nullable N/A 8 ms N/A
🆕 Simulation add_decimal_i64_nonnull N/A 3 ms N/A
🆕 Simulation div_decimal_i128_constant N/A 14.7 ms N/A
🆕 Simulation mul_decimal_i64_nonnull N/A 5.4 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-arithmetic (6e9acbc) with develop (d2b2378)2

Open in CodSpeed

Footnotes

  1. 49 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.

  2. No successful run was found on mk/decimal-scalar-fixed-point (591320d) during the generation of this report, so develop (d2b2378) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@mhk197

mhk197 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the per-operator restructure: #8724 lands the infrastructure plus Add/Sub; Mul and Div follow as separate PRs, each bundled with its scalar fix. The full implementation from this PR is preserved on mk/decimal-arithmetic and will be carved up into those follow-ups.

@mhk197 mhk197 closed this Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant