ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4258644279
##########
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:
This check also runs when `ignore_time_in_day` is true, which is how
Gandiva's `to_date` calls this function. Even though the time part is thrown
away, calling it that way on `2020-12-31 23:59:60` with `%Y-%m-%d %H:%M:%S`
used to return 2020-12-31 and now fails. Could we move the check inside the `if
(!ignore_time_in_day)` block below? I checked that this restores the date-only
result while still rejecting the value when the time is used.
Also, besides CSV timestamp columns, this function is used by the compute
`strptime` function, Gandiva's `to_date`, and Flight's cookie `Expires`
parsing. In Flight, an `Expires` date with a leap second or an impossible date
now fails to parse, so the cookie is treated as expired (RFC 6265 also rejects
such dates). Could the PR description list these 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]
