Skip to content

fix: prevent next_day panic on far-future start dates - #24194

Merged
viirya merged 1 commit into
apache:mainfrom
viirya:next-day-far-future-23891
Aug 9, 2026
Merged

viirya merged 1 commit into
apache:mainfrom
viirya:next-day-far-future-23891

Conversation

@viirya

@viirya viirya commented Aug 8, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

datafusion-spark's next_day computed its result by adding the day offset to a chrono::NaiveDate:

Some(Date32Type::from_naive_date(
    date + Duration::days((7 - date.weekday().days_since(day_of_week)) as i64),
))

When the next occurrence of the requested weekday lands past chrono::NaiveDate::MAX (epoch day 95026236) — which happens for start dates in the last week of the representable Date32 range — the NaiveDate + TimeDelta add panics with NaiveDate + TimeDelta overflowed. (The linked issue reports NULL; the scalar path in fact panics.)

Spark's DateTimeUtils.getNextDateForDayOfWeek is pure integer arithmetic on the epoch day and always produces a value:

def getNextDateForDayOfWeek(startDay: Int, dayOfWeek: Int): Int = {
  startDay + 1 + ((dayOfWeek - 1 - startDay) % 7 + 7) % 7
}

What changes are included in this PR?

  • Compute the result on the epoch day directly (days + delta, via checked_add) instead of building a NaiveDate for the result, so a next occurrence past NaiveDate::MAX returns a value instead of panicking. The weekday/offset logic is unchanged, so results for all in-range dates are identical.

Scope note: a start day beyond NaiveDate::MAX (> 95026236) still returns NULL, because the weekday is derived via NaiveDate. Extending full Int-range parity (computing the weekday arithmetically too) is a larger change left as a follow-up; this PR fixes the panic, which is the concrete reported harm.

Are these changes tested?

Yes.

  • Unit test next_day_handles_far_future_start_dates covering the two cases from the issue (95026236/Mon, 95026230/Tue), each of which panicked before this change.
  • SLT coverage in spark/datetime/next_day.slt asserting the same two cases (casting the Date32 result to Int32, since these dates are past the printable range).

Are there any user-facing changes?

next_day no longer panics for far-future start dates; it returns the correct epoch day. No API changes.

Spark's `next_day` computed its result by adding the day offset to a
`chrono::NaiveDate`. When the next occurrence lands past
`NaiveDate::MAX` (epoch day 95026236) — which happens for start dates in
the last week of the representable `Date32` range — the `NaiveDate +
TimeDelta` add panics with `NaiveDate + TimeDelta overflowed`.

Spark's `DateTimeUtils.getNextDateForDayOfWeek` is pure integer
arithmetic on the epoch day and always produces a value. This change
computes the result on the epoch day directly (`days + delta`, with
`checked_add`) so those start dates return a value instead of panicking.

Closes apache#23891.

Co-authored-by: Claude Code
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) spark Changes to datafusion-spark crate labels Aug 8, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.05%. Comparing base (66c3840) to head (f310c59).
⚠️ Report is 69 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24194      +/-   ##
==========================================
+ Coverage   80.86%   81.05%   +0.18%     
==========================================
  Files        1101     1106       +5     
  Lines      375446   382289    +6843     
  Branches   375446   382289    +6843     
==========================================
+ Hits       303592   309848    +6256     
- Misses      53761    54123     +362     
- Partials    18093    18318     +225     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@viirya
viirya added this pull request to the merge queue Aug 9, 2026
@viirya

viirya commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

Thanks @Jefffrey for review

Merged via the queue into apache:main with commit 354f2d7 Aug 9, 2026
37 checks passed
@viirya
viirya deleted the next-day-far-future-23891 branch August 9, 2026 07:20
kosiew pushed a commit to kosiew/datafusion that referenced this pull request Aug 12, 2026
## Which issue does this PR close?

- Closes apache#23891.

## Rationale for this change

