feat: [EXPERIMENT] Spike: convert next_day from chrono to jiff#4755
Draft
andygrove wants to merge 1 commit into
Draft
feat: [EXPERIMENT] Spike: convert next_day from chrono to jiff#4755andygrove wants to merge 1 commit into
andygrove wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Relates to #4754.
Rationale for this change
chrono/chrono-tzare expected to be deprecated, and #4754 proposes evaluating a move tojiff. Rather than estimate in the abstract, this spike converts one self-contained, timezone-free expression (next_day) end to end so we can see the concrete API mapping, the dependency implications, and any behavioral edges before committing to a broader migration.What changes are included in this PR?
next_day.rsto compute entirely withjiff::civil(Date,Weekday), removing allchronousage from that file.jifftodatafusion-comet-spark-exprwithdefault-features = false, features = ["std"](a date-only expression does not need the tzdb bundlescargo addenables by default).API mapping observed
Weekday::Mon..Weekday::Monday.. (full names)Date32Type::to_naive_date_opt(i32)Date::constant(1970, 1, 1).checked_add(days.days())weekday().days_since(other)weekday().since(other)(identical semantics)date + Duration::days(n)thenfrom_naive_datedays + advance(epoch-day arithmetic)Findings
i32epoch days (Date32Array::value). The expression builds jiff types directly from the integer and bypasses Arrow's chrono-returning helpers (to_naive_date_opt,date32_to_datetime). Comet's own code can move to jiff today.chronostays in the dependency tree regardless, becausearrowdepends on it (and pullschrono-tzvia its feature). jiff adds a dependency; it cannot remove chrono until arrow-rs migrates. The near-term benefit is API ergonomics in Comet, not a smaller tree.from_unix_epoch_day/to_unix_epoch_dayare crate-private). The public idiom is anchor date plus a daySpan. Expressions that genuinely needDateto epoch-day will useepoch.until(date).get_days(), which is more verbose than chrono.timezone.rs(currently chrono-tz based). That path is deliberately out of scope here.Suggested follow-up order if we proceed: tz-free date expressions first (next_day, day/month name, make_date) behind a shared epoch-day helper, then timestamp/timezone expressions, with full chrono removal gated on arrow-rs.
How are these changes tested?
The existing
next_dayRust unit tests were translated to jiff and pass, andcargo clippyis clean. End-to-end behavior continues to be covered by the existing JVMnext_daytests, since the public function semantics are unchanged.