viirya commented on code in PR #6844:
URL: https://github.com/apache/datafusion-comet/pull/6844#discussion_r4236715970
##########
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:
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.
--
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]