`datafusion-spark`'s `next_day` computed its result by adding the day
offset to a `chrono::NaiveDate`:

```rust
Some(Date32Type::from_naive_date(
    date + Duration::days((7 - date.weekday().days_since(day_of_week)) as i64),
))
```

When the next occurrence of the requested weekday lands past
`chrono::NaiveDate::MAX` (epoch day `95026236`) — which happens for
start dates in the last week of the representable `Date32` range — the
`NaiveDate + TimeDelta` add **panics** with `NaiveDate + TimeDelta
overflowed`. (The linked issue reports NULL; the scalar path in fact
panics.)

Spark's `DateTimeUtils.getNextDateForDayOfWeek` is pure integer
arithmetic on the epoch day and always produces a value:

```scala
def getNextDateForDayOfWeek(startDay: Int, dayOfWeek: Int): Int = {
  startDay + 1 + ((dayOfWeek - 1 - startDay) % 7 + 7) % 7
}
```

## What changes are included in this PR?

- Compute the result on the epoch day directly (`days + delta`, via
`checked_add`) instead of building a `NaiveDate` for the result, so a
next occurrence past `NaiveDate::MAX` returns a value instead of
panicking. The weekday/offset logic is unchanged, so results for all
in-range dates are identical.

Scope note: a *start* day beyond `NaiveDate::MAX` (`> 95026236`) still
returns NULL, because the weekday is derived via `NaiveDate`. Extending
full `Int`-range parity (computing the weekday arithmetically too) is a
larger change left as a follow-up; this PR fixes the panic, which is the
concrete reported harm.

## Are these changes tested?

Yes.

- Unit test `next_day_handles_far_future_start_dates` covering the two
cases from the issue (`95026236`/`Mon`, `95026230`/`Tue`), each of which
panicked before this change.
- SLT coverage in `spark/datetime/next_day.slt` asserting the same two
cases (casting the `Date32` result to `Int32`, since these dates are
past the printable range).

## Are there any user-facing changes?

`next_day` no longer panics for far-future start dates; it returns the
correct epoch day. No API changes.
andygrove added a commit to andygrove/datafusion that referenced this pull request Sep 3, 2026
Spark's NextDay has taken failOnError = SQLConf.get.ansiEnabled since at least
3.5, raising on an unparseable day-of-week name and returning NULL otherwise.
datafusion-spark ignored the flag and always returned NULL, with TODO comments
acknowledging the gap. It now honors datafusion.execution.enable_ansi_mode,
preserving Spark's per-row null intolerance: a NULL start date short circuits
to NULL before the name is validated.

One version-independent bug is fixed alongside it:

- SELECT next_day(<date literal>, <string column>) hit an unreachable
  catch-all and raised a hard error. The match over ColumnarValue shapes is
  now exhaustive.

The day-of-week parsing and the date arithmetic are split into
parse_day_of_week and next_date_for_day_of_week so the ANSI branch has a single
place to decide between raising and appending NULL.

This branch originally also fixed a panic for epoch days in 95026230..=95026236
by switching to checked_add_signed, which avoided the panic but returned NULL.
That panic has since been fixed on main by apache#24194, which computes the result on
the epoch day and returns the value Spark's Int arithmetic produces. This rebase
keeps main's behavior and drops the weaker fix; the tests and slt cases that
asserted NULL past chrono::NaiveDate::MAX now assert the epoch day instead.

The only version-specific change is the error message text, which now matches
Spark 4.2.0's ILLEGAL_DAY_OF_WEEK template rather than 3.5.8's older
IllegalArgumentException string. The remaining cross-version divergence, the
ANSI default flip and error class change, is not addressed here and is tracked
by https://github.com/apache/datafusion/issues/23890
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spark Changes to datafusion-spark crate sqllogictest SQL Logic Tests (.slt) v55.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] next_day returns NULL for far-future start dates where Spark returns a value

4 participants