kou commented on code in PR #54657:
URL: https://github.com/apache/arrow/pull/54657#discussion_r4333841387
##########
cpp/src/arrow/util/value_parsing_test.cc:
##########
@@ -869,7 +870,14 @@ 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",
+ // 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"};
Review Comment:
Could you also add a zero month case?
BTW, could you also apply this change?
```diff
diff --git a/cpp/src/arrow/vendored/musl/strptime.c
b/cpp/src/arrow/vendored/musl/strptime.c
index eb325108b8..13c0f3172d 100644
--- a/cpp/src/arrow/vendored/musl/strptime.c
+++ b/cpp/src/arrow/vendored/musl/strptime.c
@@ -212,7 +212,7 @@ char *strptime(const char *__restrict s, const char
*__restrict f, struct tm *__
*dest = 0;
for (i=1; i<=min+range && isdigit(*s); i*=10)
*dest = *dest * 10 + *s++ - '0';
- if (*dest - min >= range) return 0;
+ if ((unsigned)(*dest - min) >= (unsigned)range) return
0;
*dest -= adj;
switch((char *)dest - (char *)tm) {
case offsetof(struct tm, tm_yday):
```
The upstream has `(unsigned)range` here:
https://git.musl-libc.org/cgit/musl/plain/src/time/strptime.c
Our vendored version doesn't have it. It may have been removed to avoid a
`-Wsign-compare` warning. So I also cast `*dest - min` to `unsigned` to avoid
the warning.
The change in this PR fixes the problem caused by this difference. So this
change isn't strictly needed. But I want to keep our vendored version as close
to the upstream as possible.
--
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]
