temporal conversion functions should work on negative input properly#2326
temporal conversion functions should work on negative input properly#2326viirya merged 4 commits intoapache:masterfrom
Conversation
viirya
force-pushed
the
negative_timestamp
branch
from
cb28745 to
71d2bc8
Compare
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
The input to time32ms_to_time etc seems to be positive, so I don't touch them.
tustvold
left a comment
tustvold
left a comment
There was a problem hiding this comment.
Can we revert the change to the integration test to remove negative inputs now?
| }; | ||
| use chrono::NaiveDateTime; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Is it worth a test of an exact number of seconds, to check the branch in split_seconds?
There was a problem hiding this comment.
Yea, I will add a few more tests.
yea |
|
Benchmark runs are scheduled for baseline = 50d1e5f and contender = 3ed0e28. 3ed0e28 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
| /// | ||
| #[inline] | ||
| pub(crate) fn split_second(v: i64, base: i64) -> (i64, u32) { | ||
| if v < 0 { |
There was a problem hiding this comment.
Might be possible to simplify this using builtin div_euclid / rem_euclid functions
4 participants
Which issue does this PR close?
Closes #2325.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?