sunchao commented on code in PR #5457:
URL: https://github.com/apache/datafusion-comet/pull/5457#discussion_r3855042866
##########
spark/src/main/scala/org/apache/comet/SparkErrorConverter.scala:
##########
@@ -117,8 +117,13 @@ object SparkErrorConverter extends ShimSparkErrorConverter
{
val summary: String = errorJson.summary.getOrElse("")
- // Delegate to version-specific shim - let conversion exceptions propagate
- val optEx = convertErrorType(errorJson.errorType, errorClass, params,
sparkContext, summary)
+ // Math.multiplyExact throws a plain JVM exception in every Spark version,
without an
+ // ANSI error class or configuration advice. Delegate other errors to the
version-specific shim.
+ val optEx = if (errorJson.errorType == "LongOverflow") {
Review Comment:
Spark does have integer-overflow cases, but this date-to-timestamp path
widens the Date32 value to `i64` before converting to seconds. Only the checked
`i64` microsecond multiplication can overflow, so there is no `IntegerOverflow`
producer on this path.
Existing ANSI integer arithmetic already uses `ArithmeticOverflow` and keeps
its structured Spark exception handling. I'm keeping this new plain-exception
case scoped to the long overflow that this conversion actually produces.
##########
native/common/src/error.rs:
##########
@@ -69,6 +69,10 @@ pub enum SparkError {
#[error("[ARITHMETIC_OVERFLOW] {from_type} overflow. If necessary set
\"spark.sql.ansi.enabled\" to \"false\" to bypass this error.")]
ArithmeticOverflow { from_type: String },
+ // Spark's checked date/timestamp conversions throw this even with ANSI
disabled.
+ #[error("long overflow")]
+ LongOverflow,
Review Comment:
The existing `ArithmeticOverflow` maps to Spark's structured
`SparkArithmeticException` with error class `ARITHMETIC_OVERFLOW`, including
ANSI configuration advice.
For DATE to TIMESTAMP, Spark instead calls `daysToMicros` ->
`instantToMicros` -> `Math.multiplyExact`, which throws a plain
`ArithmeticException("long overflow")` even with ANSI disabled. Reusing the
existing variant unchanged would change both the exception class and message,
so `LongOverflow` preserves that distinction. The regressions check the exact
class and message with ANSI both on and off.
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -418,6 +419,36 @@ pub(crate) fn cast_array(
Ok(spark_cast_postprocess(cast_result?, &from_type, to_type))
}
+/// Recursive casts must not evaluate child values hidden by a null parent or
outside a slice.
+fn prepare_nested_cast_input(array: ArrayRef) -> DataFusionResult<ArrayRef> {
Review Comment:
`IF` is respecting Arrow's null semantics here. For `IF(flag, column,
NULL)`, DataFusion can use Arrow's `nullif` to mask the parent without
rewriting the child buffers. A null struct is allowed to retain nonnull child
values, as described in the [Arrow struct-validity
specification](https://arrow.apache.org/docs/format/Columnar.html#struct-validity).
The recursive cast must honor that enclosing validity before evaluating the
children. We also need this normalization for sliced lists and maps, whose
child arrays retain values outside the visible slice independently of `IF`.
Changing only `IF` would leave those cases uncovered. The regressions cover
both hidden parent values and slices, preserve visible-overflow errors, and
verify that map keys remain nonnull.
--
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]
