From b303c03b715cbf1d1a7d8367338ea67fe9b59c6d Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Tue, 15 Sep 2026 20:38:27 +0800 Subject: [PATCH] fix(python): build Decimal scalars from the unscaled integer and an exponent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- vortex-python/src/scalar/into_py.rs | 74 +++++------------------------ vortex-python/test/test_scalar.py | 50 +++++++++++++++++++ 2 files changed, 62 insertions(+), 62 deletions(-) diff --git a/vortex-python/src/scalar/into_py.rs b/vortex-python/src/scalar/into_py.rs index 44931f16145..be4bc72d5b8 100644 --- a/vortex-python/src/scalar/into_py.rs +++ b/vortex-python/src/scalar/into_py.rs @@ -1,8 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -use std::cmp::Ordering; - use pyo3::Bound; use pyo3::IntoPyObject; use pyo3::PyAny; @@ -22,7 +20,6 @@ use vortex::buffer::ByteBuffer; use vortex::dtype::DType; use vortex::dtype::PType; use vortex::dtype::half::f16; -use vortex::dtype::i256; use vortex::error::VortexExpect; use vortex::error::vortex_err; use vortex::scalar::DecimalValue; @@ -151,59 +148,6 @@ impl<'py> IntoPyObject<'py> for PyVortex> { } } -trait DecimalIntoParts: Sized { - /// Split an integer encoding a decimal with the given `scale` into a - /// (whole number, decimal) parts. - /// - /// For example, for the number 123i128 and scale 2, this will return returns (1, 23). - fn decimal_parts(self, scale: i8) -> (Self, Self); -} - -macro_rules! impl_decimal_into_parts { - ($ty:ident, $ten:expr) => { - impl DecimalIntoParts for $ty { - fn decimal_parts(self, scale: i8) -> (Self, Self) { - let scale_factor = $ten.pow(scale.unsigned_abs() as u32); - match scale.cmp(&0) { - Ordering::Equal => (self, 0), - Ordering::Less => { - // Negative scale -> apply the given number of trailing zeros - (self * scale_factor, 0) - } - Ordering::Greater => { - // Positive scale -> extract the leading/trailing digits separately. - (self / scale_factor, self % scale_factor) - } - } - } - } - }; -} - -impl_decimal_into_parts!(i8, 10i8); -impl_decimal_into_parts!(i16, 10i16); -impl_decimal_into_parts!(i32, 10i32); -impl_decimal_into_parts!(i64, 10i64); -impl_decimal_into_parts!(i128, 10i128); - -impl DecimalIntoParts for i256 { - fn decimal_parts(self, scale: i8) -> (Self, Self) { - match scale.cmp(&0) { - Ordering::Equal => (self, i256::ZERO), - Ordering::Less => { - // Negative scale -> apply the given number of trailing zeros - let scale_factor = i256::from_i128(10).wrapping_pow(-scale as u32); - (self * scale_factor, i256::ZERO) - } - Ordering::Greater => { - // Positive scale -> extract the leading/trailing digits separately. - let scale_factor = i256::from_i128(10).wrapping_pow(scale as u32); - (self / scale_factor, self % scale_factor) - } - } - } -} - fn decimal_value_to_py( py: Python, scale: i8, @@ -211,10 +155,16 @@ fn decimal_value_to_py( ) -> PyResult> { let decimal_class = decimal_class(py)?; - match_each_decimal_value!(decimal_value, |value| { - let (whole, decimal) = value.decimal_parts(scale); - let repr = - format!("{}.{:0>width$}", whole, decimal, width = scale as usize).into_pyobject(py)?; - decimal_class.call1((repr,)) - }) + // Hand `Decimal` the unscaled integer and an exponent rather than splitting the value into + // whole and fractional digits here. Splitting has to reproduce sign handling, zero padding and + // the negative-scale case, and `Decimal` already does all three: it parses this form exactly + // (the context precision bounds arithmetic, not construction) and the value it returns 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, and `MIN_SCALE` is not + // bounded away from `i8::MIN`. + let repr = match_each_decimal_value!(decimal_value, |value| { + format!("{value}E{}", -i16::from(scale)) + }); + decimal_class.call1((repr.into_pyobject(py)?,)) } diff --git a/vortex-python/test/test_scalar.py b/vortex-python/test/test_scalar.py index c99d7a7c756..24c5889c9a4 100644 --- a/vortex-python/test/test_scalar.py +++ b/vortex-python/test/test_scalar.py @@ -1,6 +1,8 @@ # SPDX-License-Identifier: Apache-2.0 # SPDX-FileCopyrightText: Copyright the Vortex contributors +import decimal + import pytest import vortex as vx @@ -35,3 +37,51 @@ def test_f16() -> None: scalar = vx.scalar(1.0, dtype=vx.float_(16)) assert scalar.dtype == vx.float_(16) assert scalar.as_py() == 1.0 + + +@pytest.mark.parametrize( + "precision,scale,stored,expected", + [ + (10, 2, 12345, "123.45"), + # A negative stored value used to render as "-123.-45", which Decimal refuses. + (10, 2, -12345, "-123.45"), + # Truncating division put the whole part at 0 and left the sign on the fraction. + (10, 2, -5, "-0.05"), + (10, 2, 5, "0.05"), + # The stored value picks the narrowest storage that holds it, so a small value at an + # everyday scale reached `10i8.pow(3)`, which overflows an i8. + (10, 3, 5, "0.005"), + (10, 5, 5, "0.00005"), + (10, 3, -5, "-0.005"), + # Scale 0 has no fractional digits, so the exponent has to be 0 and not -1. + (10, 0, -7, "-7"), + # A negative scale means trailing zeros before the point, so the exponent is positive. + (5, -5, 1, "1E+5"), + (5, -5, -1, "-1E+5"), + (1, -128, 1, "1E+128"), + # Above scale 38 no storage width holds the factor, i128 included. + (76, 39, 1, "1E-39"), + (76, 76, -1, "-1E-76"), + ], +) +def test_decimal_round_trip(precision: int, scale: int, stored: int, expected: str) -> None: + scalar = vx.scalar(stored, dtype=vx.decimal(precision=precision, scale=scale)) + + value = scalar.as_py() + assert isinstance(value, decimal.Decimal) + # Compare the string form, not just the numeric value: Decimal("-7") == Decimal("-7.0"), so an + # equality check alone would not pin the exponent to the dtype's scale. + assert str(value) == expected + assert value.as_tuple().exponent == -scale + + +def test_decimal_ignores_context_precision() -> None: + """A wide decimal must survive conversion whatever the ambient context precision is.""" + digits = "9" * 38 + with decimal.localcontext() as ctx: + ctx.prec = 3 + scalar = vx.scalar(int(digits), dtype=vx.decimal(precision=38, scale=19)) + value = scalar.as_py() + assert isinstance(value, decimal.Decimal) + assert len(value.as_tuple().digits) == 38 + assert str(value) == f"{digits[:19]}.{digits[19:]}"