Re: [PR] perf: reuse validity buffers and use unary kernels in native expressions [datafusion-comet]

["sunchao (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org