From c8ed4d5e3983b1d7c7ba53154aeddad4e6633f8c Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 27 Oct 2026 17:13:12 -0400 Subject: [PATCH] Build precision 19 decimal scalars at i128 The Python and JNI decimal scalar factories picked the value width from `DecimalDType::required_bit_width()`, which counts magnitude bits without a sign bit. At precision 19 it returns 64, so both factories chose `i64`, and a valid value above `i64::MAX` failed with an overflow error. For example, `vx.scalar(decimal.Decimal("9999999999999999999"))` failed. Both factories now match on `DecimalDType::canonical_values_type()`, which gives `i128` at precision 19. Every other precision keeps its width. Signed-off-by: Connor Tsui --- vortex-jni/src/expression.rs | 66 +++++++++++++++++------------ vortex-python/src/scalar/factory.rs | 27 +++++------- vortex-python/test/test_scalar.py | 2 + 3 files changed, 52 insertions(+), 43 deletions(-) diff --git a/vortex-jni/src/expression.rs b/vortex-jni/src/expression.rs index afcefe990c4..b271928d6dc 100644 --- a/vortex-jni/src/expression.rs +++ b/vortex-jni/src/expression.rs @@ -28,6 +28,7 @@ use vortex::VortexSessionDefault; use vortex::dtype::BigCast; use vortex::dtype::DType; use vortex::dtype::DecimalDType; +use vortex::dtype::DecimalType; use vortex::dtype::FieldName; use vortex::dtype::FieldNames; use vortex::dtype::MapDType; @@ -576,7 +577,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeExpression_literalDecimal( } /// Decode a two's-complement big-endian byte array (Java `BigInteger.toByteArray()` format) -/// into the smallest [`DecimalValue`] variant that can hold the precision. +/// into the [`DecimalValue`] variant of the canonical width of the precision. fn decimal_value_from_be_bytes( bytes: &[u8], dtype: &DecimalDType, @@ -585,32 +586,17 @@ fn decimal_value_from_be_bytes( throw_runtime!("decimal unscaled value must have at least one byte"); } let value = i256_from_twos_complement_be(bytes); - // Pick the narrowest backing integer that fits the dtype's precision. - let required_bits = dtype.required_bit_width(); - if required_bits <= 8 { - let v = - BigCast::from(value).ok_or_else(|| vortex_err!("decimal value does not fit in i8"))?; - Ok(DecimalValue::I8(v)) - } else if required_bits <= 16 { - let v = - BigCast::from(value).ok_or_else(|| vortex_err!("decimal value does not fit in i16"))?; - Ok(DecimalValue::I16(v)) - } else if required_bits <= 32 { - let v = - BigCast::from(value).ok_or_else(|| vortex_err!("decimal value does not fit in i32"))?; - Ok(DecimalValue::I32(v)) - } else if required_bits <= 64 { - let v = - BigCast::from(value).ok_or_else(|| vortex_err!("decimal value does not fit in i64"))?; - Ok(DecimalValue::I64(v)) - } else if required_bits <= 128 { - let v = value - .maybe_i128() - .ok_or_else(|| vortex_err!("decimal value does not fit in i128"))?; - Ok(DecimalValue::I128(v)) - } else { - Ok(DecimalValue::I256(value)) - } + + let values_type = dtype.canonical_values_type(); + let overflow = || vortex_err!("decimal value does not fit in {values_type}"); + Ok(match values_type { + DecimalType::I8 => DecimalValue::I8(BigCast::from(value).ok_or_else(overflow)?), + DecimalType::I16 => DecimalValue::I16(BigCast::from(value).ok_or_else(overflow)?), + DecimalType::I32 => DecimalValue::I32(BigCast::from(value).ok_or_else(overflow)?), + DecimalType::I64 => DecimalValue::I64(BigCast::from(value).ok_or_else(overflow)?), + DecimalType::I128 => DecimalValue::I128(value.maybe_i128().ok_or_else(overflow)?), + DecimalType::I256 => DecimalValue::I256(value), + }) } /// Sign-extend a two's-complement big-endian byte slice into an `i256`. @@ -1063,3 +1049,29 @@ pub extern "system" fn Java_dev_vortex_jni_NativeExpression_literalNull( Ok(into_raw(lit(Scalar::null(dtype)))) }) } + +#[cfg(test)] +mod tests { + use rstest::rstest; + use vortex::dtype::DecimalDType; + use vortex::scalar::DecimalValue; + + use super::decimal_value_from_be_bytes; + use crate::errors::JNIError; + + /// Precision 19 holds values above `i64::MAX`, so the value must use the canonical `i128`. + #[rstest] + #[case::max(9_999_999_999_999_999_999)] + #[case::min(-9_999_999_999_999_999_999)] + fn decimal_value_at_precision_19_uses_i128(#[case] unscaled: i128) -> Result<(), JNIError> { + let dtype = DecimalDType::try_new(19, 0)?; + + let value = decimal_value_from_be_bytes(&unscaled.to_be_bytes(), &dtype)?; + + assert!( + matches!(value, DecimalValue::I128(v) if v == unscaled), + "expected DecimalValue::I128({unscaled}), got {value:?}" + ); + Ok(()) + } +} diff --git a/vortex-python/src/scalar/factory.rs b/vortex-python/src/scalar/factory.rs index b46ecf934d8..1ecd944fa8c 100644 --- a/vortex-python/src/scalar/factory.rs +++ b/vortex-python/src/scalar/factory.rs @@ -17,6 +17,7 @@ use pyo3::types::PyString; use vortex::dtype::BigCast; use vortex::dtype::DType; use vortex::dtype::DecimalDType; +use vortex::dtype::DecimalType; use vortex::dtype::FieldName; use vortex::dtype::FieldNames; use vortex::dtype::MAX_PRECISION; @@ -603,29 +604,23 @@ fn decimal_scalar(value: &Bound<'_, PyAny>, hint: Option<&DecimalDType>) -> PyVo Ok(Scalar::try_new( DType::Decimal(decimal_dtype, Nullability::NonNullable), - Some(ScalarValue::from(narrowest_decimal_value( + Some(ScalarValue::from(canonical_decimal_value( unscaled, &decimal_dtype, )?)), )?) } -/// The narrowest [`DecimalValue`] variant able to hold every value of `dtype`'s precision. -fn narrowest_decimal_value(value: i256, dtype: &DecimalDType) -> PyResult { +/// The [`DecimalValue`] variant of the canonical width of `dtype`'s precision. +fn canonical_decimal_value(value: i256, dtype: &DecimalDType) -> PyResult { let overflow = || PyValueError::new_err(format!("Decimal value {value} overflows {dtype}")); - let required_bits = dtype.required_bit_width(); - Ok(if required_bits <= 8 { - DecimalValue::I8(BigCast::from(value).ok_or_else(overflow)?) - } else if required_bits <= 16 { - DecimalValue::I16(BigCast::from(value).ok_or_else(overflow)?) - } else if required_bits <= 32 { - DecimalValue::I32(BigCast::from(value).ok_or_else(overflow)?) - } else if required_bits <= 64 { - DecimalValue::I64(BigCast::from(value).ok_or_else(overflow)?) - } else if required_bits <= 128 { - DecimalValue::I128(value.maybe_i128().ok_or_else(overflow)?) - } else { - DecimalValue::I256(value) + Ok(match dtype.canonical_values_type() { + DecimalType::I8 => DecimalValue::I8(BigCast::from(value).ok_or_else(overflow)?), + DecimalType::I16 => DecimalValue::I16(BigCast::from(value).ok_or_else(overflow)?), + DecimalType::I32 => DecimalValue::I32(BigCast::from(value).ok_or_else(overflow)?), + DecimalType::I64 => DecimalValue::I64(BigCast::from(value).ok_or_else(overflow)?), + DecimalType::I128 => DecimalValue::I128(value.maybe_i128().ok_or_else(overflow)?), + DecimalType::I256 => DecimalValue::I256(value), }) } diff --git a/vortex-python/test/test_scalar.py b/vortex-python/test/test_scalar.py index af477e6de74..c6dd0e43d7c 100644 --- a/vortex-python/test/test_scalar.py +++ b/vortex-python/test/test_scalar.py @@ -219,6 +219,8 @@ def test_timestamp_scalar_rejects_precision_loss() -> None: ("-0.001", 3, 3), ("12E+2", 4, 0), ("0", 1, 0), + ("9" * 19, 19, 0), + ("-" + "9" * 19, 19, 0), ("9" * 38, 38, 0), ], )