sunchao commented on code in PR #6953:
URL: https://github.com/apache/datafusion-comet/pull/6953#discussion_r4266087593
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -617,46 +565,27 @@ macro_rules! cast_decimal_to_int32_up {
.downcast_ref::<Decimal128Array>()
.expect("Expected a Decimal128ArrayType");
- let output_array = match $eval_mode {
- EvalMode::Ansi => cast_array
- .iter()
- .map(|value| match value {
- Some(value) => {
- let divisor = 10_i128.pow($scale as u32);
- let truncated = value / divisor;
- let is_overflow = truncated.abs() >
$max_dest_val.into();
- if is_overflow {
- return Err(cast_overflow(
- &format!(
- "{}BD",
- format_decimal_str(
- &value.to_string(),
- $precision as usize,
- $scale
- )
- ),
- &format!("DECIMAL({},{})", $precision, $scale),
- $dest_type_str,
- ));
- }
- Ok(Some(truncated as $rust_dest_type))
- }
- None => Ok(None),
- })
- .collect::<Result<$dest_array_type, _>>()?,
- _ => cast_array
- .iter()
- .map(|value| match value {
- Some(value) => {
- let divisor = 10_i128.pow($scale as u32);
- let truncated = value / divisor;
- Ok::<Option<$rust_dest_type>, SparkError>(Some(
- truncated as $rust_dest_type,
- ))
- }
- None => Ok(None),
- })
- .collect::<Result<$dest_array_type, _>>()?,
+ let divisor = 10_i128.pow($scale as u32);
+ // The legacy conversion cannot fail, so `unary` maps the values
buffer in one pass and
+ // keeps the null buffer. `try_unary` only visits valid slots, so an
out-of-range value
+ // under a null cannot raise a spurious overflow error.
+ let output_array: $dest_array_type = match $eval_mode {
+ EvalMode::Ansi => cast_array.try_unary(|value| {
+ let truncated = value / divisor;
+ let is_overflow = truncated.abs() > $max_dest_val.into();
+ if is_overflow {
+ return Err(cast_overflow(
+ &format!(
+ "{}BD",
+ format_decimal_str(&value.to_string(), $precision
as usize, $scale)
+ ),
+ &format!("DECIMAL({},{})", $precision, $scale),
+ $dest_type_str,
+ ));
+ }
+ Ok(truncated as $rust_dest_type)
+ })?,
+ _ => cast_array.unary(|value| (value / divisor) as
$rust_dest_type),
Review Comment:
[P2] Preserve null short-circuiting for negative-scale decimal inputs. With
`spark.sql.legacy.allowNegativeScaleOfDecimal=true`, an all-null
`DecimalType(38, -1)` column can reach this cast because
`CometCast.canCastFromDecimal` accepts integral targets without a scale guard.
Previously the iterator skipped every null and returned nulls, matching Spark.
Now `10_i128.pow(scale as u32)` wraps to zero in release builds, and `unary`
divides by that zero even beneath null slots, failing the query. The same
regression affects the byte/short macro at line 546. Please return the
null-only result before calculating the divisor in both macros, or handle
negative scales safely, and cover this input in regression tests.
Evidence: Reproduced using verbatim decimal-cast macros from the specified
base and head with Arrow 59.3.0 and release overflow checks disabled, matching
the repository profile. Input: `Decimal128Array::from(vec![None::<i128>;
3]).with_precision_and_scale(38, -1).unwrap()`. Calling
`spark_cast_nonintegral_numeric_to_integral` with `EvalMode::Legacy` and each
of `Int8`, `Int16`, `Int32`, and `Int64` returns three nulls on the base and
panics with `attempt to divide by zero` on the head. Harness:
`/tmp/pr6953-kernel-review-7d2rw4x3`, test `negative_scale_nulls_regression`.
Spark's `UnaryExpression.eval` returns null before invoking the cast, and
Comet's cast dispatcher has no equivalent all-null guard. Full JVM/native
execution was unavailable because of the documented dependency-resolution
limitation.
--
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]
