zilto commented on code in PR #12865:
URL: https://github.com/apache/arrow/pull/12865#discussion_r2229427897


##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -1979,6 +2003,16 @@ TEST_F(ScalarTemporalTest, 
TestAssumeTimezoneNonexistent) {
                    &options_earliest);
 }
 
+TEST_F(ScalarTemporalTest, StrftimeOffsetTimezone) {
+  auto options_ymdhms = StrftimeOptions("%Y-%m-%dT%H:%M:%S");
+
+  const char* seconds = R"(["1970-01-01T01:59:00", "2021-08-18T16:12:00", 
null])";
+  const char* seconds_offset = R"(["1970-01-01T03:00:00", 
"2021-08-18T17:13:00", null])";
+
+  CheckScalarUnary("strftime", timestamp(TimeUnit::SECOND, "+01:01"), seconds, 
utf8(),

Review Comment:
   Hi! I have no proficiency in C++ to review the code. I shared it with 
colleagues, but no one could review the PR either. 
   
   > Perhaps we should limit that to 15min increments then?
   Minute resolution makes sense given the format `+XX:XX`. Users would be 
responsible for handling values downstream. Values that aren't in 15min 
increments simply wouldn't match any "named IANA time zone"
   



##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -1979,6 +2003,16 @@ TEST_F(ScalarTemporalTest, 
TestAssumeTimezoneNonexistent) {
                    &options_earliest);
 }
 
+TEST_F(ScalarTemporalTest, StrftimeOffsetTimezone) {
+  auto options_ymdhms = StrftimeOptions("%Y-%m-%dT%H:%M:%S");
+
+  const char* seconds = R"(["1970-01-01T01:59:00", "2021-08-18T16:12:00", 
null])";
+  const char* seconds_offset = R"(["1970-01-01T03:00:00", 
"2021-08-18T17:13:00", null])";
+
+  CheckScalarUnary("strftime", timestamp(TimeUnit::SECOND, "+01:01"), seconds, 
utf8(),

Review Comment:
   Hi! I have no proficiency in C++ to review the code. I shared it with 
colleagues, but no one could review the PR either. 
   
   > Perhaps we should limit that to 15min increments then?
   
   Minute resolution makes sense given the format `+XX:XX`. Users would be 
responsible for handling values downstream. Values that aren't in 15min 
increments simply wouldn't match any "named IANA time zone"
   



-- 
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-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to