Support cross-timezone timestamp comparison via coercsion
#11711
Conversation
github-actions
bot
added
logical-expr
jeffreyssmith2nd
force-pushed
the
smith/timezone-comparisons
branch
from
2c50956
to
80c12c2
Compare
There was a problem hiding this comment.
Thank you @jeffreyssmith2nd -- this looks pretty good to me. I had one question about the intent of coercing with UTC that I think should probably be addressed before merge, but otherwise this looks good to me.
Thank you
alamb
changed the title
alamb
changed the title
timestamp comparison via coercsion
There was a problem hiding this comment.
Thanks @jeffreyssmith2nd -- this looks great to me 🙏
I'll plan to merge it tomorrow unless anyone else would like time to comment or review
|
Thanks again @jeffreyssmith2nd |
| /// | ||
| /// An example of this is binary comparisons (<, >, ==, etc). Arrow stores timestamps | ||
| /// as relative to UTC epoch, and then adds the timezone as an offset. As a result, we can always | ||
| /// do a binary comparison between the two times. |
There was a problem hiding this comment.
This is only true as long as the timezone is present. If tz is None, then the timestamp is not necessarily relative to UTC epoch (but an "arbitrary" epoch).
FWIW, I think the changes you've done here make sense (*) - I'm just not sure if the original behavior of coercing a (None, Some(tz)) or a (Some(tz), None) are correct. Or well, I'm pretty sure it's not guaranteed to be correct, but maybe it's convenient enough to exist 🤷
(*): though is there need for the "strict" check to exist? I'd assume coercing two timestamps with timezones would always be fine, given they are always UTC-epoch based.
There was a problem hiding this comment.
Or well, I'm pretty sure it's not guaranteed to be correct, but maybe it's convenient enough to exist 🤷
I think it at least has guaranteed semantics 🤔
Which issue does this PR close?
Closes #11653
This PR closes the above issue, but also slightly relaxes the rules further. It also enables the
UNIONfunction to handle different timezones, where it coerces to whatever timezone is on the left side. It does not change the rules around arithmetic on times with timezones.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?