gh-99772: Do not ignore utc offset microseconds - #99774
Conversation
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
|
All commit authors signed the Contributor License Agreement. |
a542283 to
d75ce27
Compare
d75ce27 to
9079ad3
Compare
Jason-Y-Z
left a comment
Jason-Y-Z
left a comment
There was a problem hiding this comment.
Thanks for your contributions. Please see comments inline.
| for op in lt, le, gt, ge, eq, ne: | ||
| got = op(x, y) | ||
| expected = op( | ||
| -x.utcoffset().microseconds, |
There was a problem hiding this comment.
why do we need the minus sign here?
There was a problem hiding this comment.
Because the utc offset contributes to the time in a negative way, in other words, the greater the utc offset, the earlier the actual time.
| /* The hard case: both aware with different UTC offsets */ | ||
| else if (offset1 != Py_None && offset2 != Py_None) { | ||
| int offsecs1, offsecs2; | ||
| int64_t offusecs1, offusecs2, diff64; |
There was a problem hiding this comment.
This might be a minor discussion point, but we could avoid using int64_t and the following big-integer calculations by doing a multi-step comparison, i.e. comparing the hours, then minutes, then seconds etc.
The motivation for this is a bit of gut feeling, but I think that way will be slightly more efficient in terms of time and space.
There was a problem hiding this comment.
I actually implemented it like you suggest earlier, because I share your gut feeling. Using the existing code for checks up to seconds and adjusting only the microseconds check. Indeed with the advantage that it all fits in 32 bits.
What I encountered using this approach was the following: 13:59:59.9+02 is actually later than 14:00:00+02:00:00.9. The combined microseconds values, in the time values and the offsets, can contribute up to a maximum of 1.999998 seconds to the diff in either way. That means that the simple intermediary check that is used in other multi step comparisons if (diff == 0) can not be used. It should become if (diff > -2 && diff < 2). I chose to go for the int64_t approach because I thought it to be less confusing, than the 2 seconds intermediary check, but I am perfectly willing to go with your suggestion, if that is the consensus.
There was a problem hiding this comment.
@blenq Does one of the tests already handle the use case of 13:59:59.9+02 vs. 14:00:00+02:00:00.9? If not we should add that as a test case.
|
I'm going to move my conversation here for visibility.
Given that the most exotic time zones seem to have at most millisecond precision and the shared concerns about using int64s, would it make sense to limit the scope from microsecond support to millisecond support? If my back-of-the-napkin math is correct, 32 bit integers should be sufficient for that. FWIW- I benchmarked datetime comparisons with your code and cpython-main, and there was no difference in timing on my 64 bit machine. |
|
I actually created an alternative implementation that does not use 64 integers. If this is preferable I'll add the commit to the pull request. |
|
This PR is stale because it has been open for 30 days with no activity. |
|
I'm sorry but I'm closing in favour of #155024 as it includes additional fixes. |
6 participants
Fixes the compare operations for datetime.time values with microseconds in the utc offset.