Repository navigation
fix: bin date_bin month strides outside chrono's range - #26153
ThilakShekharShriyan wants to merge 4 commits into
Conversation
Month strides went through DateTime<Utc>, so a timestamp past chrono's range returned NULL even when the month start still fit the output type.
github-actions
Bot
added
sqllogictest
mhilton
left a comment
mhilton
left a comment
There was a problem hiding this comment.
I'd be fascinated to know about any applications that actually need to process dates outside of chrono's range. I would normally ask for a benchmark just to show that this change doesn't have adverse performance characteristics, but as this is limited to the slower "wide" path I don't think it's necessary.
| // Compared in field order, which is chronological. | ||
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| struct CivilTimestamp { | ||
| year: i64, |
There was a problem hiding this comment.
Since this has a relatively limited scope, I think it's fine to leave these types as signed, but a comment saying what are legal and illegal combinations of signed and unsigned components would be helpful.
| ScalarValue::TimestampNanosecond(Some(i64::MIN), None), | ||
| ), | ||
| // Month bins are limited by the range of `DateTime<Utc>`. | ||
| // The month bin of Timestamp(Second)::MIN does not fit (#25855). |
There was a problem hiding this comment.
I personally think that we should define date_bin to use i64::MIN for any value that would be less then i64::MIN to ensure that it always preserves sort order. However that is not really part of this PR.
| /// derivation of the constants and the March-based year used below: | ||
| /// <https://howardhinnant.github.io/date_algorithms.html#civil_from_days> | ||
| fn civil_from_days(days: i64) -> (i64, i64, i64) { | ||
| pub(super) fn civil_from_days(days: i64) -> (i64, i64, i64) { |
There was a problem hiding this comment.
Since this is now being used outside of date_trunc maybe it should be moved to some shared module.
The month-stride fix landed a civil-calendar binning path alongside the existing chrono one, leaving two implementations of the same semantics (month diff, stride distance, shift from origin, month-end clamp, step back on overshoot) and a test whose job was to keep them in sync. Collapse them onto one core. `bin_months_civil` does the binning in i64 and both widths are thin adapters over it, so the i128 path exists only to widen at the nanosecond boundary. `bin_months`, `shift_months` and `to_utc_date_time` drop out. Move the calendar arithmetic to its own `datetime::civil` module. `civil_from_days` and `days_from_civil` were private to date_trunc and had been opened up to `pub(super)` so date_bin could reach them; they now live next to `CivilTimestamp`, which owns the nanosecond conversions, `month_index` and `shift_months`. date_trunc and date_bin are both plain consumers again. Two smaller changes fall out of that move. `days_in_month` computes a month length from a table and a leap test instead of two `days_from_civil` calls, which is where most of the month path's cost was. And the helpers that cannot fail for any reachable input no longer return `Option`, leaving one real failure - whether the bin fits the output type - at the boundary where the caller already checks it. `CivilTimestamp` derives `Ord`, and binning depends on that order being chronological. Its invariants (valid day of month, normalised time of day) are documented on the type, and every constructor lives in the module that maintains them. Add a month-stride benchmark; the existing one only covered day-time strides, so the month path had no coverage. No public API change: everything touched here is private to date_bin.rs or crate-internal. Co-Authored-By: Claude Opus 5 <[email protected]>
|
I had a look at this PR and I found that having two implementations for month-bin semantics wasn't ideal and that perhaps a bit of a refactor would be worth it. Refactor was done by claude and reviewed by myself PR @ ThilakShekharShriyan#1 |
|
@adriangb you seem to review and/or do work related to date/time stuff so perhaps you can review this PR or at least fire off the workflow? Thanks! |
|
Marked as draft after 14 days without activity: merge conflicts. Please address these items and mark Ready for review when ready. |
|
@adriangb ^^ |
Which issue does this PR close?
Closes #25855.
Rationale for this change
date_binwith a month stride returnedNULLfor timestamps that sit outsideDateTime<Utc>, even when the month start still fits in the output type. Second, millisecond, and microsecond timestamps can be wider than chrono's range. A month bin should beNULLonly when that bin itself does not fit.What changes are included in this PR?
The narrow month path is unchanged and still uses chrono for timestamps that fit in
i64nanoseconds.The wide path, used when the nanosecond conversion overflows, now bins on the civil calendar with the same day-count conversion
date_truncalready uses. It keeps the existing stride rule, including negative strides, and clamps a missing day to the end of the month the waychecked_add_monthsdoes. When the candidate is after the source, it steps back one stride from the origin rather than from the already-clamped date.A result that does not fit the output unit is still
NULL. That coversTimestamp(Second)::MINand a nanosecond source with a stride too large fori64nanoseconds.What is the testing strategy for this PR?
date_bin_errors.sltcovers the issue:10_000_000_000_000seconds bins to month start9999998294400, and the negative input bins to-10000001059200.-4306016287785600000) and used to beNULLonly because chrono overflowed. The nanosecond form of that stride still expectsNULL.Are there any user-facing changes?
Yes. Month-stride
date_binnow returns the month start for timestamps outside chrono's range when that instant fits the output type. Values that already fit ini64nanoseconds are unchanged. No public API change.Made with Cursor