From 591320d2e31b9cbb93180e4d1b64db9f4ccdc87e Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Fri, 10 Jul 2026 10:27:47 -0700 Subject: [PATCH] Fix decimal scalar Mul/Div to rescale at the shared scale 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 --- .../src/scalar/typed_view/decimal/dvalue.rs | 61 +++++++++++- .../src/scalar/typed_view/decimal/scalar.rs | 17 ++-- .../src/scalar/typed_view/decimal/tests.rs | 93 +++++++++++++++---- 3 files changed, 145 insertions(+), 26 deletions(-) diff --git a/vortex-array/src/scalar/typed_view/decimal/dvalue.rs b/vortex-array/src/scalar/typed_view/decimal/dvalue.rs index c1a3be7b71c..425880b0fce 100644 --- a/vortex-array/src/scalar/typed_view/decimal/dvalue.rs +++ b/vortex-array/src/scalar/typed_view/decimal/dvalue.rs @@ -154,6 +154,27 @@ impl DecimalValue { } } + /// Rescales a stored decimal value from one scale to another, truncating toward zero when + /// reducing the scale discards fractional digits. + /// + /// Scales are `i16` because intermediate scales (e.g. the raw product of a fixed-point + /// multiplication, whose scale is twice the operand scale) can exceed the `i8` range. + /// Returns `None` if increasing the scale overflows [`i256`]. + pub(crate) fn rescale_i256_trunc(value: i256, from_scale: i16, to_scale: i16) -> Option { + if from_scale == to_scale || value == i256::ZERO { + return Some(value); + } + + let scale_delta = to_scale - from_scale; + if scale_delta > 0 { + let factor = decimal_scale_factor(scale_delta as u32).ok()?; + value.checked_mul(&factor) + } else { + let factor = decimal_scale_factor((-scale_delta) as u32).ok()?; + Some(value / factor) + } + } + /// Rescales this value to `to_decimal_dtype`, checks precision, and stores it in the target /// decimal value width. pub(crate) fn cast_decimal( @@ -239,15 +260,51 @@ impl DecimalValue { checked_widening_binary_op!(self, other, CheckedSub::checked_sub) } - /// Checked multiplication. Returns `None` on overflow. + /// Checked multiplication of the raw stored integers. Returns `None` on overflow. + /// + /// This is scale-oblivious: the raw product of two values at scale `s` has scale `2s`. Use + /// [`DecimalValue::checked_mul_fixed_point`] for scale-preserving multiplication. pub fn checked_mul(&self, other: &Self) -> Option { checked_widening_binary_op!(self, other, CheckedMul::checked_mul) } - /// Checked division. Returns `None` on overflow or division by zero. + /// Checked division of the raw stored integers. Returns `None` on overflow or division by + /// zero. + /// + /// This is scale-oblivious: the raw quotient of two values at scale `s` has scale `0`. Use + /// [`DecimalValue::checked_div_fixed_point`] for scale-preserving division. pub fn checked_div(&self, other: &Self) -> Option { checked_widening_binary_op!(self, other, CheckedDiv::checked_div) } + + /// Checked fixed-point multiplication of two stored values sharing `scale`. + /// + /// The raw integer product has scale `2 * scale`; the result is rescaled back to `scale`, + /// truncating toward zero. Returns `None` if an intermediate value overflows [`i256`]. + pub fn checked_mul_fixed_point(&self, other: &Self, scale: i8) -> Option { + let product = self.as_i256().checked_mul(&other.as_i256())?; + let rescaled = Self::rescale_i256_trunc(product, 2 * scale as i16, scale as i16)?; + Some(Self::I256(rescaled)) + } + + /// Checked fixed-point division of two stored values sharing `scale`, truncating toward + /// zero. + /// + /// Returns `None` on division by zero or if an intermediate value overflows [`i256`]. + pub fn checked_div_fixed_point(&self, other: &Self, scale: i8) -> Option { + let lhs = self.as_i256(); + let rhs = other.as_i256(); + // The raw quotient has scale 0; pre-scaling the dividend (or, for negative scales, + // the divisor) by 10^|scale| yields a quotient at `scale`. + let quotient = if scale >= 0 { + let factor = decimal_scale_factor(scale as u32).ok()?; + lhs.checked_mul(&factor)?.checked_div(&rhs)? + } else { + let factor = decimal_scale_factor(scale.unsigned_abs() as u32).ok()?; + lhs.checked_div(&rhs.checked_mul(&factor)?)? + }; + Some(Self::I256(quotient)) + } } fn decimal_scale_factor(exp: u32) -> VortexResult { diff --git a/vortex-array/src/scalar/typed_view/decimal/scalar.rs b/vortex-array/src/scalar/typed_view/decimal/scalar.rs index e365a1be02c..6be4503412d 100644 --- a/vortex-array/src/scalar/typed_view/decimal/scalar.rs +++ b/vortex-array/src/scalar/typed_view/decimal/scalar.rs @@ -179,14 +179,14 @@ impl<'a> DecimalScalar<'a> { /// Apply the (checked) operator to self and other using SQL-style null semantics. /// - /// If the operation overflows, None is returned. + /// Both operands must share the same decimal type (precision and scale), and the result + /// keeps that type. Add and Sub are exact; Mul and Div are fixed-point at the shared scale, + /// truncating toward zero when digits beyond the scale are discarded. /// - /// If the types are incompatible (ignoring nullability and precision/scale), an error is returned. + /// If the operation overflows, exceeds the precision, or divides by zero, `None` is + /// returned. /// /// If either value is null, the result is null. - /// - /// The result will have the same decimal type (precision/scale) as `self`, and the result - /// is checked to ensure it fits within the precision constraints. pub fn checked_binary_numeric( &self, other: &DecimalScalar<'a>, @@ -212,12 +212,13 @@ impl<'a> DecimalScalar<'a> { let result_value = match (self.decimal_value, other.decimal_value) { (None, _) | (_, None) => None, (Some(lhs), Some(rhs)) => { - // Perform the operation + // Add/Sub on stored integers preserve the shared scale; Mul/Div must rescale. + let scale = self.decimal_type.scale(); let operation_result = match op { NumericOperator::Add => lhs.checked_add(&rhs), NumericOperator::Sub => lhs.checked_sub(&rhs), - NumericOperator::Mul => lhs.checked_mul(&rhs), - NumericOperator::Div => lhs.checked_div(&rhs), + NumericOperator::Mul => lhs.checked_mul_fixed_point(&rhs, scale), + NumericOperator::Div => lhs.checked_div_fixed_point(&rhs, scale), }?; // Check if the result fits within the precision constraints diff --git a/vortex-array/src/scalar/typed_view/decimal/tests.rs b/vortex-array/src/scalar/typed_view/decimal/tests.rs index 63f924b2239..6c3a32a38b4 100644 --- a/vortex-array/src/scalar/typed_view/decimal/tests.rs +++ b/vortex-array/src/scalar/typed_view/decimal/tests.rs @@ -14,6 +14,7 @@ use crate::dtype::Nullability; use crate::dtype::PType; use crate::dtype::i256; use crate::scalar::DecimalValue; +use crate::scalar::NumericOperator; use crate::scalar::Scalar; #[rstest] @@ -834,8 +835,6 @@ fn test_decimal_i256_overflow_cast() { // Tests for checked_binary_numeric #[test] fn test_decimal_scalar_checked_add() { - use crate::scalar::NumericOperator; - let decimal1 = Scalar::decimal( DecimalValue::I64(100), DecimalDType::new(10, 2), @@ -861,8 +860,6 @@ fn test_decimal_scalar_checked_add() { #[test] fn test_decimal_scalar_checked_sub() { - use crate::scalar::NumericOperator; - let decimal1 = Scalar::decimal( DecimalValue::I64(500), DecimalDType::new(10, 2), @@ -888,8 +885,6 @@ fn test_decimal_scalar_checked_sub() { #[test] fn test_decimal_scalar_checked_mul() { - use crate::scalar::NumericOperator; - let decimal1 = Scalar::decimal( DecimalValue::I32(50), DecimalDType::new(10, 2), @@ -904,19 +899,18 @@ fn test_decimal_scalar_checked_mul() { ); let scalar2 = decimal2.as_decimal(); + // 0.50 * 0.10 = 0.05, stored as 5 at scale 2. let result = scalar1 .checked_binary_numeric(&scalar2, NumericOperator::Mul) .unwrap(); assert_eq!( result.decimal_value(), - Some(DecimalValue::I256(i256::from_i128(500))) + Some(DecimalValue::I256(i256::from_i128(5))) ); } #[test] fn test_decimal_scalar_checked_div() { - use crate::scalar::NumericOperator; - let decimal1 = Scalar::decimal( DecimalValue::I64(1000), DecimalDType::new(10, 2), @@ -931,19 +925,18 @@ fn test_decimal_scalar_checked_div() { ); let scalar2 = decimal2.as_decimal(); + // 10.00 / 0.10 = 100.00, stored as 10000 at scale 2. let result = scalar1 .checked_binary_numeric(&scalar2, NumericOperator::Div) .unwrap(); assert_eq!( result.decimal_value(), - Some(DecimalValue::I256(i256::from_i128(100))) + Some(DecimalValue::I256(i256::from_i128(10000))) ); } #[test] fn test_decimal_scalar_checked_div_by_zero() { - use crate::scalar::NumericOperator; - let decimal1 = Scalar::decimal( DecimalValue::I64(1000), DecimalDType::new(10, 2), @@ -962,10 +955,80 @@ fn test_decimal_scalar_checked_div_by_zero() { assert_eq!(result, None); } +/// Applies `op` to two non-null decimal scalars of `dtype`, returning the stored result value or +/// `None` if the operation failed (overflow, precision violation, or division by zero). +fn checked_decimal_op( + lhs: i128, + rhs: i128, + dtype: DecimalDType, + op: NumericOperator, +) -> Option { + let lhs = Scalar::decimal(DecimalValue::I128(lhs), dtype, Nullability::NonNullable); + let rhs = Scalar::decimal(DecimalValue::I128(rhs), dtype, Nullability::NonNullable); + lhs.as_decimal() + .checked_binary_numeric(&rhs.as_decimal(), op)? + .decimal_value() +} + +#[rstest] +#[case::mul_rescales(50, 10, NumericOperator::Mul, Some(5))] // 0.50 * 0.10 = 0.05 +#[case::mul_truncates(15, 15, NumericOperator::Mul, Some(2))] // 0.15 * 0.15 = 0.0225 -> 0.02 +#[case::mul_truncates_toward_zero(-15, 15, NumericOperator::Mul, Some(-2))] +#[case::div_rescales(1000, 10, NumericOperator::Div, Some(10000))] // 10.00 / 0.10 = 100.00 +#[case::div_truncates(1000, 300, NumericOperator::Div, Some(333))] // 10.00 / 3.00 = 3.33... +#[case::div_truncates_toward_zero(-1000, 300, NumericOperator::Div, Some(-333))] +fn test_decimal_scalar_fixed_point( + #[case] lhs: i128, + #[case] rhs: i128, + #[case] op: NumericOperator, + #[case] expected: Option, +) { + let result = checked_decimal_op(lhs, rhs, DecimalDType::new(10, 2), op); + assert_eq!(result, expected.map(DecimalValue::I128)); +} + +#[rstest] +#[case::mul(5, 3, NumericOperator::Mul, Some(1500))] // 500 * 300 = 150_000, stored 1500 +#[case::div(600, 3, NumericOperator::Div, Some(2))] // 60_000 / 300 = 200, stored 2 +#[case::div_truncates(5000, 3, NumericOperator::Div, Some(16))] // 500_000 / 300 = 1666.67 -> 1600 +fn test_decimal_scalar_fixed_point_negative_scale( + #[case] lhs: i128, + #[case] rhs: i128, + #[case] op: NumericOperator, + #[case] expected: Option, +) { + let result = checked_decimal_op(lhs, rhs, DecimalDType::new(10, -2), op); + assert_eq!(result, expected.map(DecimalValue::I128)); +} + #[test] -fn test_decimal_scalar_null_handling() { - use crate::scalar::NumericOperator; +fn test_decimal_scalar_checked_mul_precision_overflow() { + // 99.9 * 99.9 = 9980.01, which exceeds precision 3 at scale 1. + let result = checked_decimal_op(999, 999, DecimalDType::new(3, 1), NumericOperator::Mul); + assert_eq!(result, None); +} + +#[test] +fn test_decimal_scalar_checked_mul_wider_than_operand_storage() { + // 3000.00 * 3000.00 = 9_000_000.00: the raw product (9e10) overflows the i32 operand + // storage, but the rescaled result (9e8) fits both i32 and precision 10. + let lhs = Scalar::decimal( + DecimalValue::I32(300000), + DecimalDType::new(10, 2), + Nullability::NonNullable, + ); + let result = lhs + .as_decimal() + .checked_binary_numeric(&lhs.as_decimal(), NumericOperator::Mul) + .unwrap(); + assert_eq!( + result.decimal_value(), + Some(DecimalValue::I128(900_000_000)) + ); +} +#[test] +fn test_decimal_scalar_null_handling() { let decimal1 = Scalar::null(DType::Decimal( DecimalDType::new(10, 2), Nullability::Nullable, @@ -987,8 +1050,6 @@ fn test_decimal_scalar_null_handling() { #[test] fn test_decimal_scalar_precision_overflow() { - use crate::scalar::NumericOperator; - // Create decimals with precision 3 (max value 999) let decimal1 = Scalar::decimal( DecimalValue::I16(999),