Skip to content

fix: make wide date-to-timestamp casts safe - #5457

Open
sunchao wants to merge 2 commits into
apache:mainfrom
sunchao:dev/chao/codex/fix-wide-date-timestamp
Open

fix: make wide date-to-timestamp casts safe#5457
sunchao wants to merge 2 commits into
apache:mainfrom
sunchao:dev/chao/codex/fix-wide-date-timestamp

Conversation

@sunchao

@sunchao sunchao commented Aug 24, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #5456. Follow-up to #5443.

Why are the changes needed?

Native make_date now accepts Spark's wide date range, but its timestamp consumers still assume narrower ranges. For example, casting make_date(300000, 6, 15) to TIMESTAMP_NTZ silently wraps the microsecond count in release builds instead of throwing Spark's ArithmeticException: long overflow. Casting make_date(262143, 1, 1) to TIMESTAMP in 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_date null 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:MM fixed-offset casts. Apply the offset in seconds before checked multiplication to microseconds, so the final range check includes the timezone adjustment. Overflow becomes a plain ArithmeticException in both legacy and ANSI modes. Nullable scalar TRY_CAST returns 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 take with 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 core rebuilt 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.
  • Full CometNativeCastSuite, CometCodegenSuite, CometCodegenSourceSuite, CometCodegenHOFSuite, and SparkErrorConverterSuite on Spark 4.1.3: 332 passed, with 8 existing ignored tests.
  • Focused CometNativeCastSuite DateType to and SparkErrorConverterSuite on Spark 3.5.9 and 4.0.4: 27 passed per profile. All JVM tests used JDK 17, with clean builds between Spark profiles.
  • The new native null-parent and sliced-input regressions failed before the fix and passed afterward; visible-overflow controls still throw.
  • An independent harness linked against the rebuilt production spark_cast passed 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.
  • Native SQL reproductions for reused IF aliases 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, and git diff --check passed.

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.

@sunchao

sunchao commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

cc @peterxcli @comphead

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") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also handle IntegerOverflow here? as I remember there are at least these two type of arithmetic overflow in spark sql error result

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering can ArithmeticOverflow be reused?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sunchao
sunchao force-pushed the dev/chao/codex/fix-wide-date-timestamp branch from 0bd0041 to 8b072ab Compare August 25, 2026 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Date-to-timestamp casts can overflow or panic for wide dates

3 participants