mbutrovich commented on code in PR #4939:
URL: https://github.com/apache/datafusion-comet/pull/4939#discussion_r3632034098
##########
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:
`test_cast_int_to_decimal128_overflow_ansi_errors` asserts only
`result.is_err()`. The rescan comment promises it "reports the first offending
value with Spark's exact error," but nothing tests that. A refactor that
scanned in reverse, or returned a different error variant, would still pass
`is_err()`. Match on the variant and the offending value so that behavior is
pinned:
```rust
let err = result.unwrap_err();
assert!(
matches!(
err,
SparkError::NumericValueOutOfRange { ref value, precision: 3, scale:
2 } if value == "1000"
),
"unexpected error: {err:?}"
);
```
Adding a second overflowing value ahead of `1000` in the input and keeping
the assertion on `1000` would also pin the "first offending value" part of the
promise, since that is the row the rescan is documented to report.
Requesting changes to fold this in on the same push, since the branch has
merge conflicts to resolve anyway.
--
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]
