Skip to content

Commit 1345b5d

Browse files
committed
Improve decimal arithmetic execution
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
1 parent e3fdf45 commit 1345b5d

6 files changed

Lines changed: 353 additions & 95 deletions

File tree

vortex-array/src/compute/conformance/binary_numeric.rs

Lines changed: 38 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -313,18 +313,6 @@ fn test_decimal_binary_numeric_with_scalar(
313313
(rhs_const.clone(), array.clone())
314314
};
315315

316-
let result = lhs
317-
.binary(rhs, operator.into())
318-
.vortex_expect("apply shouldn't fail")
319-
.execute::<RecursiveCanonical>(ctx)
320-
.map(|c| c.0.into_array());
321-
322-
// Skip this operator if the entire operation fails (e.g. an overflowing lane).
323-
let Ok(result) = result else {
324-
continue;
325-
};
326-
327-
let actual_values = to_vec_of_scalar(&result, ctx);
328316
let expected_results: Vec<Option<Scalar>> = original_values
329317
.iter()
330318
.map(|x| {
@@ -337,20 +325,46 @@ fn test_decimal_binary_numeric_with_scalar(
337325
})
338326
.collect();
339327

340-
// For elements that didn't overflow, check they match.
328+
let result = lhs
329+
.binary(rhs, operator.into())
330+
.vortex_expect("apply shouldn't fail")
331+
.execute::<RecursiveCanonical>(ctx)
332+
.map(|c| c.0.into_array());
333+
334+
let expects_overflow = expected_results.iter().any(Option::is_none);
335+
if expects_overflow {
336+
assert!(
337+
result.is_err(),
338+
"Decimal binary numeric operation should overflow for encoding {}: \
339+
{operator:?} {scalar} (lhs_is_array: {lhs_is_array})",
340+
array.encoding_id(),
341+
);
342+
continue;
343+
}
344+
345+
let result = result.unwrap_or_else(|err| {
346+
vortex_panic!(
347+
"Decimal binary numeric operation unexpectedly failed for encoding {}: \
348+
{operator:?} {scalar} (lhs_is_array: {lhs_is_array}): {err}",
349+
array.encoding_id(),
350+
)
351+
});
352+
353+
let actual_values = to_vec_of_scalar(&result, ctx);
341354
for (idx, (actual, expected)) in actual_values.iter().zip(&expected_results).enumerate()
342355
{
343-
if let Some(expected_value) = expected {
344-
assert_eq!(
345-
actual,
346-
expected_value,
347-
"Decimal binary numeric operation failed for encoding {} at index {}: \
348-
({array:?})[{idx}] {operator:?} {scalar} (lhs_is_array: {lhs_is_array}) \
349-
expected {expected_value:?}, got {actual:?}",
350-
array.encoding_id(),
351-
idx,
352-
);
353-
}
356+
let expected_value = expected
357+
.as_ref()
358+
.vortex_expect("non-overflowing decimal lane must have an expected value");
359+
assert_eq!(
360+
actual,
361+
expected_value,
362+
"Decimal binary numeric operation failed for encoding {} at index {}: \
363+
({array:?})[{idx}] {operator:?} {scalar} (lhs_is_array: {lhs_is_array}) \
364+
expected {expected_value:?}, got {actual:?}",
365+
array.encoding_id(),
366+
idx,
367+
);
354368
}
355369
}
356370
}

vortex-array/src/expr/transform/coerce.rs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ mod tests {
7070
use vortex_error::VortexResult;
7171

7272
use crate::dtype::DType;
73+
use crate::dtype::DecimalDType;
7374
use crate::dtype::Nullability::NonNullable;
7475
use crate::dtype::PType;
7576
use crate::dtype::StructFields;
@@ -153,6 +154,52 @@ mod tests {
153154
Ok(())
154155
}
155156

157+
#[test]
158+
fn decimal_arithmetic_coerces_precision_and_scale() -> VortexResult<()> {
159+
let result_dtype = DType::Decimal(DecimalDType::new(4, 2), NonNullable);
160+
let scope = DType::Struct(
161+
StructFields::new(
162+
["a", "b"].into(),
163+
vec![
164+
DType::Decimal(DecimalDType::new(3, 1), NonNullable),
165+
result_dtype.clone(),
166+
],
167+
),
168+
NonNullable,
169+
);
170+
let expr = Binary.new_expr(Operator::Add, [col("a"), col("b")]);
171+
172+
let coerced = coerce_expression(expr, &scope)?;
173+
174+
assert!(coerced.child(0).is::<Cast>());
175+
assert!(!coerced.child(1).is::<Cast>());
176+
assert_eq!(coerced.return_dtype(&scope)?, result_dtype);
177+
Ok(())
178+
}
179+
180+
#[test]
181+
fn decimal_arithmetic_coerces_integer_operand() -> VortexResult<()> {
182+
let result_dtype = DType::Decimal(DecimalDType::new(5, 2), NonNullable);
183+
let scope = DType::Struct(
184+
StructFields::new(
185+
["a", "b"].into(),
186+
vec![
187+
DType::Decimal(DecimalDType::new(4, 2), NonNullable),
188+
DType::Primitive(PType::I8, NonNullable),
189+
],
190+
),
191+
NonNullable,
192+
);
193+
let expr = Binary.new_expr(Operator::Add, [col("a"), col("b")]);
194+
195+
let coerced = coerce_expression(expr, &scope)?;
196+
197+
assert!(coerced.child(0).is::<Cast>());
198+
assert!(coerced.child(1).is::<Cast>());
199+
assert_eq!(coerced.return_dtype(&scope)?, result_dtype);
200+
Ok(())
201+
}
202+
156203
#[test]
157204
fn boolean_operators_no_coercion() -> VortexResult<()> {
158205
let scope = DType::Struct(

0 commit comments

Comments
 (0)