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
Thanks for following up on #5443 so quickly. The panic inside chrono and the silent wraparound are both real, and the observation that a null struct or a sliced list can hide an overflowing value in a non-nullable child buffer is a subtle one that I would not have thought of. The I think the fix costs more than it needs to, though, and I would like to see that addressed before merge. Region-zone dates lose native execution entirely
The failing values require a date beyond roughly year 262143, which in practice means If the fallback does stay, please put a number in the description. How much slower is
Could this be gated on whether the cast is actually fallible, so that infallible widening casts keep their current path? Even a coarse check would avoid the cost in the common case.
The timezone allowlist and the native parser can disagree Scala uses Concretely, the Scala regex rejects several spellings that mean UTC and would be fine: Dead code question
|
|
Updated in 23ae9bb95. @andygrove I checked each suggestion against the cast boundary and added changes for the avoidable copies and fixed-zone aliases.
Final validation against the revised release library: 690 native expression/common unit tests and 28 focused Spark 4.1.3 date-cast/error tests passed; all-target Clippy and the full JVM reactor style checks passed. An earlier full cast/error run had 184 passes and one failure in a newly added test that expected native execution for the already unsupported direct |
|
Updated in 3bd44648e. Merged Apache main Because main changes JNI and shuffle protocols, I rebuilt the release native library and ran the full Spark 4.1.3 reactor with the 28 date/error tests, then 8 RSS/protocol/datatype tests. All 36 passed and loaded the newly built library. The 771 native common/expression/protocol/shuffle unit tests, all-target expression Clippy, formatting, and style checks also passed. The PR body now records this merged validation separately. The benchmark results remain pinned to |
|
Fixed both review issues. 10a06cc64 keeps unbounded non-TRY 129cc1646 removes the redundant interpolator that failed Scalafix. I reproduced that failure against the unchanged-head test file and verified that the full patched Spark source passes CI's exact syntactic check. The full Spark 4.1.3 JVM reactor passed 201 tests, with 8 existing ignored tests and no failures, including the full cast/error suites and ten NTZ SQL configurations. The actual loaded native library matches this PR's unchanged native source. Spotless and Scalastyle also pass. The PR description now records the fallback cost and validation limits; CI for the new commit is pending. |
- Revert cast_date_to_timestamp to main's version to avoid conflicting with apache#5457, which rewrites the same function as a safety fix - Document the Int32 input guarantee and zero-copy reinterpret path in date_from_unix_date - Make the microsecond-physical/millisecond-target rejection message actionable (report link + scan fallback workaround) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed the remaining Parquet reader-boundary issue in The implicit conversion from a physical DATE column to a requested TIMESTAMP_NTZ column bypassed the Catalyst cast guard. On Spark 4.x, a file with an overflowing date at row 5000 should still let The fix keeps V1 Parquet scans requesting top-level NTZ data columns on Spark and blocks both Arrow bridge paths, including with native scans disabled or AQE enabled. Ordinary NTZ files conservatively fall back too because the logical schema cannot prove each file's physical type. Pruned, partition-only, nested, and Iceberg cases retain their existing scope. The guide explains this tradeoff and the Spark 3.x schema rejection versus Spark 4.x conversion behavior. Existing native kernel/operator assertions remain, using materialized or nested fixtures where needed. Validation passed:
The PR description now includes the new behavior and validation, while retaining earlier benchmark revisions. Hosted CI for this commit is pending. |
4 participants
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 fixed whole-minute timezone offsets. Resolve fixed Java aliases through Spark and serialize a canonical
UTCor+/-HH:MMoffset, so spellings such asGMT,Etc/UTC,Z,UTC+00:00,GMT+05:30, andEtc/GMT+8stay native, including inside nested 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.Checked arithmetic must also run only for rows Spark would consume. A whole-batch NTZ cast can otherwise throw for a later row below
LIMIT, or a later semi/anti join candidate after an earlier match. Non-TRYDATEtoTIMESTAMP_NTZcasts now keep the affected operator in Spark when the input date range is not provably safe. Both native serialization and the JVM batch dispatcher enforce this restriction, including nested casts and enclosing expressions.This deliberately includes ordinary date columns: without a range proof, their projections, filters, or join conditions use Spark row execution. Safe literal/null dates and
date_from_unix_dateof byte/short inputs remain eligible for native execution. Nullable scalarTRY_CAST, fixed-zoneDATEtoTIMESTAMP, and ordinary widening retain their existing routes and guards. The checked Rust arithmetic and hidden-container handling are unchanged. No new timing is claimed for the NTZ fallback; the benchmark tables below retain their stated earlier revisions and scopes.V1 Parquet schema conversion needs the same protection even when there is no Catalyst
Castin the query. Reading a physicalDATEcolumn asTIMESTAMP_NTZcan evaluate an overflowing value in a batch Spark would never consume. The native reader now declines scans that request a top-level NTZ data column, and the Spark-to-Arrow rule blocks both row and columnar bridges for those scans. Preserving each Spark batch in the columnar bridge is insufficient: a native filter can request another batch before returning its first selected row toLIMIT.This is a conservative fallback for ordinary NTZ files as well, because the logical scan schema cannot establish each file's physical type. It applies even with
spark.comet.convert.parquet.enabled=trueor native scans disabled. Pruned NTZ fields, partition-only NTZ columns, nested fields using the separate conversion path, and native Iceberg scans are unaffected. Spark 3.x rejects the DATE-to-NTZ reader conversion; Spark 4.0+ supports it and can throw while decoding a consumed batch. Using Spark's reader without either bridge preserves both behaviors. Native kernel and operator tests now use fully materialized or nested Parquet fixtures where necessary, with their native execution assertions retained; separate tests assert the reader fallback. No new NTZ performance measurement is claimed.Named regions and fixed offsets with seconds 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. A chrono range guard cannot preserve valid Spark results:
+262143-01-01in Los Angeles is a valid timestamp outside chrono's range, and the pinned chrono-tz version is one hour wrong for Los Angeles on2101-07-04, inside that range. Native parsing also drops offset seconds, so subminute offsets cannot use the minute-offset path. This routing applies to ordinary dates too. In the local ordinary-date column benchmark, the revised head's Los Angeles query means averaged 12.3% higher through the dispatcher and 21.5% higher with dispatch disabled than the exact PR base, which ran both cases natively. UTC remained native and its average query means fell 39–45%. These are workload-specific whole-query costs; the separate benchmark protocol and raw per-JVM results are below.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 applying a potentially fallible cast to 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. A conservative whitelist avoids this copy for signed integer widenings and unchanged fixed-width leaves, including struct trees, beneath the current container. Structs can then reuse their input, as can lists/maps with sparse null rows and few hidden child values. Nested lists/maps and unchanged variable-width children retain the existing compaction path, avoiding large hidden allocations and buffer retention. Lists/maps with unused backing values or dense null rows still compact. Scalar casts, whole-cast identity, and contiguous containers without nulls keep their existing path. Visible overflowing dates still throw in legacy and ANSI modes; TRY behavior and admission remain unchanged. The native reuse optimization was measured separately against the original PR head, not the PR base: the 13 targeted nullableInt32toInt64struct and sparse list/map shapes used 12–92% less batch time, with no reproducible regression among the 46 measured shapes after repeating initial control flags.How was this PR tested?
Reader follow-up validation (2026-08-28): the full Spark 4.1.3 Maven reactor passed 221 selected tests, with 3 expected version-dependent cancellations and 8 existing ignored tests. This includes the entire native cast and error-converter suites, all reader regressions, and the affected timestamp expression, dispatcher, dictionary-sort, and NTZ SQL fixtures. The previously failing short-circuiting cast test now reaches the cast admission guard through a native NTZ comparison fixture and passes with its original assertions retained.
The focused reader/NTZ selection also passed 28 tests per profile on Spark 3.4.3, 3.5.9, 4.0.4, and 4.2.0, with three expected version-dependent cancellations per profile. Spark 3.x checks the precise unsupported-schema error; Spark 4.x checks returned rows or consumed overflow. The matrix includes both batch-size directions, filtering below LIMIT, AQE, both bridge paths, disabled native scans, and genuine/pruned/partition NTZ controls. Spark 3.x used JDK 17 and Spark 4.x used JDK 21.
Broader validation passed 145 tests across the complete fuzz, aggregate-fuzz, math-fuzz, map, and Parquet-writer suites. All 33 temporal tests also passed on the unchanged temporal suite, shared helpers, and production sources. Additional SQL checks reported 130 successful cases; one Spark-3.4-only case is intentionally bypassed on Spark 4.1, so 129 SQL bodies were exercised. Native execution assertions remain in place. The final cast run supersedes the earlier cast-fixture failure; earlier compile/oracle attempts and the unrelated passing temporal results were retained separately.
The changed JVM sources were rebuilt, and actual loaded and bundled native libraries were checked against the unchanged native source tree and SHA-256
22b7310faaa011b4be25807bcba0b2c8696733463566c01e9695939342a3c855. No native source changed, and this follow-up does not claim a new native build or benchmark measurement. These are local checks; hosted CI for the new commit is pending. Earlier validation and benchmark results below retain their original revisions and scopes.Spotless, Scalastyle, and CI's syntactic Scalafix 0.14.6 check passed on the final sources. The only edit after runtime validation was a one-line test-comment correction; the full reactor recompiled the test sources and repeated the style checks without rerunning tests.
Review fixes at
10a06cc64(with the redundant-interpolator lint correction in129cc1646): the full Spark 4.1.3 JVM reactor passed 201 tests, with 8 pre-existing ignored tests and no failures. This includes the entireCometNativeCastSuite,SparkErrorConverterSuite, and ten NTZ SQL-file configurations. The changed sources were rebuilt, and the actual loaded and bundled native library matched this PR's unchanged native source; no native rebuild was needed for this follow-up.Six new tests cover skipped and consumed overflows in projections, filters, semi/anti joins, and nested/enclosing casts with ANSI and batch dispatch independently enabled or disabled. They assert Spark row execution for unsafe casts, continued native scans, matching consumed-overflow errors, and native nullable
TRY_CASTand byte/short controls. The SQL fixture checks the specific fallback reason across four timezones and both dispatch settings. The unchanged-head test file reproduces the redundant-interpolator failure under CI's Scalafix 0.14.6, while all patched Spark sources pass that check. The full reactor also passes Spotless and Scalastyle. These runtime checks use Spark 4.1.3; the new commit's cross-version CI is pending.Post-merge validation on
3bd44648e(2026-08-27): merged Apache main525c05b52while retaining both parents. The only manual conflict was the Cargo benchmark list; the resolution keeps this PR'scast_nestedand all upstream benchmark registrations. All 11 other files changed by this PR remain byte-for-byte unchanged from23ae9bb.A fresh default-feature release native build and the full Spark 4.1.3/JDK 17 Maven reactor passed. Native library unit tests passed: 35 common, 672 expression, 4 protocol, and 60 shuffle tests (771 total). The 28 focused date/error tests and 8 RSS/protocol/datatype tests passed, including actual JVM callbacks, exception propagation, registration cleanup, local/RSS/legacy plan serialization, and dictionary modes. Both JVM runs loaded the new native library, and the bundled resource matched its SHA-256. All-target expression Clippy, Rust formatting, JVM style checks, and
git diff --checkagainst main passed.The benchmark tables below remain pinned to the measured
c54a5310implementation and their stated base revisions; timings were not rerun after this merge. Those native libraries, JVM classes, benchmark binaries, and results were preserved separately.Review follow-up validation before the main merge (2026-08-27):
CometNativeCastSuite DateType toandSparkErrorConverterSuitetests green, including timezone serialization and nested alias routing.ARRAY<DATE>toARRAY<TIMESTAMP>shape. The test now uses the supported array-of-struct shape; the affected 28 tests passed afterward. The entire 185-test selection was not rerun after this test correction.git diff --checkpassed. Current follow-up JVM execution was on Spark 4.1.3/JDK 17; the older Spark-profile checks listed below were not repeated.Native nested-widening benchmark: original PR head to revised head
cast_nested.rscompared unmodified PR head8b072abb394dfdd389687d60b71b68f8459a8123with revised implementationc54a5310cc0e991d7a333b2abf4fe94dac0fa786. The original implementation was benchmarked before production edits, and its preserved executable was rerun immediately before the final executable. Both use the same 46-shape harness and repository optimized profile (thin LTO, one codegen unit), on AMD EPYC Milan. Criterion used 30 samples, one second warmup, and two seconds measurement per shape; batches have 8,192 parent rows, list/map widths 4 or 64, and 0/10/25/50/75/99% nominal parent nulls. The sliced cases also include 8,190 and 32 visible parents. Checked DATE and visible-overflow cases exercise the unchanged fallible path.list_date_to_timestamp/width=4/nulls=0%list_date_to_timestamp/width=4/nulls=10%list_date_to_timestamp/width=4/nulls=25%list_date_to_timestamp/width=4/nulls=50%list_date_to_timestamp/width=4/nulls=75%list_date_to_timestamp/width=4/nulls=99%list_date_to_timestamp/width=64/nulls=0%list_date_to_timestamp/width=64/nulls=10%list_date_to_timestamp/width=64/nulls=25%list_date_to_timestamp/width=64/nulls=50%list_date_to_timestamp/width=64/nulls=75%list_date_to_timestamp/width=64/nulls=99%list_date_to_timestamp/visible_overflowlist_int_to_long/width=4/nulls=0%list_int_to_long/width=4/nulls=10%list_int_to_long/width=4/nulls=25%list_int_to_long/width=4/nulls=50%list_int_to_long/width=4/nulls=75%list_int_to_long/width=4/nulls=99%list_int_to_long/width=64/nulls=0%list_int_to_long/width=64/nulls=10%list_int_to_long/width=64/nulls=25%list_int_to_long/width=64/nulls=50%list_int_to_long/width=64/nulls=75%list_int_to_long/width=64/nulls=99%list_int_to_long/one_large_null_parentmap_int_to_long/width=4/nulls=0%map_int_to_long/width=4/nulls=10%map_int_to_long/width=4/nulls=25%map_int_to_long/width=4/nulls=50%map_int_to_long/width=4/nulls=75%map_int_to_long/width=4/nulls=99%map_int_to_long/width=64/nulls=0%map_int_to_long/width=64/nulls=10%map_int_to_long/width=64/nulls=25%map_int_to_long/width=64/nulls=50%map_int_to_long/width=64/nulls=75%map_int_to_long/width=64/nulls=99%sliced_list_int_to_long/width=4/nulls=10%small_slice_int_to_long/width=64/nulls=0%struct_int_to_long/width=1/nulls=0%struct_int_to_long/width=1/nulls=10%struct_int_to_long/width=1/nulls=25%struct_int_to_long/width=1/nulls=50%struct_int_to_long/width=1/nulls=75%struct_int_to_long/width=1/nulls=99%Six controls initially exceeded Criterion's 1% noise threshold with a positive 95% change interval. Repeating those shapes in the opposite binary order did not reproduce any regression; do not interpret the small control fluctuations as optimization gains. The no-regression finding is limited to these measured native shapes, not the intentional named-zone routing change below.
list_date_to_timestamp/width=4/nulls=25%list_date_to_timestamp/width=64/nulls=75%list_date_to_timestamp/width=64/nulls=99%list_int_to_long/width=4/nulls=99%map_int_to_long/width=64/nulls=0%struct_int_to_long/width=1/nulls=0%Spark DATE column benchmark: exact PR base to revised head
CometCastDateToTimestampBenchmarkwas run unchanged on exact PR basebd7dc601aa9a99289530598d4778c40e6ded7772and revised implementationc54a5310cc0e991d7a333b2abf4fe94dac0fa786. Both native libraries were rebuilt with default features and the repository release profile; both JVM reactors used Spark 4.1.3 and JDK 17. Each fresh JVM wrote the same 1,048,576-row DATE column to one Parquet file: ordinary dates from 1960–2029 with 1% nulls. The timed query wasSELECT CAST(d AS TIMESTAMP) FROM date_cast_inputinto Spark's noop sink, with Comet enabled,local[1], AQE disabled, batch size 8,192, and at least 10 warmed iterations per case. Native library loading and actual execution routes were checked.Four fresh JVMs ran in base/head/head/base order. Each table cell preserves the two per-JVM mean times in chronological order within its revision, rounded to milliseconds by Spark Benchmark. The final column compares the averages of those two means; these small local repeats are descriptive, not confidence intervals or an estimate for every workload. The measurement includes scan, projection, and sink overhead; it does not isolate the cast kernel or only the planner change.
UTC/ onUTC/ offAmerica/Los_Angeles/ onAmerica/Los_Angeles/ offIn particular, disabling dispatch still ran the base Los Angeles cast natively; only the revised head used Spark row projection. Both revisions kept UTC native. These numbers quantify the correctness tradeoff for named-zone DATE columns, independently of the native nested-widening improvement.
The earlier wide-date and null-container fix was validated as follows:
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.