ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4258526489
##########
cpp/src/arrow/util/value_parsing.h:
##########
@@ -826,10 +826,24 @@ static inline bool ParseTimestampStrptime(const char*
buf, size_t length,
if (!allow_trailing_chars && static_cast<size_t>(ret - clean_copy.c_str())
!= length) {
return false;
}
+ // Reject leap seconds, which Arrow cannot represent, instead of adding them
+ // to the time of day (which wraps them into the next minute); consistent
with
+ // the ISO-8601 parser. See GH-50146 for how strptime implementations and
+ // standards differ on these values.
+ if (result.tm_sec >= 60) {
+ return false;
+ }
+ // Reject impossible dates such as February 30, which strptime accepts (the
+ // day of month is only checked to be in 1..31), instead of rolling them over
+ // into the next month; consistent with the ISO-8601 parser.
+ const arrow_vendored::date::year_month_day ymd =
+ arrow_vendored::date::year(result.tm_year + 1900) / (result.tm_mon + 1) /
+ std::max(result.tm_mday, 1);
Review Comment:
The `std::max(result.tm_mday, 1)` here still changes an explicitly parsed
day `00` into `01` before `ymd.ok()` can reject it:
- With `%d-%b-%y`, `00-FEB-15` becomes `2015-02-01`.
- With `%Y-%m-%d`, `2020-01-00` becomes `2020-01-01`.
I reproduced this for date32, date64, and timestamp columns, and confirmed
the underlying behavior with both macOS strptime and the vendored fallback.
Could we initialize `result.tm_mday = 1` before calling strptime, then use
`result.tm_mday` directly when constructing `ymd`? This distinguishes an
omitted day from an explicitly invalid zero day. I verified that it rejects
these examples while preserving month-only formats such as `%Y-%m`.
Please add regression tests rejecting both zero-day examples and retain the
test confirming that `2000-02` with `%Y-%m` becomes `2000-02-01`.
--
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]
