andygrove commented on code in PR #4917:
URL: https://github.com/apache/datafusion-comet/pull/4917#discussion_r3603717684
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1161,6 +1160,25 @@ fn days_from_civil(y: i64, m: i64, d: i64) -> i64 {
era * 146097 + doe - 719468
}
+fn is_leap_year(year: i64) -> bool {
+ year % 4 == 0 && (year % 100 != 0 || year % 400 == 0)
+}
+
+/// Days since 1970-01-01 for a proleptic Gregorian year/month/day, or `None`
when the
+/// combination is not a real calendar date. Unlike `NaiveDate::from_ymd_opt`,
this accepts
+/// any year that fits in `i64`.
+fn ymd_to_epoch_day(year: i64, month: i64, day: i64) -> Option<i64> {
Review Comment:
Added canonical-form invalid dates (`2020-02-30`, `2021-02-29`,
`2020-13-01`, `2020-00-15`, `2020-04-31`, `2020-01-00`) asserting `None` for
all three eval modes, plus a valid leap-day case (`2020-02-29` → `18321`) that
exercises the successful fast path.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1908,6 +1892,29 @@ fn date_parser(date_str: &str, eval_mode: EvalMode) ->
SparkResult<Option<i32>>
return return_result(date_str, eval_mode);
}
+ // Fast path for the canonical `yyyy-mm-dd` form. A 4-digit year is always
inside the
+ // range checked below, so the only way this can fail is an invalid
calendar date, which
+ // is a null in every eval mode rather than an ANSI error. Any other shape
(including a
Review Comment:
Rewrote the fast-path comment to describe the actual Comet behavior
(Ok(None) treated as null in every eval mode by the caller) instead of the
incorrect claim about Spark ANSI.
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -1960,15 +1967,12 @@ fn date_parser(date_str: &str, eval_mode: EvalMode) ->
SparkResult<Option<i32>>
return Ok(None);
}
- match NaiveDate::from_ymd_opt(year, date_segments[1] as u32,
date_segments[2] as u32) {
- Some(date) => {
- let duration_since_epoch = date
- .signed_duration_since(DateTime::UNIX_EPOCH.naive_utc().date())
- .num_days();
- Ok(Some(duration_since_epoch.to_i32().unwrap()))
- }
- None => Ok(None),
- }
+ Ok(ymd_to_epoch_day(
+ year as i64,
+ date_segments[1] as i64,
+ date_segments[2] as i64,
+ )
+ .map(|days| days as i32))
Review Comment:
Added a `debug_assert!(i32::try_from(days).is_ok(), ...)` in the
general-path `.map` so a future weakening of the year range check fails loudly
instead of silently wrapping. Fast path kept branch-free since a 4-digit year
can never overflow.
--
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]
