Repository navigation
Conversation
Spark's stringToDate accepts a year of up to 7 digits and only rejects a date whose epoch day overflows Int, so years such as 294248, -290309 and 1000000 parse to a date. Comet's native date_parser already computed the epoch day without chrono, but then returned NULL in every eval mode for any year outside chrono's NaiveDate range (-262143..=262142), silently diverging from Spark for CAST, TRY_CAST and to_date. Drop that extra bound so the parser returns the epoch day Spark returns, keeping the Int-overflow and invalid-calendar-date checks that raise CAST_INVALID_INPUT under ANSI.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the entire four-file diff from bfacc43a50aba13da4fa8a828db52309541191b4 to 8d0f144efe72459b6525903f3da4f3c92dd2353b. The PR is non-draft. Used review-comet-pr and review-comet-expression-pr. The snapshot and live discussion contained no reviews or comments.
Summary
- Prior state and problem:
CAST(string AS DATE),TRY_CASTand unformattedto_datereturned null for valid years outside chrono’s range, although Spark accepts dates throughout the signed 32-bit epoch-day range. - Design approach: Remove the chrono year guard and retain Gregorian calendar validation plus checked conversion to
i32. - Correctness: The parser’s new boundary cases pass. However, newly accepted dates reach an unchanged timestamp consumer that panics. This materially worsens the acknowledged concern in #5456 and is the P2 finding below.
- Compatibility analysis: Checked Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 sources. Their date parsers agree on seven-digit years, calendar validation and epoch-day overflow. Spark’s unformatted
to_datedelegates to a date cast. The latest release branch,branch-1.1, has the same parser and range guard as the base. - Key design decisions: Preserve the existing Legacy/Try null versus ANSI error behavior for malformed input. Exercise extreme dates through
unix_dateto avoid Spark’s separatejava.sql.Datecollection limitation. - Implementation sketch: Update
resolve_epoch_day, replace out-of-range null assertions with exact epoch-day assertions, add SQL tests under both ANSI settings, widen Scala fuzz coverage and update the compatibility template. - Performance: The production change removes a range check without introducing allocations or additional passes. No performance regression was identified from the implementation. No benchmark was run or speedup established.
- Design: The parser change is small and straightforward. Expanding its output domain requires safe downstream consumption, which the current timestamp path does not provide.
- Abstraction & complexity: Reusing
ymd_to_epoch_dayavoids another calendar implementation. Separate valid-input and malformed-input assertion helpers remain simple and useful. - Behavioral changes worth calling out: Standalone wide-year casts now return Spark-compatible dates. Composed date-to-timestamp casts can change from a null result to a query failure, including in Try mode.
- Suggested improvements: Make the affected timestamp consumer handle wide dates correctly, or route affected compositions through Spark, before removing this protection. Add a column-based composition regression covering all evaluation modes.
Validation: date_parser_test passed. A disposable native integration test confirmed the timestamp panic in Legacy, ANSI and Try modes, checked the exact base parser’s null result, and verified that the corresponding null input avoids the panic. Spark 3.5.9 date utilities independently returned the expected epoch day and UTC microseconds. The disposable test was removed and the worktree is clean. Full JVM/SQL suites and benchmarks were not run locally.
Exact-head CI: At the final inspection, 23 checks succeeded, 3 remained in progress and 14 were skipped. Native build, Rust tests, scans, shuffle and TPC-H passed. Expressions, execution and TPC-DS remained running. Spark SQL compatibility jobs and macOS were skipped. No failed checks were reported. Run: https://github.com/apache/datafusion-comet/actions/runs/38028364407
| // through chrono, so years past chrono's `NaiveDate` range (+/-262142) still produce | ||
| // the epoch day Spark returns, up to the `i32` limit (about year +/-5881580). | ||
| match ymd_to_epoch_day(year, month, day).and_then(|d| i32::try_from(d).ok()) { | ||
| Some(days) => Ok(Some(days)), |
There was a problem hiding this comment.
[P2] Handle wide dates in timestamp consumers before removing this guard. With a string column containing 262143-01-01 and the session timezone set to UTC, CAST(CAST(s AS DATE) AS TIMESTAMP) now passes epoch day 95026237 into cast_date_to_timestamp, whose chrono addition panics at temporal.rs:74. Spark successfully returns 8210266876800000000 microseconds. The base returned null from the inner cast and avoided the panic, so this PR materially expands #5456 to previously non-failing string-input queries. One such row can fail a mixed batch, even with Try evaluation. Please make the consumer support these dates or fall back to Spark for affected compositions, and cover the composition in regression tests.
Evidence: A temporary integration test invoked the exact-head spark_cast on ['2020-01-01', NULL, '262143-01-01'], then cast the resulting Date32 array to Timestamp(Microsecond, UTC). Legacy, ANSI and Try each panicked with 'NaiveDate + TimeDelta overflowed' at conversion_funcs/temporal.rs:74. The exact base parser returned None for the wide input in all three modes, and the corresponding timestamp conversion preserved null without panicking. Spark 3.5.9 stringToDate returned Some(95026237), and daysToMicros with UTC returned 8210266876800000000. SQL trigger with Comet enabled: SET spark.sql.session.timeZone=UTC; CREATE TABLE wide_date_review(s STRING) USING parquet; INSERT INTO wide_date_review VALUES ('2020-01-01'), ('262143-01-01'); SELECT unix_micros(CAST(CAST(s AS DATE) AS TIMESTAMP)) FROM wide_date_review. Validation executed the native composition and Spark utility calls, not the full SQL query.
There was a problem hiding this comment.
Agreed that this composition needs to be safe before this lands. The panic itself is #5456: such dates already reach cast_date_to_timestamp from Parquet date columns and from native make_date, and this PR adds string input as another source. Since #5457 fixes that consumer, I'd like to keep this PR scoped to the parser and merge it after #5457. Once #5457 is in, I'll rebase and add a column-based regression for CAST(CAST(s AS DATE) AS TIMESTAMP) and TIMESTAMP_NTZ under LEGACY, ANSI and TRY.
One note for that test: in the SQL trigger above, unix_micros runs through the codegen dispatcher, so the inner casts are evaluated by Spark and never reach the native kernel. The regression will use a consumer that stays native, such as CAST(... AS BIGINT) on the timestamp.
|
WE should just move to edit: I meant https://crates.io/crates/jiff, not |
|
+1 on jiff for timezone handling. apache/arrow-rs#11482 fixes a real class of bugs here: chrono-tz stops applying DST rules after its precomputed table, while jiff follows the TZif footer the way The year range in this PR is a separate limit, though, and jiff doesn't widen it. jiff's |
Which issue does this PR close?
Closes #6842.
Depends on #5457, which makes the downstream date-to-timestamp cast safe for these dates (#5456). This PR should merge after it.
Rationale for this change
Spark's
stringToDateaccepts a year of up to 7 digits (optionally signed) and only rejects a date whenLocalDate.offails or the epoch day overflowsInt, which happens past5881580-07-11or before-5877641-06-23. Comet'sdate_parsercomputed the same epoch day without chrono, but then returnedNULLin every eval mode for any year outside chrono'sNaiveDaterange (-262143 to 262142). As a result,CAST,TRY_CASTandto_datereturnedNULLfor valid dates such as294248-01-01or1000000-01-01, even in ANSI mode.What changes are included in this PR?
date_parser. Its remaining checks match Spark: an invalid calendar date or an epoch day that overflowsi32givesNULLin LEGACY/TRY mode andCAST_INVALID_INPUTin ANSI mode.Known downstream limitations
This makes
CAST(string AS DATE)return dates outside chrono'sNaiveDaterange, which previously came back asNULL. Some native consumers already mishandle such dates when they come from Parquet ormake_date, and this change lets them come from strings too: casting them toTIMESTAMPpanics (#5456), and several date-field functions andCAST(date AS STRING)returnNULLor fail (#6843). For those queries, a wrongNULLfrom the string cast becomes an error from the downstream expression.How are these changes tested?
string.rsnow assert exact epoch days for years past chrono's range, including leap days, a+sign and aTsuffix, and bothi32boundaries. They also check that one day past either boundary, invalid calendar dates past chrono's range, and 8-digit years are rejected.expressions/cast/cast_string_to_date_wide_year.sql, run with ANSI enabled and disabled. It checksunix_dateofcast,try_castandto_date, plus the raw date values, and fails onmain. The raw-date query leaves out the twoInt-boundary rows because Spark cannot convert them tojava.sql.Datewhen collecting.CometNativeCastSuite"cast StringType to DateType" adds explicit wide-year inputs. Its fuzz filter now drops only 7-digit years near theIntlimit, for the samejava.sql.Datereason.