mbutrovich commented on code in PR #4939:
URL: https://github.com/apache/datafusion-comet/pull/4939#discussion_r3591242909
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1160,6 +1153,60 @@ 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
Review Comment:
`native/spark-expr/src/conversion_funcs/numeric.rs:1159-1160` (new test
`test_cast_int_to_decimal128_overflow_legacy_nulls`) says the test exercises
"the vectorized sentinel + masking path." The new implementation has no
sentinel value and no second masking pass. `unary_opt` writes nulls directly.
The comment is left over from an earlier design and will mislead the next
reader.
Suggested change: replace with something like "exercises the vectorized
null-on-overflow path with valid values and an input null preserved."
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -663,48 +663,41 @@ where
T: ArrowPrimitiveType,
T::Native: Into<i128>,
{
- let mut builder = Decimal128Builder::with_capacity(array.len());
let multiplier = 10_i128.pow(scale as u32);
- for i in 0..array.len() {
- if array.is_null(i) {
- builder.append_null();
- } else {
- let v = array.value(i).into();
- let scaled = v.checked_mul(multiplier);
- match scaled {
- Some(scaled) => {
- if !is_validate_decimal_precision(scaled, precision) {
- match eval_mode {
- EvalMode::Ansi => {
- return Err(SparkError::NumericValueOutOfRange {
- value: v.to_string(),
- precision,
- scale,
- });
- }
- EvalMode::Try | EvalMode::Legacy =>
builder.append_null(),
- }
- } else {
- builder.append_value(scaled);
- }
+ // Single vectorized pass: a value that overflows the multiply or does not
fit the output
+ // precision maps to null. `unary_opt` only applies the closure to
non-null slots and carries
+ // the input null buffer over, replacing the per-element builder loop
without a second pass.
+ let result: Decimal128Array = array.unary_opt::<_, Decimal128Type>(|v| {
Review Comment:
`native/spark-expr/src/conversion_funcs/numeric.rs`: the fast path closure
computes `v.checked_mul(multiplier).filter(|scaled|
is_validate_decimal_precision(*scaled, precision))`, and the ANSI rescan
recomputes the same thing inverted as `checked_mul(multiplier).map(|scaled|
!is_validate_decimal_precision(...)).unwrap_or(true)`. Two spellings of one
predicate invite drift where one is updated and the other is not.
Suggested change: extract a small local closure or helper, for example:
```rust
let fits = |v: i128| -> Option<i128> {
v.checked_mul(multiplier)
.filter(|scaled| is_validate_decimal_precision(*scaled, precision))
};
let result: Decimal128Array = array.unary_opt::<_, Decimal128Type>(|v|
fits(v.into()));
// rescan:
if fits(v).is_none() { return Err(SparkError::NumericValueOutOfRange { ..
}); }
```
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -1160,6 +1153,60 @@ mod tests {
assert_eq!(decimal_array.value(1), -10000); // -100 * 10^2
assert!(decimal_array.is_null(2));
}
+
+ #[test]
Review Comment:
The three new tests cover Legacy null-on-overflow, ANSI no-overflow, and
ANSI overflow-error. None cover `EvalMode::Try`. Try shares the Legacy null
branch today, so this is untested rather than broken, but Try is a distinct
enum arm that a future edit could regress silently, and the PR text claims all
eval modes are covered.
Suggested change: add a `test_cast_int_to_decimal128_overflow_try_nulls`
mirroring the Legacy test with `EvalMode::Try`, asserting overflow maps to null
and the input null is preserved.
--
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]
