fix(python): build Decimal scalars from the unscaled integer and an exponent - #9885
Open
jackylee-ch wants to merge 1 commit into
Open
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
…xponent
`decimal_value_to_py` split the stored integer into whole and fractional
parts and pasted them together as `"{whole}.{fraction}"`. That reimplements
three things `Decimal` already does, and got all three wrong:
- a negative value kept its sign on both parts, so `decimal(10,2)` storing
-12345 rendered as `-123.-45` and `Decimal` raised `InvalidOperation`;
- the pad width came from `scale as usize`, which wraps to ~1.8e19 for a
negative scale;
- the factor was `10.pow(scale.unsigned_abs())` in the *storage* type, and
the storage type is the narrowest that holds the value, so an everyday
`decimal(10,3)` storing 5 evaluated `10i8.pow(3)`.
Scale 0 was wrong too: it emitted a `.0` the dtype does not have.
Pass `Decimal` the unscaled integer and an exponent instead. It parses that
form exactly — the context precision bounds arithmetic, not construction —
and the result carries an exponent of exactly `-scale`, so the dtype's scale
survives the round trip. The exponent is negated through `i16` because
`-i8::MIN` overflows.
This drops the `DecimalIntoParts` trait, its macro and the `i256` impl.
Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
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.
decimal_value_to_py(vortex-python/src/scalar/into_py.rs:207) split the stored integer into awhole and a fractional part and pasted them together as
"{whole}.{fraction}". That reimplementsthree things
Decimalalready does, and all three were wrong. Measured againstdevelop:decimal(10,2)storing12345Decimal('123.45')decimal(10,2)storing-12345decimal.InvalidOperationDecimal('-123.45')decimal(10,2)storing-5decimal.InvalidOperationDecimal('-0.05')decimal(10,0)storing-7Decimal('-7.0')Decimal('-7')decimal(10,3)storing5attempt to exponentiate with overflowDecimal('0.005')-123.-45, whichDecimalrefuses.Truncating division makes the smaller case worse:
-5at scale 2 renders as0.-5, dropping thesign entirely even if it had parsed.
scale as usize, which wraps to ~1.8e19 for a negative scale.10.pow(scale.unsigned_abs())in the storage type, andscalar_helper_inner(factory.rs:80) picks the narrowest storage that holds the value, not onederived from the dtype. So
vx.scalar(5, dtype=vx.decimal(precision=10, scale=3))— an ordinarydtype with a small value — evaluates
10i8.pow(3). This is not an extreme-scale corner..0the dtype does not have, so the returnedDecimal's exponent did not matchthe dtype's scale.
On profiles, so the last row is not overstated:
pyproject.toml:63builds wheels withprofile = "release"andCargo.toml:410leavesoverflow-checksat its default, off. Theoverflow panic above is what an editable/dev build does; a release wheel wraps the factor instead
and returns a wrong
Decimal. TheInvalidOperationand scale-0 rows are string-shape bugs andhold in both profiles.
Fix
Hand
Decimalthe unscaled integer and an exponent. It parses that form exactly — the contextprecision bounds arithmetic, not construction — and the result carries an exponent of exactly
-scale, so the dtype's scale survives the round trip. The exponent is negated throughi16because
-i8::MINoverflows and the scale's lower bound is not held away fromi8::MIN.This deletes the
DecimalIntoPartstrait, its macro and thei256impl, for a net -5 lines.Not in this change
vortex/__init__.py:74asks whetherDecimalScalaris missing from the public exports — it is,though
scalar.pyi:31declares it. Separately,scalar_helper_innerhas noi256branch, so avalue wider than
i128cannot be built into a decimal scalar at all(
Value can't be represented as decimal). Both are gaps in the other direction and belong in theirown changes.
Tests
pytest vortex-python/test/test_scalar.py: 27 passed, 13 before — the file had no decimal coverage.Cases span negative values, scale 0, negative scale down to
-128, and scale past 38, and assertthe string form rather than numeric equality, since
Decimal("-7") == Decimal("-7.0")would let thescale-0 bug through.
pytest vortex-python/test: 354 passed, 2 skipped, 1 xfailed.For the non-hollowness check the production change was stashed and the extension rebuilt after
uv cache clean vortex-data, since--reinstall-packagealone reuses the cached wheel: 5 of thenew cases fail, one of them as a Rust panic through the FFI boundary. The negative-scale path was
probed separately under a 2 GiB
RLIMIT_DATAcap so a wrapped pad width could not take the machinedown; it panics at the
powbefore reaching the formatter.uvx ruff format --check,uvx ruff check,uvx ty check vortex-python,cargo +nightly fmt --all --check,cargo clippy --release -p vortex-python --all-features --all-targets --no-deps: all clean.AI assistance
Written with agentic AI assistance; every row of the table above was reproduced before and after the
change. The
decimal(10,3)row was not something I predicted — I found it after noticing thestorage width comes from the value.