andygrove commented on code in PR #5083:
URL: https://github.com/apache/datafusion-comet/pull/5083#discussion_r3692850082
##########
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,
+ ));
Review Comment:
Confirmed and fixed in 815cc8a6f. I ran the new test on `spark-3.5` before
changing anything, and it fails exactly as you predicted:
```
- cast BooleanType to DecimalType where 10^scale overflows precision ***
FAILED ***
"...ALUE_OUT_OF_RANGE] 1[0] cannot be represent..." did not equal
"...ALUE_OUT_OF_RANGE] 1[] cannot be represent..."
(CometCastSuite.scala:2431)
```
So the PR as it stood would have failed CI on `spark-3.4`/`spark-3.5`, and
your reading of why `spark-4.x` misses it is right too — the 40-character
prefix stops inside `[NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION] ` and never
reaches the value. Worth noting the decimal-to-decimal escape hatch just above
(`assert(cometMessage.contains("too large to store"))`) does not apply here
either, since the *input* is boolean, so the exact comparison is what runs.
Fixed by passing the pre-scale logical value, matching
`cast_int_to_decimal128_internal`. The four `cast BooleanType to DecimalType*`
tests now pass on `spark-3.5`.
##########
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")
- {
- // Use the scaled value as it's the only non-zero value that
could overflow
- crate::error::decimal_overflow_error(scaled_val, precision,
scale)
- } else {
- SparkError::Arrow(Arc::new(e))
- }
- })?;
-
+ .map_err(|e| SparkError::Arrow(Arc::new(e)))?;
Ok(Arc::new(decimal_array))
}
Review Comment:
Thanks for checking this explicitly — agreed, and no change made here.
Leaving `is_validate_decimal_precision` and the `eval_mode` precheck as they
are.
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -189,6 +189,18 @@ class CometCastSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
castTest(generateBools(), DataTypes.createDecimalType(30, 0))
}
+ test("cast BooleanType to DecimalType where 10^scale overflows precision") {
+ // Regression for https://github.com/apache/datafusion-comet/issues/5068.
+ // `true` scales to 10^scale, which does not fit in
DECIMAL(1,1)/(2,2)/(3,3).
+ // Spark returns NULL in legacy/try mode and only throws under ANSI.
+ Seq(
+ DataTypes.createDecimalType(1, 1),
+ DataTypes.createDecimalType(2, 2),
+ DataTypes.createDecimalType(3, 3)).foreach { dt =>
+ castTest(generateBools(), dt)
+ }
+ }
Review Comment:
Added in 815cc8a6f. The comment now names ANSI explicitly: that `castTest`'s
default `testAnsi = true` is what drives the `EvalMode::Ansi` throw path in
`boolean.rs`, and that on Spark 3.4/3.5 the message comparison is exact, so
this test also pins the reported value to Spark's logical `1` rather than the
scaled `10^scale`. That second part is worth stating given it is exactly what
the first thread caught.
--
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]
