You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rok (via GitHub)" <gi...@apache.org> on 2023/02/20 16:43:39 UTC
[GitHub] [arrow] rok opened a new pull request, #34263: MINOR: [C++] Naming and doc/test changes for local_time compute kernel
rok opened a new pull request, #34263:
URL: https://github.com/apache/arrow/pull/34263
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
--
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 #34263: GH-33143: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #34263:
URL: https://github.com/apache/arrow/pull/34263#discussion_r1113660985
##########
cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc:
##########
@@ -1807,11 +1807,12 @@ const FunctionDoc is_dst_doc{
"An error is returned if the values do not have a defined timezone."),
{"values"}};
-const FunctionDoc local_time_doc{
+const FunctionDoc local_timestamp_doc{
"Convert timestamp to a timezone-naive local time timestamp",
- ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
- "and removes timezone metadata. If input is in UTC or doesn't have\n"
- "timezone metadata, it is returned as is.\n"
+ ("LocalTimestamp converts timezone-aware timestamp to local timestamp\n"
+ "of the given timestamps timezone and removes timezone metadata.\n"
Review Comment:
Changed and added.
--
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 #34263: GH-33143: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #34263:
URL: https://github.com/apache/arrow/pull/34263#issuecomment-1438818319
Sorry @kou this is indeed not minor. @jorisvandenbossche thanks for linking to the original issue, I think this mapping is satisfactory?
I think this is also ready for review.
--
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] westonpace commented on pull request #34263: GH-33143: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34263:
URL: https://github.com/apache/arrow/pull/34263#issuecomment-1439295476
This looks fine to me.
--
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 #34263: GH-33143: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #34263:
URL: https://github.com/apache/arrow/pull/34263#discussion_r1113660854
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -834,7 +834,7 @@ SCALAR_EAGER_UNARY(DayOfYear, "day_of_year")
SCALAR_EAGER_UNARY(Hour, "hour")
SCALAR_EAGER_UNARY(YearMonthDay, "year_month_day")
SCALAR_EAGER_UNARY(IsDaylightSavings, "is_dst")
-SCALAR_EAGER_UNARY(LocalTime, "local_time")
+SCALAR_EAGER_UNARY(LocalTime, "local_timestamp")
Review Comment:
Good catch! Fixed.
--
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] ursabot commented on pull request #34263: GH-33143: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34263:
URL: https://github.com/apache/arrow/pull/34263#issuecomment-1442142438
Benchmark runs are scheduled for baseline = e0e740bd7a24de68262c0b7e47eeed62a6cbd2a0 and contender = 863cdd4272bd5caf5b3e73ad5a9832f520624827. 863cdd4272bd5caf5b3e73ad5a9832f520624827 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/c8c5c9549352408788d9a2e202c3c81a...31c81b96549d493faf2364c15b174b43/)
[Failed :arrow_down:0.55% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/cfbde4d6afab457e9bce9e5b33ad7f6f...49830bde94c84c4e9dd63709452322ad/)
[Finished :arrow_down:1.79% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/a217ccf3b3f74cfbab8909f91d5b34c0...13fa94b856b64f91b00f9ec2e7bae4ff/)
[Finished :arrow_down:0.48% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/f5a181317ad94ed5ba15fe6546d87936...64817f6a418c449499371a0c724bbb57/)
Buildkite builds:
[Finished] [`863cdd42` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2423)
[Failed] [`863cdd42` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2452)
[Finished] [`863cdd42` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2420)
[Finished] [`863cdd42` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2444)
[Finished] [`e0e740bd` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2422)
[Finished] [`e0e740bd` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2451)
[Finished] [`e0e740bd` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2419)
[Finished] [`e0e740bd` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2443)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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 merged pull request #34263: GH-33143: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok merged PR #34263:
URL: https://github.com/apache/arrow/pull/34263
--
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] jorisvandenbossche commented on a diff in pull request #34263: GH-33143: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #34263:
URL: https://github.com/apache/arrow/pull/34263#discussion_r1113545506
##########
cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc:
##########
@@ -1807,11 +1807,12 @@ const FunctionDoc is_dst_doc{
"An error is returned if the values do not have a defined timezone."),
{"values"}};
-const FunctionDoc local_time_doc{
+const FunctionDoc local_timestamp_doc{
"Convert timestamp to a timezone-naive local time timestamp",
- ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
- "and removes timezone metadata. If input is in UTC or doesn't have\n"
- "timezone metadata, it is returned as is.\n"
+ ("LocalTimestamp converts timezone-aware timestamp to local timestamp\n"
+ "of the given timestamps timezone and removes timezone metadata.\n"
Review Comment:
"timestamps timezone" -> should that be "timestamp's timezone" ? (otherwise I don't fully understand the sentence)
Maybe we can also include "wall clock time" somewhere in the explanation?
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -834,7 +834,7 @@ SCALAR_EAGER_UNARY(DayOfYear, "day_of_year")
SCALAR_EAGER_UNARY(Hour, "hour")
SCALAR_EAGER_UNARY(YearMonthDay, "year_month_day")
SCALAR_EAGER_UNARY(IsDaylightSavings, "is_dst")
-SCALAR_EAGER_UNARY(LocalTime, "local_time")
+SCALAR_EAGER_UNARY(LocalTime, "local_timestamp")
Review Comment:
Also rename LocalTime ?
--
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] kou commented on pull request #34263: MINOR: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34263:
URL: https://github.com/apache/arrow/pull/34263#issuecomment-1437506784
Could you open a new issue for this?
This is not a MINOR change.
See also: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#minor-fixes
--
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 #34263: GH-33143: [C++] Naming and doc/test changes for local_time compute kernel
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34263:
URL: https://github.com/apache/arrow/pull/34263#issuecomment-1437508396
* Closes: #33143
--
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