sunchao commented on code in PR #6844:
URL: https://github.com/apache/datafusion-comet/pull/6844#discussion_r4236662030
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -2041,22 +2041,13 @@ fn date_parser(date_str: &str, eval_mode: EvalMode) ->
SparkResult<Option<i32>>
) -> SparkResult<Option<i32>> {
// Spark builds a `LocalDate` and narrows its epoch day to an `Int`,
so an invalid
// calendar date or an epoch day that overflows `i32` both yield
`None` there, which
- // `stringToDateAnsi` turns into CAST_INVALID_INPUT.
- let Some(days) = ymd_to_epoch_day(year, month, day).and_then(|d|
i32::try_from(d).ok())
- else {
- return return_result(date_str, eval_mode);
- };
- // Spark accepts years beyond what chrono can represent, and
downstream date kernels
- // cannot handle those values, so Comet keeps returning null for them
in every eval mode
- // rather than raising. This is a Comet limitation, not a malformed
input.
- //
- // The bound is chrono's representable year range: `NaiveDate::MIN` is
`-262143-01-01`
- // and `NaiveDate::MAX` is `262142-12-31`
- //
(https://docs.rs/chrono/latest/chrono/naive/struct.NaiveDate.html#associatedconstant.MIN).
- if !(-262143..=262142).contains(&year) {
- return Ok(None);
+ // `stringToDateAnsi` turns into CAST_INVALID_INPUT.
`ymd_to_epoch_day` does not go
+ // 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)),
Review 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.
--
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]
