pearu commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4261129725
##########
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:
Thanks, done in 5e683d0185 as you suggested (`tm_mday = 1` before strptime,
no `std::max`), with regression tests rejecting `00-FEB-15` and `2020-01-00`
for date32, date64 and timestamp columns. The `%Y-%m` → `2000-02-01` test is
kept.
##########
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) /
Review Comment:
Thanks, applied your fix in 292cd37b18. The regression test uses `%s` and is
skipped where `%s` does not parse. Besides your two values it includes
`67768036000000000`, since glibc's `%s` rejects `67768036191676799` but accepts
this one, which overflows the `int` addition.
##########
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) {
Review Comment:
Thanks, moved the check in 71652c4c27, with `2020-12-31 23:59:60` and
`YYYY-MM-DD HH24:MI:SS` as a `to_date` regression test. The PR description now
lists the compute `strptime` function, Gandiva's `to_date` and Flight's cookie
`Expires` along with timestamp columns.
--
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]
