[ABA-25] fix(vortex-array): rescale raw value on Decimal scalar cast#5
Open
abnobdoss wants to merge 2 commits into
Open
[ABA-25] fix(vortex-array): rescale raw value on Decimal scalar cast#5abnobdoss wants to merge 2 commits into
abnobdoss wants to merge 2 commits into
Conversation
…s scale change
Adds two failing regression tests for ABA-25:
- `issue_aba25_decimal_cast_rescales_raw_value` covers a scale grow
(Decimal(10,2) -> Decimal(10,4)): the raw integer must be multiplied
by 10^(target - source) so the numeric value 1.23 is preserved.
- `issue_aba25_decimal_cast_rescales_raw_value_scale_shrink` covers a
scale shrink (Decimal(10,4) -> Decimal(10,2)): the raw integer must
be divided (truncating) by 10^|delta|.
Before the fix the cast at vortex-array/src/scalar/typed_view/decimal/scalar.rs
reuses the raw `DecimalValue` unchanged under the new scale, silently
turning 1.23 into 0.0123 (three orders of magnitude off) — these tests
fail on develop and pass after the rescale fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: abnobdoss <abnobdoss@proton.me>
Previously, casting a `DecimalScalar` to a `DType::Decimal` with a
different scale reused the raw integer unchanged under the new scale.
For example, casting a value with raw 123 at scale 2 (numeric 1.23) to
scale 4 produced raw 123 at scale 4 (numeric 0.0123) — silently three
orders of magnitude off, with no error.
This is the bug tracked by Linear ABA-25 and the in-source TODO at
`vortex-array/src/scalar/typed_view/decimal/scalar.rs` that asked
"Implement proper decimal scaling logic - whatever that means???".
The fix introduces `rescale_decimal_value`, which:
- Upcasts the raw integer to `i256`.
- When the target scale is larger than the source scale, multiplies
by `10^(target_scale - source_scale)` using `i256::checked_mul`,
returning a `vortex_bail!` overflow error rather than silently
wrapping.
- When the target scale is smaller, integer-divides (truncating
toward zero) by `10^|delta|`. This drops digits below the target
scale, matching Apache Arrow's `cast_decimal_to_decimal`.
- Verifies the rescaled value fits within the target precision via
`DecimalValue::fits_in_precision` and returns a
`numeric_value_out_of_range`-style error if not. This incidentally
tightens the previous behavior, which did no precision check on
the decimal-to-decimal path at all.
- Stores the result in the smallest `DecimalValue` variant required
by the target dtype's precision (consistent with
`DecimalType::smallest_decimal_value_type`).
Two pre-existing tests pinned the buggy behavior:
- `test_decimal_cast_between_decimal_types` asserted the raw value
was reused unchanged (with a "TODO: proper scaling logic" comment).
- `test_decimal_to_decimal_different_scale` likewise asserted the
pre-rescale raw integer (with a "TODO: this should properly
rescale" comment).
Both are updated to assert the rescaled raw integer in the smallest
variant for the target precision. These updates correct tests that
documented broken behavior — not the inverse.
Two regression tests added in the preceding commit
(`issue_aba25_decimal_cast_rescales_raw_value` and
`issue_aba25_decimal_cast_rescales_raw_value_scale_shrink`) now pass.
Closes ABA-25.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: abnobdoss <abnobdoss@proton.me>
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
1 participant
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.
Summary
Decimal-to-Decimal scalar casts in
vortex-arraypreviously reused the raw integer unchanged under the new scale, silently turning e.g.1.23(raw123at scale2) into0.0123when cast to scale4. The cast path atvortex-array/src/scalar/typed_view/decimal/scalar.rswas guarded by a TODO ("Implement proper decimal scaling logic - whatever that means???") and did no scaling at all.This PR replaces the passthrough with a proper rescale:
i256and applies10^(target_scale - source_scale)viachecked_mul(scale grow) or truncatingchecked_div(scale shrink).vortex_bail!error instead of silently wrapping.DecimalValue::fits_in_precision, returning a SQL-stylenumeric_value_out_of_rangeerror if not. The previous code did no precision check on this path at all — this is an incidental tightening worth flagging.DecimalValuevariant required by the target dtype's precision (consistent withDecimalType::smallest_decimal_value_type).Two pre-existing tests (
test_decimal_cast_between_decimal_types,test_decimal_to_decimal_different_scale) pinned the buggy behavior with explicit "TODO: should rescale" comments. They are updated to assert the rescaled raw integer — correcting tests that documented broken behavior.Linear
ABA-25 — https://linear.app/abanoubdoss/issue/ABA-25
Validation
Local checks (run on the affected crate only, per
AGENTS.md):cargo build -p vortex-array— cleancargo test -p vortex-array --lib— 2754 passed, 0 failed (was 2753 before; +1 net from two added regression tests, two existing tests updated to assert correct behavior)cargo clippy -p vortex-array --all-targets --all-features -- -D warnings— cleancargo +nightly fmt --all— cleanNew regression tests (in
vortex-array/src/scalar/typed_view/decimal/tests.rs):issue_aba25_decimal_cast_rescales_raw_value—Decimal(10,2) -> Decimal(10,4), asserts raw123rescales to12300.issue_aba25_decimal_cast_rescales_raw_value_scale_shrink—Decimal(10,4) -> Decimal(10,2), asserts raw12300rescales (truncating) to123.Both tests fail on
developwithout the fix and pass with it.Notes for reviewers
cast_decimal_to_decimaland is documented in the helper's doc comment. If you'd prefer half-even or half-up, happy to adjust.smallest_decimal_value_type(target_dtype). This changes the variant returned by the cast even when the source's variant would suffice, but the comparison/equality semantics onDecimalValueupcast toi256so downstream comparisons are unaffected.vortex-array/src/arrays/decimal/compute/cast.rs) still bails on scale changes with "not yet implemented". That is out of scope for this PR.🤖 Generated with Claude Code