Support offset-based timezone

[PHILO-HE]

The external date lib cannot recognize timezone like "GMT+8", but it can recognize "Etc/GMT-8". Actually they are equivalent. So we can do a conversion to support such timezone pattern. See a discussion in that community: https://github.com/HowardHinnant/date/issues/823.

[netlify[bot]]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	1fb948d058261cfd90461ab42f1ed411e92cd841
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6638e16e7fde460008f26b35

[PHILO-HE]

Hi @pedroerp, could you review this pr?

[aditi-pandit]

@svm1

[aditi-pandit]

@svm1

[pedroerp]

Another option is to use:

https://github.com/facebookincubator/velox/blob/main/velox/type/Timestamp.cpp#L78

it supports conversions of timezone offsets without relying on velox/external/date. It would be nice to align on a single method to support timezone offsets instead of creating a separate one.

https://github.com/facebookincubator/velox/blob/main/velox/type/Timestamp.cpp#L28

[PHILO-HE]

Another option is to use:

https://github.com/facebookincubator/velox/blob/main/velox/type/Timestamp.cpp#L78

it supports conversions of timezone offsets without relying on velox/external/date. It would be nice to align on a single method to support timezone offsets instead of creating a separate one.

https://github.com/facebookincubator/velox/blob/main/velox/type/Timestamp.cpp#L28

Hi @pedroerp, thanks for your comment! Do you mean we can just use tzID to support offset-based timezone, like https://github.com/facebookincubator/velox/pull/9403? Does the community plan to remove velox/external/date at some time point?

[pedroerp]

Do you mean we can just use tzID to support offset-based timezone, like https://github.com/facebookincubator/velox/pull/9403?

Yes :)

Does the community plan to remove velox/external/date at some time point?

velox/external/date is an implementation of std::chrono from C++20, so at the point where all platforms we need to support catch up and provide this as part of the standard library, we can remove it. But outside of that we still need this library to do timezone conversions (just not for offset-based ones).

[czentgr]

I had a question about this. The strings that are now changed to the Etc/GMT form. It appears that the Velox time zone key to timezone name mapping function does not generate the timezone string that would benefit from this change.

Prestissimo has a problem with the mappings that I have created an issue for: https://github.com/prestodb/presto/issues/22789 The string that the timezone key gets converted to by velox::util::getTimeZoneName() -> velox::util::getTimeZoneDB() in Prestissimo (toVeloxConfig()) isn't an actual timezone. Unfortunately, this PR wouldn't help with that issue because it requires the GMT prefix already being present.

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

[pedroerp]

Support for this was already added. We should be good to close this one.

[PHILO-HE]

Support for this was already added. We should be good to close this one.

@pedroerp, thanks! Closing this pr.