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

ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings #12865

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rok
Copy link
Member

@rok rok commented Apr 12, 2022

Currently timestamp arrays have unit timestamp(unit, zone name). This would add "offset timezones" where timestamp array would also support units like timestamp(unit, "GMT+/-HH:MM").

@rok
Copy link
Member Author

rok commented Apr 12, 2022

@pitrou @lidavidm @jorisvandenbossche this is complete yet but I would like to validate the approach with you before I continue. Do you feel this makes enough sense to complete the PR?
Please note date.h recognises zones names like "Etc/GMT+10" but only in iterations of 1hr, while we might want to cover 30 minute iterations. I'm basing this off of the date.h recommendation for custom time zones.

@pitrou
Copy link
Member

pitrou commented Apr 12, 2022

Please note date.h recognises zones names like "Etc/GMT+10" but only in iterations of 1hr, while we might want to cover 30 minute iterations.

What is this comment in reference to? It seems this PR would handle 30 minute iterations correctly?

@@ -294,6 +294,40 @@ struct zoned_traits
{
};

class OffsetZone {
Copy link
Member

Choose a reason for hiding this comment

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

As a policy, we shouldn't add our own code inside vendored libraries. Just put it somewhere else in the source tree (for example in a arrow/util/date_internal.h where the vendored code would be included)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved. The code in the file is adapted from here, but I've added our license header. Is that ok?

@rok
Copy link
Member Author

rok commented Apr 12, 2022

What is this comment in reference to? It seems this PR would handle 30 minute iterations correctly?

Current implementation can handle fixed offsets of iterations of one hour. This PR would enable offsets of arbitrary number of minutes. So primary use of that would indeed be to operate in GMT+/-HH:30 offsets. Otherwise we don't need an extra type just a translation utility for +/-HH:00 -> Etc/GMT+/-HH.

@github-actions
Copy link

Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants