Skip to content

Fix decimal scalar Mul/Div to rescale at the shared scale #8721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mhk197 wants to merge 1 commit into from
Closed

Fix decimal scalar Mul/Div to rescale at the shared scale #8721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 59 additions & 2 deletions vortex-array/src/scalar/typed_view/decimal/dvalue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<i256> {
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(
Expand Down Expand Up @@ -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<Self> {
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<Self> {
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<Self> {
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<Self> {
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<i256> {
Expand Down
17 changes: 9 additions & 8 deletions vortex-array/src/scalar/typed_view/decimal/scalar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -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
Expand Down
93 changes: 77 additions & 16 deletions vortex-array/src/scalar/typed_view/decimal/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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<DecimalValue> {
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<i128>,
) {
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<i128>,
) {
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,
Expand All @@ -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),
Expand Down
Loading