rok commented on a change in pull request #11358:
URL: https://github.com/apache/arrow/pull/11358#discussion_r725644761
##########
File path: cpp/src/arrow/compute/kernels/scalar_string_test.cc
##########
@@ -1429,6 +1430,17 @@ TYPED_TEST(TestStringKernels, Strptime) {
this->CheckUnary("strptime", input1, timestamp(TimeUnit::MICRO), output1,
&options);
}
+TYPED_TEST(TestStringKernels, StrptimeZoneOffset) {
+ if (!arrow::internal::kStrptimeSupportsZone) {
+ GTEST_SKIP() << "strptime does not support %z on this platform";
+ }
+ std::string input1 = R"(["5/1/2020 +01", null, "12/11/1900 -01:30"])";
+ std::string output1 =
+ R"(["2020-04-30T23:00:00.000000", null, "1900-12-11T01:30:00.000000"])";
+ StrptimeOptions options("%m/%d/%Y %z", TimeUnit::MICRO);
+ this->CheckUnary("strptime", input1, timestamp(TimeUnit::MICRO), output1,
&options);
Review comment:
ISO 8601 is annoyingly ambiguous on offsets - it allows for local time
(timezone naive), UTC (`Z`) and offset in time format. [See
wikipedia.](https://en.wikipedia.org/wiki/ISO_8601#Time_zone_designators)
So to follow ISO 8601 we should check which case (local/UTC/offset) we're
dealing with and return appropriately. If we want to do something else we
should probably call it something else to avoid confusion? Perhaps
`ISO8601_ignore_offset`.
Ignoring offsets at parse time is like implicitly rounding when casting.
Perhaps a lot of users expect it to work in a certain way but it is introducing
a really easy way to shoot oneself in the foot.
For returning type I agree with @jorisvandenbossche:
> Since timestamp strings with an offset represent a specific moment on the
timeline, it feels more correct to make it timezone-aware.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
