mbutrovich commented on code in PR #4917:
URL: https://github.com/apache/datafusion-comet/pull/4917#discussion_r3591113183
##########
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:
`date_parser_test` (`native/spark-expr/src/conversion_funcs/string.rs:2712`)
never feeds a canonical-shape `dddd-dd-dd` string that is an invalid calendar
date, so the branch where the fast path calls `ymd_to_epoch_day` and gets
`None` is untested. The existing invalid-date cases (`2020-010-01`, `202`,
`2020-\r8`) all fall through to the general parser because they are not 10-char
`dddd-dd-dd`. A regression that made the fast path skip calendar validation
would pass the current suite.
Suggested change: add to the invalid list in `date_parser_test` (which
asserts `None` for Legacy/Try and `is_err()` for Ansi is wrong here, see
finding 2, so put these in a Legacy/Try-only null assertion) canonical invalid
dates and one valid boundary:
```rust
// invalid calendar dates in canonical yyyy-mm-dd form (exercise the fast
path)
for date in &["2020-02-30", "2021-02-29", "2020-13-01", "2020-00-15",
"2020-04-31", "2020-01-00"] {
for eval_mode in &[EvalMode::Legacy, EvalMode::Try, EvalMode::Ansi] {
assert_eq!(date_parser(date, *eval_mode).unwrap(), None);
}
}
// valid leap day through the fast path
assert_eq!(date_parser("2020-02-29", EvalMode::Legacy).unwrap(),
Some(18321));
```
##########
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:
The comment at the fast path
(`native/spark-expr/src/conversion_funcs/string.rs:1897` in the diff) says an
invalid calendar date "is a null in every eval mode rather than an ANSI error."
That is not what Spark does. `stringToDate` returns `None` for an invalid
calendar date, and `stringToDateAnsi` (SparkDateTimeUtils.scala:398) turns any
`None` into `invalidInputInCastToDatetimeError`, so Spark ANSI throws on
`2020-02-30`. Comet's existing code already returns `Ok(None)` here (the caller
at string.rs:716-720 treats `Ok(None)` as null in every mode), so this PR
introduces no regression, but the comment states a false fact about Spark and
will mislead the next reader.
Suggested change: describe Comet's actual behavior, not an incorrect Spark
claim:
```rust
// 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 fails is an invalid calendar
date. That
// path already returns null in Comet for every eval mode (the caller maps
Ok(None) to
// null), matching the general parser here. Any other shape, including a
leading sign
// that makes the first byte a non-digit, falls through to the general
parser.
```
##########
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:
The general path now ends with `.map(|days| days as i32)`
(`native/spark-expr/src/conversion_funcs/string.rs:1975` in the diff) replacing
`duration_since_epoch.to_i32().unwrap()`. The year range check at
string.rs:1966 guarantees the value fits in i32, so both are correct today, but
`as i32` would silently wrap if that invariant were ever weakened, whereas
`.unwrap()` failed loudly. Since the invariant is enforced two lines up, prefer
a `debug_assert!` over reintroducing the checked conversion so the fast path
stays branch-free:
```rust
Ok(ymd_to_epoch_day(year as i64, date_segments[1] as i64, date_segments[2]
as i64).map(|days| {
debug_assert!(i32::try_from(days).is_ok(), "epoch day {days} out of i32
range for year {year}");
days as i32
}))
```
The fast path can keep the bare `days as i32` 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]
