mbutrovich commented on code in PR #4940:
URL: https://github.com/apache/datafusion-comet/pull/4940#discussion_r3591276273
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1287,6 +1292,48 @@ mod tests {
assert!(casted.is_null(9));
}
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_no_overflow_fast_path() {
+ // All values fit precision 10, scale 2, so the vectorized fast path
is taken and the
+ // input null is preserved.
+ let a: ArrayRef = Arc::new(Float64Array::from(vec![
+ Some(42.0),
+ Some(-1.5),
+ None,
+ Some(0.0),
+ ]));
+ let b =
+ cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2,
EvalMode::Legacy).unwrap();
+ let d = b.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 4200); // 42.00
+ assert_eq!(d.value(1), -150); // -1.50
+ assert!(d.is_null(2));
+ assert_eq!(d.value(3), 0);
+ assert_eq!(d.data_type(), &DataType::Decimal128(10, 2));
+ }
+
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_ansi_no_overflow() {
+ let a: ArrayRef = Arc::new(Float64Array::from(vec![Some(42.0), None,
Some(-1.5)]));
+ let b =
+ cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2,
EvalMode::Ansi).unwrap();
+ let d = b.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 4200);
+ assert!(d.is_null(1));
+ assert_eq!(d.value(2), -150);
+ }
+
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_ansi_overflow_errors() {
Review Comment:
`test_cast_float_to_decimal_ansi_overflow_errors` (diff, new test) only
exercises a finite precision overflow (`4242.42` into `Decimal(4,2)`). The
rescan at `numeric.rs` (new ANSI block) also has to raise for NaN and infinity,
since those make `to_i128()` return `None` and get nulled by `unary_opt`
exactly like a precision overflow. The legacy-mode `test_cast_float_to_decimal`
at `numeric.rs:1265` covers NaN/Inf-to-null, but nothing covers NaN/Inf under
ANSI, which is a distinct code path (closure returns `None` for a non-finite
value, then rescan must recompute `fits == false` and error). If a future
change made the rescan use a finite-only overflow predicate, this would
silently regress with no failing test.
Suggested change: extend the ANSI error test with `f64::NAN` and
`f64::INFINITY` inputs and assert `is_err()`, and assert the reported value
string is `"NaN"` / `"inf"` to lock the message.
```rust
let a: ArrayRef = Arc::new(Float64Array::from(vec![Some(1.0),
Some(f64::NAN)]));
assert!(cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2,
EvalMode::Ansi).is_err());
let a: ArrayRef = Arc::new(Float64Array::from(vec![Some(f64::INFINITY)]));
assert!(cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2,
EvalMode::Ansi).is_err());
```
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1287,6 +1292,48 @@ mod tests {
assert!(casted.is_null(9));
}
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_no_overflow_fast_path() {
+ // All values fit precision 10, scale 2, so the vectorized fast path
is taken and the
+ // input null is preserved.
+ let a: ArrayRef = Arc::new(Float64Array::from(vec![
+ Some(42.0),
+ Some(-1.5),
+ None,
+ Some(0.0),
+ ]));
+ let b =
+ cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2,
EvalMode::Legacy).unwrap();
+ let d = b.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 4200); // 42.00
+ assert_eq!(d.value(1), -150); // -1.50
+ assert!(d.is_null(2));
+ assert_eq!(d.value(3), 0);
+ assert_eq!(d.data_type(), &DataType::Decimal128(10, 2));
+ }
+
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_ansi_no_overflow() {
+ let a: ArrayRef = Arc::new(Float64Array::from(vec![Some(42.0), None,
Some(-1.5)]));
+ let b =
+ cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2,
EvalMode::Ansi).unwrap();
+ let d = b.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 4200);
+ assert!(d.is_null(1));
+ assert_eq!(d.value(2), -150);
+ }
+
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_ansi_overflow_errors() {
Review Comment:
`test_cast_float_to_decimal_ansi_overflow_errors` asserts only
`result.is_err()`. The whole point of the rescan is to reproduce Spark's exact
`NumericValueOutOfRange { value, precision, scale }` with the first offending
value in row order. A bare `is_err()` would still pass if the rescan returned
the wrong variant, the wrong value, or the second offender instead of the first.
Suggested change: match on the error and assert the fields.
```rust
match cast_floating_point_to_decimal128::<Float64Type>(&a, 4, 2,
EvalMode::Ansi) {
Err(SparkError::NumericValueOutOfRange { value, precision, scale }) => {
assert_eq!(value, "4242.42");
assert_eq!(precision, 4);
assert_eq!(scale, 2);
}
other => panic!("expected NumericValueOutOfRange, got {other:?}"),
}
```
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1287,6 +1292,48 @@ mod tests {
assert!(casted.is_null(9));
}
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_no_overflow_fast_path() {
+ // All values fit precision 10, scale 2, so the vectorized fast path
is taken and the
+ // input null is preserved.
+ let a: ArrayRef = Arc::new(Float64Array::from(vec![
+ Some(42.0),
+ Some(-1.5),
+ None,
+ Some(0.0),
+ ]));
+ let b =
+ cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2,
EvalMode::Legacy).unwrap();
+ let d = b.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 4200); // 42.00
+ assert_eq!(d.value(1), -150); // -1.50
+ assert!(d.is_null(2));
+ assert_eq!(d.value(3), 0);
+ assert_eq!(d.data_type(), &DataType::Decimal128(10, 2));
+ }
+
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_ansi_no_overflow() {
+ let a: ArrayRef = Arc::new(Float64Array::from(vec![Some(42.0), None,
Some(-1.5)]));
+ let b =
+ cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2,
EvalMode::Ansi).unwrap();
+ let d = b.as_primitive::<Decimal128Type>();
+ assert_eq!(d.value(0), 4200);
+ assert!(d.is_null(1));
+ assert_eq!(d.value(2), -150);
+ }
+
+ #[test]
+ #[cfg_attr(miri, ignore)]
+ fn test_cast_float_to_decimal_ansi_overflow_errors() {
+ // 4242.42 * 10^2 = 424242 does not fit precision 4 -> ANSI error.
+ let a: ArrayRef = Arc::new(Float64Array::from(vec![Some(1.0),
Some(4242.42)]));
+ let result = cast_floating_point_to_decimal128::<Float64Type>(&a, 4,
2, EvalMode::Ansi);
+ assert!(result.is_err());
+ }
+
Review Comment:
The rescan iterates `0..input.len()` and returns the first row where `fits`
is false, matching the old loop that erred on the first offending row. No test
pins this. A batch with two overflowing rows where the second has a different
value would not distinguish "first" from "last".
Suggested change: add an ANSI input like `[Some(1.0), Some(overflow_a),
Some(overflow_b)]` where `overflow_a` and `overflow_b` stringify differently,
and assert the error reports `overflow_a`.
--
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]
