mbutrovich commented on code in PR #4938:
URL: https://github.com/apache/datafusion-comet/pull/4938#discussion_r3591224473
##########
native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs:
##########
@@ -343,6 +349,19 @@ mod tests {
assert!(result.is_err());
}
+ #[test]
+ fn test_overflow_with_nulls_legacy() {
Review Comment:
The added `test_overflow_with_nulls_legacy` (`decimal_rescale_check.rs:352`)
covers the mixed case, and existing tests cover single-value overflow. There is
no array test where every value overflows, which is the shape that exercises
`contains` finding the sentinel at index 0 and the masking pass nulling the
whole array.
Suggested change: add a legacy test over `vec![Some(10_000), Some(20_000),
Some(30_000)]` into precision 4 and assert all three slots are null.
##########
native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs:
##########
@@ -197,7 +197,13 @@ impl PhysicalExpr for DecimalRescaleCheckOverflow {
rescale_and_check(value, delta, scale_factor, bound,
fail_on_error)
})?;
- let result = if !fail_on_error {
+ let result = if !fail_on_error &&
result.values().contains(&i128::MAX) {
Review Comment:
`decimal_rescale_check.rs:200-206`. The new comment reads "`any`
short-circuits at the first sentinel," but the code calls
`result.values().contains(&i128::MAX)`. Both short-circuit, so behavior is
right, the prose just names the wrong method and will confuse the next reader.
Suggested change: replace "`any`" with "`contains`" in the comment.
##########
native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs:
##########
@@ -343,6 +349,19 @@ mod tests {
assert!(result.is_err());
}
+ #[test]
Review Comment:
No test pins the value exactly at `10^p - 1` (fits) versus `10^p`
(overflows) for the same output precision, which is the boundary the whole
predicate turns on.
Suggested change: add a legacy test with output precision 4 over
`vec![Some(9999), Some(10_000)]` at scale 0, asserting slot 0 keeps `9999` and
slot 1 is null. This makes the exact bound a regression guard rather than an
implied property.
--
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]
