ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4262440339
##########
cpp/src/arrow/util/value_parsing_test.cc:
##########
@@ -869,11 +870,45 @@ TEST(TimestampParser, StrptimeParser) {
}
// Unparseable strings
- std::vector<std::string> unparseables = {"foo", "5/1/2000", "5/1/2000
12:34:56:6"};
+ std::vector<std::string> unparseables = {
+ "foo", "5/1/2000", "5/1/2000 12:34:56:6",
+ // Leap seconds are rejected, as by the
+ // ISO-8601 parser
+ "5/31/2000 12:30:60", "5/31/2000 23:59:60", "5/31/2000 23:59:61",
+ // Impossible dates are rejected rather
+ // than rolled over into the next month
+ "2/30/2000 12:34:56", "4/31/2000 12:34:56", "2/29/2001 12:34:56",
+ // A zero day is rejected rather than
+ // taken as the first of the month
+ "2/00/2000 12:34:56"};
for (auto& value : unparseables) {
int64_t dummy;
ASSERT_FALSE((*parser)(value.c_str(), value.size(), TimeUnit::SECOND,
&dummy));
}
+
+ // A format without a day still parses (the day defaults to the first)
+ auto month_parser = TimestampParser::MakeStrptime("%Y-%m");
+ int64_t converted, expected;
+ ASSERT_TRUE((*month_parser)("2000-02", 7, TimeUnit::SECOND, &converted));
+ ASSERT_TRUE(ParseTimestampISO8601("2000-02-01", 10, TimeUnit::SECOND,
&expected));
+ ASSERT_EQ(expected, converted);
+}
+
+TEST(TimestampParser, StrptimeYearOutOfRange) {
+ auto parser = TimestampParser::MakeStrptime("%s");
+ int64_t converted = 0;
+ if (!(*parser)("1584282600", 10, TimeUnit::SECOND, &converted)) {
Review Comment:
I think this test will fail on the `test-debian-13-cpp-i386` nightly job.
Debian i386 still uses a 32-bit `time_t`, and glibc's `%s` reads the digits
into a `time_t` with no overflow check. So all three values wrap to ordinary
dates and are accepted:
- `1000000000000000` → 1921-07-02 22:30:24
- `67768036000000000` → 2030-01-10 03:33:20
- `67768036191676799` → 2036-02-06 15:06:39
`1584282600` fits in 32 bits, so the skip doesn't trigger there. Could the
skip check use a value past 2038 instead? For example:
```cpp
// A value past 2038, as strptime may parse %s into a 32-bit time_t (as
glibc
// does on i386), which wraps larger values
if (!(*parser)("4102444800", 10, TimeUnit::SECOND, &converted) ||
converted != 4102444800) {
GTEST_SKIP() << "strptime does not support 64-bit %s values on this
platform";
}
```
With a 32-bit `time_t`, `4102444800` (2100-01-01) wraps to 1963-11-25, so
the test would be skipped. On macOS it parses correctly with this PR, so the
test still runs there. I couldn't run this on i386. The dates above come from
glibc's `time/strptime_l.c` and 32-bit arithmetic, so it may be worth checking
with `@github-actions crossbow submit test-debian-13-cpp-i386`.
--
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]
