Skip to content

Fix decimal scalar Mul/Div to rescale at the shared scale#8721

Closed
mhk197 wants to merge 1 commit into
developfrom
mk/decimal-scalar-fixed-point
Closed

Fix decimal scalar Mul/Div to rescale at the shared scale#8721
mhk197 wants to merge 1 commit into
developfrom
mk/decimal-scalar-fixed-point

Conversation

@mhk197

@mhk197 mhk197 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What

DecimalScalar::checked_binary_numeric applied the checked operator to the raw stored integers with no scale handling, so Mul results were off by 10^scale (its own test asserted 0.50 × 0.10 = "5.00"; mathematically 0.05) and Div dropped the scale entirely (10.00 / 0.10 = "1.00" instead of 100.00).

This fixes Mul and Div to fixed-point semantics at the shared scale, truncating toward zero:

  • Mul: the raw product (scale 2s) is rescaled back to s via a new truncating rescale helper. The product is computed in i256, so it can exceed the operand storage width as long as the rescaled result fits the precision.
  • Div: (a · 10^s) / b for s ≥ 0, a / (b · 10^{-s}) for negative scales; division by zero returns None.
  • Add/Sub are unchanged (raw ops are exact at a shared scale).

The raw DecimalValue::checked_mul/checked_div remain scale-oblivious and are now documented as such, pointing at the new checked_mul_fixed_point/checked_div_fixed_point.

The only production callers of the scalar op use Add (sum aggregate, stats), so this is a safe bug fix. It is also a prerequisite for native decimal array arithmetic (follow-up PR), whose conformance tests compare element-wise against this scalar implementation.

Note: the truncating rescale helper takes i16 scales because intermediate scales (e.g. 2s for the raw product) can exceed the i8 range.

Testing

  • Fixed the Mul/Div test expectations and added truncation (0.15 × 0.15 → 0.02, toward-zero for negatives), negative-scale, wider-than-storage, and precision-overflow cases.
  • cargo nextest run -p vortex-array, cargo clippy --all-targets --all-features, cargo +nightly fmt --all.

🤖 Generated with Claude Code

checked_binary_numeric applied raw integer ops, so Mul results were off by
10^scale and Div dropped the scale entirely. Mul now rescales the raw product
from 2s back to s and Div pre-scales the dividend, both truncating toward
zero.

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

⚡ 6 improved benchmarks
❌ 2 regressed benchmarks
✅ 1629 untouched benchmarks
⏩ 42 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_varbinview_canonical_into[(1000, 10)] 154.6 µs 190.9 µs -19.02%
Simulation slice_empty_vortex 339.4 ns 397.8 ns -14.66%
Simulation bitwise_not_vortex_buffer_mut[128] 244.4 ns 186.1 ns +31.34%
Simulation bitwise_not_vortex_buffer_mut[1024] 304.7 ns 246.4 ns +23.68%
Simulation bitwise_not_vortex_buffer_mut[2048] 398.6 ns 340.3 ns +17.14%
Simulation encode_varbin[(10000, 2)] 977.1 µs 835.6 µs +16.93%
Simulation chunked_varbinview_opt_canonical_into[(100, 100)] 340.6 µs 305.7 µs +11.41%
Simulation compare_int_constant 298.7 µs 268.4 µs +11.28%

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-scalar-fixed-point (591320d) with develop (d2b2378)

Open in CodSpeed

Footnotes

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

@mhk197

mhk197 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the per-operator restructure starting at #8724. The scalar Mul and Div fixes from this PR will land with the decimal Mul and Div kernel PRs respectively, since each fix is only observable through the op it corrects.

@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