mbutrovich commented on code in PR #5083:
URL: https://github.com/apache/datafusion-comet/pull/5083#discussion_r3691562696
##########
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:
`is_validate_decimal_precision` from arrow-rs (`arrow-data/src/decimal.rs`,
re-exported via `arrow::datatypes`) is the right existing kernel for the
precision check and is used consistently with the same import already in
`numeric.rs`. There is no arrow-rs or DataFusion cast kernel that already
implements Spark's `nullOnOverflow = !ansiEnabled` semantics for decimal casts,
since that is Spark-specific behavior rather than a general Arrow cast concept,
so a hand-written check here is expected and this part does not need to change.
Dropping the old string-matching error detection (`e.to_string().contains("too
large to store in a Decimal128")`) in favor of the `eval_mode` precheck removes
a fragile pattern and is worth keeping as is.
##########
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:
The new test relies on `castTest`'s default `testAnsi = true` to exercise
the ANSI overflow path, but nothing in the test documents that this is
intentional or names the ANSI behavior being checked, unlike the comment block
above it which only describes the legacy/try semantics. Add a line noting that
`castTest`'s ANSI branch, enabled by default, is what exercises the
`EvalMode::Ansi` throw path added in `boolean.rs`, since a reader scanning this
test in isolation cannot otherwise tell that ANSI is covered here at all.
##########
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:
The ANSI error reports the wrong value. `decimal_overflow_error(scaled_val,
precision, scale)` passes the already-scaled internal representation
(`10_i128.pow(scale)`, e.g. 10 for `Decimal(1,1)`). Spark's actual path for
boolean-to-decimal ANSI overflow is `Cast.scala` calling `toPrecision(if (b)
Decimal.ONE else Decimal.ZERO, target, ...)`, and `Decimal.toPrecision` on
failure calls `DataTypeErrors.cannotChangeDecimalPrecisionError(this,
precision, scale, context)` where `this` is `Decimal.ONE`, i.e. the unscaled
logical value 1. `Decimal.ONE.toPlainString` is `"1"`, so Spark's message is `1
cannot be represented as Decimal(1, 1)....`. On the JVM side,
`ShimSparkErrorConverter`
(`spark/src/main/spark-3.4/org/apache/spark/sql/comet/shims/ShimSparkErrorConverter.scala:107-113`,
same in spark-3.5) reconstructs `Decimal(params("value").toString)` and calls
the same `cannotChangeDecimalPrecisionError`, so whatever string Comet puts in
`value` is parsed back as a `Decimal` verbatim. Si
nce Comet passes `"10"`, the reconstructed message is `10 cannot be
represented as Decimal(1, 1)...`, which does not match Spark's `1 cannot be
represented as Decimal(1, 1)...`.
This mismatch predates this PR (the old `map_err` branch also passed
`scaled_val` into `decimal_overflow_error`), but it was never exercised by any
test until this PR added the `CometCastSuite` regression test that now runs
`true` values through `DECIMAL(1,1)/(2,2)/(3,3)` under ANSI. On the `spark-3.4`
and `spark-3.5` profiles, `castTest`'s ANSI branch asserts `cometMessage ==
sparkMessage` exactly
(`spark/src/test/scala/org/apache/comet/CometCastSuite.scala:2431`), so the new
test will fail the exact-message comparison on those profiles. On
`spark-4.0`/`spark-4.1` it will not be caught, because the `isSpark40Plus`
branch only compares `cometMessage.substring(0, min(40, cometMessage.length))`
as a prefix (`CometCastSuite.scala:2426-2428`), and 40 characters only covers
the `[NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION] ` tag, not the value.
Fix: pass the pre-scale logical value, matching the convention used by
`cast_int_to_decimal128_internal`
(`native/spark-expr/src/conversion_funcs/numeric.rs:750-754`), which reports
the original face-value input, not the post-multiplier value. For boolean, that
is `1` for `true`: replace `decimal_overflow_error(scaled_val, precision,
scale)` with `decimal_overflow_error(1, precision, scale)`.
##########
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:
This is the third hand-written copy of the same decision shape: check
`is_validate_decimal_precision`, then match `eval_mode` to decide null vs
`decimal_overflow_error`. The other two copies are
`native/spark-expr/src/conversion_funcs/numeric.rs:744-772`
(`cast_int_to_decimal128_internal`, both the `checked_mul` overflow branch and
the post-multiply precision-check branch) and `numeric.rs:915-937`
(`cast_floating_point_to_decimal128`'s ANSI rescan). Each function's scan
strategy should stay different: boolean's domain is 2-valued so a single
precheck outside the loop is correct and should not be forced into a per-row
loop like the int/float paths. But the branch body itself, "precision check
fails, Ansi errors with `decimal_overflow_error(value, precision, scale)`, else
null", is identical in shape across all three sites and has already drifted
once: boolean passes the scaled value while int/float pass the face value (see
the finding above). Extract a small helper, e.g. `fn decima
l_overflow_result(value: i128, precision: u8, scale: i8, eval_mode: EvalMode)
-> SparkResult<Option<i128>>` in `numeric.rs` or a shared `decimal.rs`, and
have all three call sites call it with the correct pre-scale `value` argument.
This forces the value convention to be decided once instead of letting it
diverge a third time.
--
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]
