andygrove commented on code in PR #5083:
URL: https://github.com/apache/datafusion-comet/pull/5083#discussion_r3692851845
##########
native/spark-expr/src/conversion_funcs/boolean.rs:
##########
@@ -32,28 +32,32 @@ pub fn cast_boolean_to_decimal(
array: &ArrayRef,
precision: u8,
scale: i8,
+ eval_mode: EvalMode,
) -> SparkResult<ArrayRef> {
let bool_array = array.as_boolean();
let scaled_val = 10_i128.pow(scale as u32);
+
+ // Spark's Cast uses `nullOnOverflow = !ansiEnabled`: legacy/try return
NULL
+ // on overflow, only ANSI raises. `false` maps to 0 which always fits, so
+ // overflow only happens for `true` when 10^scale exceeds `precision`.
+ let overflows = !is_validate_decimal_precision(scaled_val, precision);
+ if overflows && eval_mode == EvalMode::Ansi {
+ return Err(crate::error::decimal_overflow_error(
+ scaled_val, precision, scale,
+ ));
+ }
+
let result: Decimal128Array = bool_array
.iter()
- .map(|v| v.map(|b| if b { scaled_val } else { 0 }))
+ .map(|v| match v {
+ Some(false) => Some(0),
+ Some(true) if !overflows => Some(scaled_val),
+ _ => None,
+ })
.collect();
-
- // Convert Arrow decimal overflow errors to SparkError
let decimal_array = result
.with_precision_and_scale(precision, scale)
- .map_err(|e| {
- if matches!(e, arrow::error::ArrowError::InvalidArgumentError(_))
- && e.to_string().contains("too large to store in a Decimal128")
Review Comment:
Done in 815cc8a6f, with one change to the signature you proposed.
`SparkResult<Option<i128>>` with an `i128` value does not fit the third
site: `cast_floating_point_to_decimal128` reports an **f64**
(`input_value.to_string()`), so forcing it through an `i128` parameter would
change the string it reports (`1e30` vs a 31-digit integer) and break message
parity for the float path. And the `Option<i128>` return would always be `None`
in practice, since the helper never produces a value — only an error or a null.
So the helper is:
```rust
pub(crate) fn decimal_overflow_or_null<T: Display>(
value: T,
precision: u8,
scale: i8,
eval_mode: EvalMode,
) -> SparkResult<()>
```
`Ok(())` means "caller should emit NULL", `Err` is the ANSI error. Generic
over `Display` so each site reports its own natural type, and the doc comment
states the pre-scale convention and why (Spark's `Decimal.toPrecision` reports
the logical value, and `ShimSparkErrorConverter` parses the string back into a
`Decimal` verbatim). It lives in `conversion_funcs/utils.rs`, which both
`boolean.rs` and `numeric.rs` already share.
All three sites route through it, and as you asked, each keeps its own scan
strategy — boolean's single precheck stays outside the loop rather than
becoming per-row.
One extra simplification it enabled: in `cast_int_to_decimal128_internal`
the `checked_mul` overflow branch and the precision-check branch had identical
bodies, both reporting `v`, so they collapse into one arm:
```rust
match scaled {
Some(scaled) if is_validate_decimal_precision(scaled, precision) => {
builder.append_value(scaled)
}
// Either the multiply overflowed i128 or the product exceeds
`precision`.
_ => {
decimal_overflow_or_null(v, precision, scale, eval_mode)?;
builder.append_null();
}
}
```
62 `conversion_funcs` unit tests pass and clippy `--all-targets` is 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]
