fix: make wide date-to-timestamp casts safe - #5457
Conversation
| 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") { |
There was a problem hiding this comment.
also handle IntegerOverflow here? as I remember there are at least these two type of arithmetic overflow in spark sql error result
There was a problem hiding this 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.
| } | ||
|
|
||
| /// 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> { |
There was a problem hiding this comment.
looks like this is for:
Checked casts must also ignore values that are not logically present. A native
IF(flag, s, NULL)can leave an overflowing date in the child buffer of a null struct or map. Casting that null container must return null, not evaluate the hidden date and throw. Sliced lists and maps can likewise retain unused child values before or after their visible rows.
so it seems like the root cause is native IF(flag, s, NULL) doesnt actually respect the null buffer? should we turn to fix it instead?
There was a problem hiding this 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.
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.
|
|
||
| // Spark's checked date/timestamp conversions throw this even with ANSI disabled. | ||
| #[error("long overflow")] | ||
| LongOverflow, |
There was a problem hiding this comment.
wondering can ArithmeticOverflow be reused?
There was a problem hiding this 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.
0bd0041 to
8b072ab
Compare
Which issue does this PR close?
Closes #5456. Follow-up to #5443.
Why are the changes needed?
Native
make_datenow accepts Spark's wide date range, but its timestamp consumers still assume narrower ranges. For example, castingmake_date(300000, 6, 15)toTIMESTAMP_NTZsilently wraps the microsecond count in release builds instead of throwing Spark'sArithmeticException: long overflow. Castingmake_date(262143, 1, 1)toTIMESTAMPin UTC panics inside chrono even though the timestamp fits in a signed 64-bit value.The fix needs to preserve the expanded date range while making both casts safe. Restoring the old
make_datenull restriction would lose the compatibility improvement from #5443.Checked casts must also ignore values that are not logically present. A native
IF(flag, s, NULL)can leave an overflowing date in the child buffer of a null struct or map. Casting that null container must return null, not evaluate the hidden date and throw. Sliced lists and maps can likewise retain unused child values before or after their visible rows.What changes were proposed in this PR?
Use direct epoch-day arithmetic for NTZ, UTC, and standard
+/-HH:MMfixed-offset casts. Apply the offset in seconds before checked multiplication to microseconds, so the final range check includes the timezone adjustment. Overflow becomes a plainArithmeticExceptionin both legacy and ANSI modes. Nullable scalarTRY_CASTreturns null for overflowing rows without discarding valid rows in the same batch.Other date-to-timestamp timezone spellings use Spark's existing JVM codegen dispatcher, or Spark row execution when the dispatcher is disabled. This keeps historical and far-future timezone rules with Spark. This routing applies to ordinary dates too, so named-zone casts may be slower; UTC and standard fixed offsets retain native execution.
Spark's nullability inference can miss date-to-timestamp overflow in
TRY_CAST. Non-nullable scalar and complex TRY casts containing this conversion therefore stay on Spark's row path, avoiding nulls in non-nullable Arrow fields or map keys. The codegen eligibility check also catches these casts inside a larger dispatched expression.Before recursively casting a struct, list, or map with null rows or unused backing values, use Arrow
takewith nullable identity indices. This propagates struct nulls to children and compacts list/map entries while preserving row validity, field metadata, and non-nullable map keys. Scalar casts and contiguous containers without nulls keep their existing path. Visible overflowing dates still raise the same error.How was this PR tested?
make corerebuilt the native library before JVM testing.cargo test --manifest-path native/Cargo.toml -p datafusion-comet-spark-expr -p datafusion-comet-common --lib: 684 passed.CometNativeCastSuite,CometCodegenSuite,CometCodegenSourceSuite,CometCodegenHOFSuite, andSparkErrorConverterSuiteon Spark 4.1.3: 332 passed, with 8 existing ignored tests.CometNativeCastSuite DateType toandSparkErrorConverterSuiteon Spark 3.5.9 and 4.0.4: 27 passed per profile. All JVM tests used JDK 17, with clean builds between Spark profiles.spark_castpassed 69,120 list/map casts across slice boundaries, null masks, and legacy/ANSI/TRY modes, plus nested, non-nullable, empty, all-null, and visible-overflow controls.IFaliases containing structs and maps match Spark in both ANSI settings, with JVM dispatch disabled and native operators verified.cargo clippy --manifest-path native/Cargo.toml -p datafusion-comet-spark-expr --all-targets -- -D warnings,cargo fmt --manifest-path native/Cargo.toml --all -- --check, Spotless, Scalastyle, andgit diff --checkpassed.The JVM regressions use Parquet-backed columns and assert native execution, JVM dispatcher execution, or full Spark fallback as appropriate. They cover wide positive/negative dates, timestamp boundaries, offset-induced overflow, nulls, both ANSI settings, and nested TRY casts. The null-container regression also covers map keys and arrays of structs. These are local checks, not hosted CI results.