Skip to content

perf: optimize date_parser / cast string to date (up to 2x faster)#4917

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:auto-opt/date_parser-datafusion-comet-20260714-043541
Open

perf: optimize date_parser / cast string to date (up to 2x faster)#4917
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:auto-opt/date_parser-datafusion-comet-20260714-043541

Conversation

@andygrove

@andygrove andygrove commented Jul 14, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

N/A

Rationale for this change

Optimize existing expression.

What changes are included in this PR?

Replaced the per-row chrono NaiveDate construction and duration-to-epoch-day conversion with direct integer calendar validation plus the existing days_from_civil helper, and added a branch-free fast path for the canonical yyyy-mm-dd form that skips trimming, sign and digit-count checks.

How are these changes tested?

Existing tests.

Benchmark (criterion):

  • canonical: 54.56% faster (base 124085ns -> cand 56384ns)
  • mixed: 23.546% faster (base 110311ns -> cand 84337ns)

Full criterion output:

cast_string_to_date/canonical
                        time:   [56.312 µs 56.372 µs 56.441 µs]
                        change: [−54.647% −54.560% −54.464%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
  1 (1.00%) low severe
  2 (2.00%) low mild
  3 (3.00%) high mild
  8 (8.00%) high severe
cast_string_to_date/mixed
                        time:   [84.211 µs 84.254 µs 84.317 µs]
                        change: [−23.818% −23.546% −23.304%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) low severe
  1 (1.00%) low mild
  1 (1.00%) high mild
  3 (3.00%) high severe

@andygrove andygrove changed the title perf: optimize date_parser in datafusion-comet-spark-expr perf: optimize date_parser / cast string to date (up to 2x faster) Jul 14, 2026
@andygrove andygrove marked this pull request as ready for review July 14, 2026 14:45

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, thanks @andygrove!

/// 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> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

// 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));


// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

// 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.

date_segments[1] as i64,
date_segments[2] as i64,
)
.map(|days| days as i32))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants