You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/12 16:25:35 UTC
[GitHub] [arrow] rok opened a new pull request, #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
rok opened a new pull request, #12865:
URL: https://github.com/apache/arrow/pull/12865
This is to add "offset timezones" to timestamp arrays.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12865:
URL: https://github.com/apache/arrow/pull/12865#issuecomment-1097011577
https://issues.apache.org/jira/browse/ARROW-14477
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #12865:
URL: https://github.com/apache/arrow/pull/12865#discussion_r848674815
##########
cpp/src/arrow/vendored/datetime/tz.h:
##########
@@ -294,6 +294,40 @@ struct zoned_traits
{
};
+class OffsetZone {
Review Comment:
Will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #12865:
URL: https://github.com/apache/arrow/pull/12865#discussion_r848662372
##########
cpp/src/arrow/vendored/datetime/tz.h:
##########
@@ -294,6 +294,40 @@ struct zoned_traits
{
};
+class OffsetZone {
Review Comment:
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on pull request #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
Posted by GitBox <gi...@apache.org>.
rok commented on PR #12865:
URL: https://github.com/apache/arrow/pull/12865#issuecomment-1096984213
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #12865:
URL: https://github.com/apache/arrow/pull/12865#discussion_r857494390
##########
cpp/src/arrow/util/date_internal.h:
##########
@@ -0,0 +1,61 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+namespace arrow_vendored {
+namespace date {
Review Comment:
Please don't put this in the `arrow_vendored` namespace as it's not vendored code.
```suggestion
namespace arrow {
namespace internal {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on pull request #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
Posted by GitBox <gi...@apache.org>.
rok commented on PR #12865:
URL: https://github.com/apache/arrow/pull/12865#issuecomment-1096954774
@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](https://howardhinnant.github.io/date/tz.html#:~:text=T%20just%20work.-,Custom%20time%20zone,-Occasionally%20the%20IANA) for custom time zones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #12865:
URL: https://github.com/apache/arrow/pull/12865#issuecomment-1096970123
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rok commented on a diff in pull request #12865: ARROW-14477: [C++] Timezone-aware kernels should also handle offset strings
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #12865:
URL: https://github.com/apache/arrow/pull/12865#discussion_r855730494
##########
cpp/src/arrow/vendored/datetime/tz.h:
##########
@@ -294,6 +294,40 @@ struct zoned_traits
{
};
+class OffsetZone {
Review Comment:
Moved. The code in the file is adapted from [here](https://howardhinnant.github.io/date/tz.html#Examples), but I've added our license header. Is that ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org