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

Adding at_timezone feature #7708

Closed
wants to merge 14 commits into from
Closed

Conversation

svm1
Copy link
Collaborator

@svm1 svm1 commented Nov 22, 2023

Adding at_timezone ("timestamp at timezone") feature.

SELECT timestamp '2012-10-31 01:00 UTC' AT TIME ZONE 'America/Los_Angeles';

2012-10-30 18:00:00.000 America/Los_Angeles

Copy link

netlify bot commented Nov 22, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 83f3491
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65b1bdd814719f0008bd6e8d

@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 Nov 22, 2023
@svm1 svm1 mentioned this pull request Nov 22, 2023
@svm1 svm1 changed the title Adding "timestamp at timezone" feature Adding at_timezone feature Nov 22, 2023
@svm1 svm1 force-pushed the at_timezone branch 2 times, most recently from 967e216 to 459a624 Compare November 22, 2023 23:17
velox/functions/prestosql/DateTimeFunctions.h Outdated Show resolved Hide resolved
velox/functions/prestosql/DateTimeFunctions.h Outdated Show resolved Hide resolved
velox/functions/prestosql/DateTimeFunctions.h Outdated Show resolved Hide resolved
std::chrono::system_clock::time_point tp{std::chrono::seconds{ts.getSeconds()}};

// Africa/Abidjan has offset +00:00 -> use this timezone to establish UTC as starting offset
auto startZonedTime = date::make_zoned("Africa/Abidjan", tp);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not GMT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was using the terms interchangeably as they are practically equivalent in this context - can replace with GMT. My point was that the input time is to be interpreted at GMT, so the Africa/Abidjan timezone, with offset of +00:00, is used to construct a starting timepoint based on the input timestamp.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we not use the actual string "GMT" for clarity?

velox/functions/prestosql/DateTimeFunctions.h Outdated Show resolved Hide resolved
velox/functions/prestosql/DateTimeFunctions.h Show resolved Hide resolved
@svm1 svm1 marked this pull request as ready for review December 1, 2023 20:47
@pedroerp
Copy link
Contributor

pedroerp commented Dec 14, 2023

@svm1 Sorry about the delay in getting back at this PR; somehow I lost the notification. Feel free to ping me on Slack if things hang around for too long.

@svm1
Copy link
Collaborator Author

svm1 commented Jan 11, 2024

Thanks for looking over @pedroerp - I must apologize now, I must've missed your ping over the holiday season!

After clarifying my understanding of the TimestampWithTimezone type and observing functionality on Presto Java, I've come to realize some issues with my implementation. Currently adjusting and making the necessary changes, I'll reach out to you again shortly once everything is in place.

@pedroerp
Copy link
Contributor

pedroerp commented Apr 6, 2024

@svm1 are you planning to get back to finish this PR?

@svm1
Copy link
Collaborator Author

svm1 commented Apr 6, 2024

@pedroerp I opened a new PR for the same here - #8554.

I initially split the PR into two, for the Timestamp and TimestampWithTimezone signatures, respectively - but I later realized the former was unnecessary to the coordinator implicitly folding it to a TimestampWithTimezone type at session time zone.

@pedroerp
Copy link
Contributor

pedroerp commented Apr 8, 2024

@svm1 sounds good, I'll check that one. Can you close this one?

@svm1 svm1 closed this Apr 9, 2024
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants