Split timestamp_s_to_datetime to date and time to avoid unnecessary computation
#6755
Conversation
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
| /// except for 1700, 1800 and 1900 -3 + | ||
| /// -------- | ||
| /// 719163 | ||
| pub const UNIX_EPOCH_DAY: i64 = 719_163; |
There was a problem hiding this comment.
this looks to be identical to the EPOCH_DAYS_FROM_CE const above, no?
There was a problem hiding this comment.
Seems that EPOCH_DAYS_FROM_CE is actually not used, remove it. To keep the variable name consistent to chrono
| /// Similar to timestamp_s_to_datetime but only compute `date` | ||
| #[inline] | ||
| pub fn timestamp_s_to_date(secs: i64) -> Option<NaiveDateTime> { | ||
| let days = secs.div_euclid(86_400) + UNIX_EPOCH_DAY; |
There was a problem hiding this comment.
I guess we're ignoring leap seconds here?
| /// Similar to timestamp_s_to_datetime but only compute `time` | ||
| #[inline] | ||
| pub fn timestamp_s_to_time(secs: i64) -> Option<NaiveDateTime> { | ||
| let secs = secs.rem_euclid(86_400); |
There was a problem hiding this comment.
and ignoring leap seconds here too?
There was a problem hiding this comment.
What do you mean leap second? The code is copied from chrono I prefer to keep than consistent if possible. If the computation is not expected we should fix it in chrono
There was a problem hiding this comment.
I suppose that depends on whether the place where this came from is supposed to respect leap seconds, etc or not. It's not technically correct when using UTC, see https://en.wikipedia.org/wiki/Leap_second for a list of leap seconds since unix epoch. It's also not going to be far off either.
There was a problem hiding this comment.
Let's support it when chrono did so.
Signed-off-by: Jay Zhan <[email protected]>
Which issue does this PR close?
Closes #6746 .
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
I think we don't need to change chrono crate