[GitHub] [arrow] westonpace commented on a change in pull request #10998: ARROW-13684: [C++][Compute] Strftime kernel follow-up

[GitBox <gi...@apache.org>]

westonpace commented on a change in pull request #10998:
URL: https://github.com/apache/arrow/pull/10998#discussion_r696849507



##########
File path: python/pyarrow/tests/test_compute.py
##########
@@ -1492,18 +1492,25 @@ def _fix_timestamp(s):
 
         # Test setting locale
         tsa = pa.array(ts, type=pa.timestamp("s", timezone))
-        options = pc.StrftimeOptions("%Y-%m-%dT%H:%M:%SZ", "C")

Review comment:
       Did you leave at least one test case where you do print the time zone?

##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -414,12 +414,18 @@ TEST_F(ScalarTemporalTest, Strftime) {
 }
 
 TEST_F(ScalarTemporalTest, StrftimeNoTimezone) {
+  auto options_default = StrftimeOptions();
   const char* seconds = R"(["1970-01-01T00:00:59", null])";
   auto arr = ArrayFromJSON(timestamp(TimeUnit::SECOND), seconds);
+
+  CheckScalarUnary("strftime", timestamp(TimeUnit::SECOND), seconds, utf8(), seconds,
+                   &options_default);
   EXPECT_RAISES_WITH_MESSAGE_THAT(
-      Invalid,
-      testing::HasSubstr("Timestamps without a time zone cannot be reliably formatted"),
-      Strftime(arr, StrftimeOptions()));
+      Invalid, testing::HasSubstr("Invalid: Timezone not present, cannot print"),

Review comment:
       Nit: I don't know if `cannot print` is quite right.  The user might be running strftime to store or display on a GUI or send to some other tool.  Perhaps `cannot convert to string`?  Or `cannot format`?

##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -835,6 +839,10 @@ const FunctionDoc strftime_doc{
     "Format timestamps according to a format string",
     ("For each input timestamp, emit a formatted string.\n"
      "The time format string and locale can be set using StrftimeOptions.\n"
+     "Output precision of seconds %S flag depends on the input timestamp precision. "
+     "Timestamps with second precision are represented as integers while microsecond "
+     "precision timestamps are represented as a floating point number with 6 decimal "
+     "points."

Review comment:
       This comment seems to suggest that only microsecond and second are supported.  I like the wording down below which makes it more clear that seconds through nanoseconds are supported.




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