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.

Sign up for GitHub

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

Jump to bottom

Support offset-based timezone #9591

Closed
wants to merge 1 commit into from
Closed

Support offset-based timezone #9591

wants to merge 1 commit into from

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented Apr 24, 2024 •
edited
Loading

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: HowardHinnant/date#823.

@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 Apr 24, 2024
@netlify Netlify
Copy link

netlify bot commented Apr 24, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

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

@PHILO-HE PHILO-HE force-pushed the GMT-timezone branch 4 times, most recently from dc71af9 to ef9f0c9 Compare April 24, 2024 07:00
jinchengchenghh
jinchengchenghh reviewed Apr 24, 2024
View reviewed changes
velox/functions/lib/TimeUtils.h Outdated
return timezone;
}

// If user doesn't explicitly ask us to adjust the timezone or configured
Copy link
Contributor

Choose a reason for hiding this comment

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

/// for multiple line

velox/functions/lib/TimeUtils.h Outdated
@@ -26,12 +26,30 @@ inline constexpr int64_t kSecondsInDay = 86'400;
inline constexpr int64_t kDaysInWeek = 7;
extern const folly::F14FastMap<std::string, int8_t> kDayOfWeekNames;

// Format timezone to make it compatible with date::locate_zone.
// For example, converts "GMT+8" to "Etc/GMT-8".
// Here is the list of IANA timezone names:
Copy link
Contributor

Choose a reason for hiding this comment

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

same

velox/expression/CastExpr-inl.h Outdated
rawTimestamps[row].toGMT(*timeZone);
});
}
auto* timeZone =
Copy link
Contributor

Choose a reason for hiding this comment

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

const auto*

velox/expression/tests/CastExprTest.cpp
@@ -2385,8 +2405,8 @@ class TestingDictionaryToFewerRowsFunction : public exec::VectorFunction {
exec::EvalCtx& context,
VectorPtr& result) const override {
const auto size = rows.size();
auto indices =
makeIndices(size, [](auto /*row*/) { return 0; }, context.pool());
auto indices = makeIndices(
Copy link
Contributor

Choose a reason for hiding this comment

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

irrelevant change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is produced by code format tool. Thanks!

velox/functions/lib/TimeUtils.h Outdated
if (timezone[3] != '+' && timezone[3] != '-') {
return timezone;
}
std::string prefix = timezone[3] == '+' ? "Etc/GMT-" : "Etc/GMT+";
Copy link
Contributor

Choose a reason for hiding this comment

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

This format will be more efficient and precise.

if (timezone[3] == '+') {
      return "Etc/GMT-" + timezone.substr(4);
    } else if (timezone[3] == '-') {
      return "Etc/GMT+" + timezone.substr(4);
    } else {
      return timezone;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jinchengchenghh, yes, thanks for your suggestion! Just updated.

velox/expression/CastExpr.cpp Outdated
auto sessionTzName = queryConfig.sessionTimezone();
if (queryConfig.adjustTimestampToTimezone() && !sessionTzName.empty()) {
auto* timeZone = date::locate_zone(sessionTzName);
auto* timeZone =
Copy link
Contributor

Choose a reason for hiding this comment

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

const auto*

This was referenced Apr 24, 2024
Open
Open
@PHILO-HE PHILO-HE changed the title Support GMT+x/GMT-x timezone Support offset-based timezone Apr 24, 2024
@PHILO-HE
Copy link
Contributor Author

Hi @pedroerp, could you review this pr?

@pedroerp pedroerp self-requested a review April 24, 2024 15:31
@aditi-pandit
Copy link
Collaborator

@svm1

1 similar comment
@aditi-pandit
Copy link
Collaborator

@svm1

pedroerp
pedroerp reviewed Apr 25, 2024
View reviewed changes
velox/expression/CastExpr-inl.h Outdated
rawTimestamps[row].toGMT(*timeZone);
});
}
const auto* timeZone = functions::getTimeZoneFromConfig(queryConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

@PHILO-HE can you rebase? I think I removed this block in the timezone fix I merged yesterday

@pedroerp
Copy link
Contributor

pedroerp commented Apr 25, 2024 •
edited
Loading

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
Copy link
Contributor Author

PHILO-HE commented May 3, 2024

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 #9403? Does the community plan to remove velox/external/date at some time point?

@pedroerp
Copy link
Contributor

pedroerp commented May 8, 2024 •
edited
Loading

Do you mean we can just use tzID to support offset-based timezone, like #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
Copy link
Collaborator

czentgr commented May 20, 2024

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: prestodb/presto#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 Stale
Copy link

stale bot commented Aug 19, 2024

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!

@stale stale bot added the stale label Aug 19, 2024
@pedroerp
Copy link
Contributor

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

@stale stale bot removed the stale label Aug 21, 2024
@PHILO-HE
Copy link
Contributor Author

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

@pedroerp, thanks! Closing this pr.

@PHILO-HE PHILO-HE closed this Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinchengchenghh jinchengchenghh jinchengchenghh left review comments

@pedroerp pedroerp pedroerp left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PHILO-HE @aditi-pandit @pedroerp @czentgr @jinchengchenghh @facebook-github-bot