andygrove commented on code in PR #4939:
URL: https://github.com/apache/datafusion-comet/pull/4939#discussion_r3669903772
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1160,6 +1152,81 @@ mod tests {
assert_eq!(decimal_array.value(1), -10000); // -100 * 10^2
assert!(decimal_array.is_null(2));
}
+
+ #[test]
+ fn test_cast_int_to_decimal128_overflow_legacy_nulls() {
+ // 1000 * 10^2 = 100000 does not fit precision 3 -> null (legacy).
Valid values and the
+ // input null are preserved, exercising the vectorized
null-on-overflow path.
+ let array: ArrayRef = Arc::new(Int32Array::from(vec![Some(9),
Some(1000), None, Some(-9)]));
+ let result = cast_int_to_decimal128(
+ &array,
+ EvalMode::Legacy,
+ &DataType::Int32,
+ &DataType::Decimal128(3, 2),
+ 3,
+ 2,
+ )
+ .unwrap();
+ let d = result.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 900); // 9.00
+ assert!(d.is_null(1)); // overflow -> null
+ assert!(d.is_null(2)); // input null preserved
+ assert_eq!(d.value(3), -900);
+ assert_eq!(d.data_type(), &DataType::Decimal128(3, 2));
+ }
+
+ #[test]
+ fn test_cast_int_to_decimal128_overflow_try_nulls() {
+ // Try shares the Legacy null-on-overflow branch but is a distinct
enum arm; assert it
+ // explicitly so a future refactor cannot regress it silently.
+ let array: ArrayRef = Arc::new(Int32Array::from(vec![Some(9),
Some(1000), None, Some(-9)]));
+ let result = cast_int_to_decimal128(
+ &array,
+ EvalMode::Try,
+ &DataType::Int32,
+ &DataType::Decimal128(3, 2),
+ 3,
+ 2,
+ )
+ .unwrap();
+ let d = result.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 900);
+ assert!(d.is_null(1));
+ assert!(d.is_null(2));
+ assert_eq!(d.value(3), -900);
+ }
+
+ #[test]
+ fn test_cast_int_to_decimal128_no_overflow_ansi() {
+ let array: ArrayRef = Arc::new(Int32Array::from(vec![Some(9), None,
Some(-9)]));
+ let result = cast_int_to_decimal128(
+ &array,
+ EvalMode::Ansi,
+ &DataType::Int32,
+ &DataType::Decimal128(3, 2),
+ 3,
+ 2,
+ )
+ .unwrap();
+ let d = result.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 900);
+ assert!(d.is_null(1));
+ assert_eq!(d.value(2), -900);
+ }
+
+ #[test]
+ fn test_cast_int_to_decimal128_overflow_ansi_errors() {
Review Comment:
Done, both in the same push.
The assertion now matches the variant and its payload, and the input carries
a second overflowing value after the first so the "first offending value" part
is pinned too:
```rust
let array: ArrayRef = Arc::new(Int32Array::from(vec![Some(9), Some(1000),
Some(2000)]));
...
assert!(
matches!(
err,
SparkError::NumericValueOutOfRange { ref value, precision: 3, scale:
2 } if value == "1000"
),
"unexpected error: {err:?}"
);
```
I checked that this is not vacuous: temporarily changing the rescan to
`(0..array.len()).rev()` fails the test with `NumericValueOutOfRange { value:
"2000", precision: 3, scale: 2 }`, so scan order is genuinely pinned.
Rebased on latest `main` to clear the conflicts. Three collisions, all
additive against the cast work that landed in the meantime: the new `[[bench]]`
entries in `Cargo.toml`, the `numeric.rs` import list (kept
`GenericStringBuilder` from main, dropped `Decimal128Builder` since this PR
removes its last use), and the audit doc entry, which now follows the #4920 and
#4940 entries.
`cargo test -p datafusion-comet-spark-expr` passes (542 tests), clippy with
`-D warnings` and `cargo fmt --check` are clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
