Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor internal time zone database #10572

Closed
wants to merge 1 commit into from

Conversation

pedroerp
Copy link
Contributor

@pedroerp pedroerp commented Jul 25, 2024

Summary:
Refactoring the internal time zone database to prepare for the next
PR. Moving map from ID to string to vector, to remove the map access
for the hot path. Also re-organizing the code to allow them to
reference each other.

Part of #10101

Differential Revision: D60211961

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 25, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D60211961

Copy link

netlify bot commented Jul 25, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit b57bc0f
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66a2abce857988000812d06f

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

extern const std::vector<std::pair<int16_t, std::string>>& getTimeZoneEntries();

// TODO: The string will be moved to TimeZone in the next PR.
using TTimeZoneDatabase = std::vector<std::unique_ptr<std::string>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use unique_pr<string>?

Copy link
Contributor Author

@pedroerp pedroerp Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next PR I'll move this to unique_ptr<TimeZone>, but I worried it would make this PR too large to review, so this seemed like a good compromise. For now unique_ptr<string> because I need to represent that some entries are empty (nullptr).

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D60211961

pedroerp added a commit to pedroerp/velox-1 that referenced this pull request Jul 25, 2024
Summary:
Pull Request resolved: facebookincubator#10572

Refactoring the internal time zone database to prepare for the next
PR. Moving map from ID to string to vector, to remove the map access
for the hot path. Also re-organizing the code to allow them to
reference each other.

Reviewed By: mbasmanova

Differential Revision: D60211961
Summary:
Pull Request resolved: facebookincubator#10572

Refactoring the internal time zone database to prepare for the next
PR. Moving map from ID to string to vector, to remove the map access
for the hot path. Also re-organizing the code to allow them to
reference each other.

Reviewed By: mbasmanova

Differential Revision: D60211961
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D60211961

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in e3e791a.

Copy link

Conbench analyzed the 1 benchmark run on commit e3e791a8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants