Repository navigation
fix: bin date_bin month strides outside chrono's range - #26153
ThilakShekharShriyan wants to merge 2 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.
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