Add support for time zone offsets to TimeZone #10577
Conversation
Summary: Adding support for time zone offsets to TimeZone. Now, we will be able to clean up the callsites to use this single APIs, which will contain time zone name, ID, and conversion capabilities in a more consistent manner. Part of facebookincubator#10101 Reviewed By: mbasmanova Differential Revision: D60213004
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D60213004 |
✅ Deploy Preview for meta-velox canceled.
|
|
|
||
| namespace facebook::velox { | ||
|
|
||
| namespace tz { |
There was a problem hiding this comment.
just to break the header dependency
| @@ -25,15 +25,20 @@ | |||
|
|
|||
| namespace facebook::velox::tz { | |||
|
|
|||
| using TTimeZoneDatabase = std::vector<std::unique_ptr<TimeZone>>; | |||
There was a problem hiding this comment.
@mbasmanova I ended up merging both PRs into one; this is the final state as I mentioned in the previous review.
| /// | ||
| /// You can overwrite the behavior in ambiguous conversions by setting the | ||
| /// TChoose flag, but it will still throws in case of nonexistent conversions. | ||
| enum class TChoose { |
There was a problem hiding this comment.
need to define our own here so we don't introduce a header to header dependency
|
This pull request has been merged in db11b69. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
| // Every single other time zone entry (outside of offsets) needs to be | ||
| // available in external/date or this will throw. | ||
| else { | ||
| timeZonePtr = std::make_unique<TimeZone>( | ||
| entry.second, entry.first, date::locate_zone(entry.second)); | ||
| } |
There was a problem hiding this comment.
It seems that date::locate_zone requires the time-zone to be recognizable by OS?
I am not sure but probably this change causes Gluten's CentOS 8 tests to throw
Europe/Kyiv not found in timezone database
Can we loose the restriction here?
There was a problem hiding this comment.
@zhztheplayer I guess you are using an outdated version of the tzdata database in your system. Could you update it? Which version are you using?
You can check with something like
$ rpm -qa | grep tzdata
tzdata-2024a-2.el9.noarch
There was a problem hiding this comment.
It's by default tzdata-2021a-1.el8.noarch in official centos:8 docker image. So I need to update it probably.
[root@47881716f3de /]# rpm -qa | grep tzdata
tzdata-2021a-1.el8.noarch
[root@47881716f3de /]# ls /usr/share/zoneinfo/Europe/
Amsterdam Belfast Brussels Chisinau Guernsey Jersey Lisbon Madrid Monaco Paris Rome Saratov Stockholm Ulyanovsk Vienna Zagreb
Andorra Belgrade Bucharest Copenhagen Helsinki Kaliningrad Ljubljana Malta Moscow Podgorica Samara Simferopol Tallinn Uzhgorod Vilnius Zaporozhye
Astrakhan Berlin Budapest Dublin Isle_of_Man Kiev London Mariehamn Nicosia Prague San_Marino Skopje Tirane Vaduz Volgograd Zurich
Athens Bratislava Busingen Gibraltar Istanbul Kirov Luxembourg Minsk Oslo Riga Sarajevo Sofia Tiraspol Vatican WarsawThere was a problem hiding this comment.
yeah, I suppose a simple update to the tzdata package should solve it. We could also do what you suggested above; allow timezone without a tzdata database, and throw at runtime if they try to be used. If you think that's useful, could you open an Issue?
There was a problem hiding this comment.
Sure. And we have been using a simple fix for the issue in Gluten. I can open a PR using that code.
…zone database (#10654) Summary: Related to discussion #10577 (comment) The patch fixes fatal error `<zone id> not found in timezone database` when at least one of the timezone IDs in [file](https://github.com/facebookincubator/velox/blob/main/velox/type/tz/TimeZoneDatabase.cpp) don't exist in OS's supported timezone list. Pull Request resolved: #10654 Reviewed By: kgpai Differential Revision: D62785133 Pulled By: pedroerp fbshipit-source-id: c8454454750040f8fdcbe053084f505d679f3142
Summary:
Adding a new TimeZone class and adding support for time zone offsets to it.
Now, we will be able to clean up the callsites to use this single APIs, which will
contain time zone name, ID, and conversion capabilities in a more consistent
manner.
Part of #10101 and #8037
Reviewed By: mbasmanova
Differential Revision: D60213004