feat: Replace tz implementation from date with LLVM implementation #12422
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevinwilfong
requested review from
majetideepak and
assignUser
as code owners
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
force-pushed
the
export-D69997637
branch
from
146e0d3 to
70babae
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 21, 2025
…acebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 21, 2025
…acebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
kevinwilfong
force-pushed
the
export-D69997637
branch
from
70babae to
32485f0
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 21, 2025
…acebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
kevinwilfong
force-pushed
the
export-D69997637
branch
from
32485f0 to
dd60d31
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 21, 2025
…acebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
kevinwilfong
force-pushed
the
export-D69997637
branch
from
dd60d31 to
4c3d587
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
force-pushed
the
export-D69997637
branch
from
4c3d587 to
b2a253e
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 25, 2025
…acebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 26, 2025
…acebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
kevinwilfong
force-pushed
the
export-D69997637
branch
from
b2a253e to
df47fe6
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
force-pushed
the
export-D69997637
branch
from
df47fe6 to
621cd13
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 27, 2025
…acebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 27, 2025
…/4] (facebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
kevinwilfong
force-pushed
the
export-D69997637
branch
from
621cd13 to
07c5b89
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 28, 2025
…/4] (facebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
kevinwilfong
force-pushed
the
export-D69997637
branch
from
07c5b89 to
8876d90
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
force-pushed
the
export-D69997637
branch
from
8876d90 to
20ca0d5
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Feb 28, 2025
…/4] (facebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 TODO Differential Revision: D69997637
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
force-pushed
the
export-D69997637
branch
from
20ca0d5 to
b6a866f
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Mar 5, 2025
…/4] (facebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 We currently use (slightly modified versions of) the tz.h/tz.cpp (along with ios.h/tz_private.h) files from https://github.com/HowardHinnant/date to handle time zone conversions. A version of this API was voted into the C++20 standard. Since then further support/development of the library has been limited. In particular it has a known limitation when daylight savings time rules used to determine transition dates/times into the distant future are not supported in the library. This means Velox only supports datetimes in time zones with DST up to the year 2037. LLVM has an implementation of the new C++20 standard that is heavily based on that in the date repo. It is not finished yet, and still considered "experimental" but it already provides some notable improvements, e.g. support of DST into the distant future. In order to take advantage of this and prepare us to adopt the C++20 library once it's available, I've replace the tz.h/tz.cpp files with the set of files from LLVM that implement the API in std::chrono in C++20. This first PR simply pulls in the files from LLVM, removes some libcpp specific macros, replaces C++20-isms with C++17 equivalents, and updates namespaces and includes. The only meaningful code change in these files is I ported the logic to find the necessary zoneinfo files/directories on Mac from the date library as LLVM only supports finding them in Linux systems. Beyond that I updated includes/namspaces in the rest of the velox codebase to use the new files. I commented out/modified some code (mostly tests) that are broken by the change, in case I include a comment indicating the follow up PR that addresses the issue and restores the code. I will land these as a single stack, I did this to simplify the review process and ensure each PR individually builds and passes tests. Note that we still use the date.h and ios_week.h files from https://github.com/HowardHinnant/date for two reasons: 1) These were not fully adopted by the C++20 standard and we use some of the types that did not make it into the standard. 2) There's some discussion of supporting a range of years beyond what std::year supports to match Presto Java's behavior, which will require forking these anyway. Differential Revision: D69997637
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
force-pushed
the
export-D69997637
branch
from
b6a866f to
c2bd099
Compare
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Mar 5, 2025
…/4] (facebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 We currently use (slightly modified versions of) the tz.h/tz.cpp (along with ios.h/tz_private.h) files from https://github.com/HowardHinnant/date to handle time zone conversions. A version of this API was voted into the C++20 standard. Since then further support/development of the library has been limited. In particular it has a known limitation when daylight savings time rules used to determine transition dates/times into the distant future are not supported in the library. This means Velox only supports datetimes in time zones with DST up to the year 2037. LLVM has an implementation of the new C++20 standard that is heavily based on that in the date repo. It is not finished yet, and still considered "experimental" but it already provides some notable improvements, e.g. support of DST into the distant future. In order to take advantage of this and prepare us to adopt the C++20 library once it's available, I've replace the tz.h/tz.cpp files with the set of files from LLVM that implement the API in std::chrono in C++20. This first PR simply pulls in the files from LLVM, removes some libcpp specific macros, replaces C++20-isms with C++17 equivalents, and updates namespaces and includes. The only meaningful code change in these files is I ported the logic to find the necessary zoneinfo files/directories on Mac from the date library as LLVM only supports finding them in Linux systems. Beyond that I updated includes/namspaces in the rest of the velox codebase to use the new files. I commented out/modified some code (mostly tests) that are broken by the change, in case I include a comment indicating the follow up PR that addresses the issue and restores the code. I will land these as a single stack, I did this to simplify the review process and ensure each PR individually builds and passes tests. Note that we still use the date.h and ios_week.h files from https://github.com/HowardHinnant/date for two reasons: 1) These were not fully adopted by the C++20 standard and we use some of the types that did not make it into the standard. 2) There's some discussion of supporting a range of years beyond what std::year supports to match Presto Java's behavior, which will require forking these anyway. Differential Revision: D69997637
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Mar 5, 2025
…/4] (facebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 We currently use (slightly modified versions of) the tz.h/tz.cpp (along with ios.h/tz_private.h) files from https://github.com/HowardHinnant/date to handle time zone conversions. A version of this API was voted into the C++20 standard. Since then further support/development of the library has been limited. In particular it has a known limitation when daylight savings time rules used to determine transition dates/times into the distant future are not supported in the library. This means Velox only supports datetimes in time zones with DST up to the year 2037. LLVM has an implementation of the new C++20 standard that is heavily based on that in the date repo. It is not finished yet, and still considered "experimental" but it already provides some notable improvements, e.g. support of DST into the distant future. In order to take advantage of this and prepare us to adopt the C++20 library once it's available, I've replace the tz.h/tz.cpp files with the set of files from LLVM that implement the API in std::chrono in C++20. This first PR simply pulls in the files from LLVM, removes some libcpp specific macros, replaces C++20-isms with C++17 equivalents, and updates namespaces and includes. The only meaningful code change in these files is I ported the logic to find the necessary zoneinfo files/directories on Mac from the date library as LLVM only supports finding them in Linux systems. Beyond that I updated includes/namspaces in the rest of the velox codebase to use the new files. I commented out/modified some code (mostly tests) that are broken by the change, in case I include a comment indicating the follow up PR that addresses the issue and restores the code. I will land these as a single stack, I did this to simplify the review process and ensure each PR individually builds and passes tests. Note that we still use the date.h and ios_week.h files from https://github.com/HowardHinnant/date for two reasons: 1) These were not fully adopted by the C++20 standard and we use some of the types that did not make it into the standard. 2) There's some discussion of supporting a range of years beyond what std::year supports to match Presto Java's behavior, which will require forking these anyway. Differential Revision: D69997637
kevinwilfong
force-pushed
the
export-D69997637
branch
from
c2bd099 to
f94f9ab
Compare
kgpai
approved these changes
Mar 7, 2025
…/4] (facebookincubator#12422) Summary: Pull Request resolved: facebookincubator#12422 We currently use (slightly modified versions of) the tz.h/tz.cpp (along with ios.h/tz_private.h) files from https://github.com/HowardHinnant/date to handle time zone conversions. A version of this API was voted into the C++20 standard. Since then further support/development of the library has been limited. In particular it has a known limitation when daylight savings time rules used to determine transition dates/times into the distant future are not supported in the library. This means Velox only supports datetimes in time zones with DST up to the year 2037. LLVM has an implementation of the new C++20 standard that is heavily based on that in the date repo. It is not finished yet, and still considered "experimental" but it already provides some notable improvements, e.g. support of DST into the distant future. In order to take advantage of this and prepare us to adopt the C++20 library once it's available, I've replace the tz.h/tz.cpp files with the set of files from LLVM that implement the API in std::chrono in C++20. This first PR simply pulls in the files from LLVM, removes some libcpp specific macros, replaces C++20-isms with C++17 equivalents, and updates namespaces and includes. The only meaningful code change in these files is I ported the logic to find the necessary zoneinfo files/directories on Mac from the date library as LLVM only supports finding them in Linux systems. Beyond that I updated includes/namspaces in the rest of the velox codebase to use the new files. I commented out/modified some code (mostly tests) that are broken by the change, in case I include a comment indicating the follow up PR that addresses the issue and restores the code. I will land these as a single stack, I did this to simplify the review process and ensure each PR individually builds and passes tests. Note that we still use the date.h and ios_week.h files from https://github.com/HowardHinnant/date for two reasons: 1) These were not fully adopted by the C++20 standard and we use some of the types that did not make it into the standard. 2) There's some discussion of supporting a range of years beyond what std::year supports to match Presto Java's behavior, which will require forking these anyway. Reviewed By: kgpai, yuandagits Differential Revision: D69997637
|
This pull request was exported from Phabricator. Differential Revision: D69997637 |
kevinwilfong
force-pushed
the
export-D69997637
branch
from
f94f9ab to
7940436
Compare
|
This pull request has been merged in 16e4460. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
We currently use (slightly modified versions of) the tz.h/tz.cpp (along with ios.h/tz_private.h) files
from https://github.com/HowardHinnant/date to handle time zone conversions. A version of this
API was voted into the C++20 standard. Since then further support/development of the library has
been limited.
In particular it has a known limitation when daylight savings time rules used to determine transition
dates/times into the distant future are not supported in the library. This means Velox only supports
datetimes in time zones with DST up to the year 2037.
LLVM has an implementation of the new C++20 standard that is heavily based on that in the date
repo. It is not finished yet, and still considered "experimental" but it already provides some notable
improvements, e.g. support of DST into the distant future.
In order to take advantage of this and prepare us to adopt the C++20 library once it's available,
I've replace the tz.h/tz.cpp files with the set of files from LLVM that implement the API in
std::chrono in C++20.
This first PR simply pulls in the files from LLVM, removes some libcpp specific macros, replaces C++20-isms with C++17 equivalents, and updates namespaces and includes. The only meaningful code change in these files is I ported the logic to find the necessary zoneinfo files/directories on Mac from the date library as LLVM only supports finding them in Linux systems.
Beyond that I updated includes/namspaces in the rest of the velox codebase to use the new files. I commented out/modified some code (mostly tests) that are broken by the change, in case I include a comment indicating the follow up PR that addresses the issue and restores the code. I will land these as a single stack, I did this to simplify the review process and ensure each PR individually builds and passes tests.
Note that we still use the date.h and ios_week.h files from https://github.com/HowardHinnant/date
for two reasons:
make it into the standard.
match Presto Java's behavior, which will require forking these anyway.
Differential Revision: D69997637