[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #34208: GH-33143: [C++] Kernel to convert timestamp with timezone to wall time

["jorisvandenbossche (via GitHub)" <gi...@apache.org>]

jorisvandenbossche commented on code in PR #34208:
URL: https://github.com/apache/arrow/pull/34208#discussion_r1111919924


##########
cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc:
##########
@@ -1784,6 +1807,13 @@ const FunctionDoc is_dst_doc{
      "An error is returned if the values do not have a defined timezone."),
     {"values"}};
 
+const FunctionDoc local_time_doc{
+    "Convert timestamp to a timezone-naive local time timestamp",
+    ("LocalTime converts a timestamp to a local time of timestamps timezone\n"

Review Comment:
   Something is missing in this sentence? 



##########
cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc:
##########
@@ -1801,8 +1831,9 @@ const FunctionDoc ceil_temporal_doc{
 const FunctionDoc round_temporal_doc{
     "Round temporal values to the nearest multiple of specified time unit",
     ("Null values emit null.\n"
-     "An error is returned if the values have a defined timezone but it\n"
-     "cannot be found in the timezone database."),
+     "If timezone is not given then timezone naive timestamp in UTC are\n"
+     "returned. An error is returned if the values have a defined timezone\n"
+     "but it cannot be found in the timezone database."),

Review Comment:
   Was this change intentional? Or was it meant to be in `local_time_doc`? 
   (and if intentional, why only for round and not ceil/floor?)



##########
cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc:
##########
@@ -1784,6 +1807,13 @@ const FunctionDoc is_dst_doc{
      "An error is returned if the values do not have a defined timezone."),
     {"values"}};
 
+const FunctionDoc local_time_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"

Review Comment:
   Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?



-- 
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