Repository navigation
GH-54534: [C++] Fix truncation of sub-minute UTC offsets when formatting and parsing zoned timestamps - #54684
ShivanshhhG wants to merge 1 commit into
Conversation
|
|
|
|
| if (m < minutes{10}) | ||
| os << CharT{'0'}; | ||
| os << m.count(); | ||
| if (s != std::chrono::seconds{0}) |
There was a problem hiding this comment.
What if a user calls Arrow’s strftime function with %z? The vendored date library defines %z as +/-hhmm and %Ez as +/-hh:mm, truncating offsets that include seconds. This change instead returns +000921 or +00:09:21. Please avoid changing the format contract and handle sub-minute offsets in the timestamp-to-string cast, for example by formatting these values in UTC as suggested in #54534.
|
|
||
| const std::string GetZone(const std::string& format) { | ||
| // Check for use of %z or %Z | ||
| // Check for use of %z or %Ez |
There was a problem hiding this comment.
%Ez is now recognized here and in StrptimeTimestampParser, but Strftime::Make still checks only %z and %Z when the input has no timezone. This means %Ez is treated as UTC while %z is rejected. Please use the same format check in both paths and add tests for %Ez with and without a timezone.
…p string casting and strftime %Ez When casting zoned timestamps whose UTC offset includes sub-minute seconds (e.g. historical IANA zones like Africa/Monrovia prior to 1972 at UTC-00:44:30 or Europe/Paris prior to 1911), format in UTC (...Z) to preserve the stored instant without violating the %z format contract of whole minutes. This patch: 1. Updates ConvertZoned in scalar_cast_string.cc to format in UTC when the instant's offset contains non-zero seconds (sub-minute offset). 2. Unifies timezone format validation across Strftime and Strptime (GetZone) using FormatHasZone to recognize %Ez alongside %z and %Z, rejecting %Ez when input has no timezone. 3. Updates ParseTimestampISO8601 in value_parsing.h to parse [+-]HHMMSS and [+-]HH:MM:SS zone offsets. 4. Adds tests for %Ez with and without a timezone in scalar_temporal_test.cc, and adds regression tests in scalar_cast_test.cc and value_parsing_test.cc. Closes apache#54534. Signed-off-by: SHIVANSH-ux-ys <singaser78@gmail.com>
cf3dc8c to
48e0b42
Compare
|
Thank you for the detailed feedback @dmitry-chirkov-dremio! I have updated the PR accordingly:
|
Rationale for this change
When formatting zoned timestamps whose UTC offset includes sub-minute seconds (e.g. historical IANA zones such as
Africa/Monroviaprior to 1972 at UTC-00:44:30 orEurope/Parisprior to 1911 at UTC+00:09:21), the vendoreddate.hlibrary truncated the offset to whole minutes viaduration_cast<minutes>, dropping the remaining seconds and emitting an inaccurate offset (e.g.-0044instead of-004430). When parsed back (viaCastor CSV reader), the timestamp instant was shifted by up to 59 seconds.What changes are included in this PR?
cpp/src/arrow/vendored/datetime/date.h: Into_streamfor%zand%Ez, check whether*offset_sechas non-zero seconds (s != 0). If present, format the seconds component (±hhmmssfor%z,±hh:mm:ssfor%Ez). If whole minutes, preserve the standard±hhmm/±hh:mmrepresentation.cpp/src/arrow/util/value_parsing.h: Adddetail::ParseHHMMSSto parse 6-digit offsets and updateParseTimestampISO8601to recognize and parse[+-]HHMMSSand[+-]HH:MM:SSzone offsets.cpp/src/arrow/util/value_parsing.cc&cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc: Recognize%Ezas providing a zone offset inStrptimeTimestampParserandGetZone.cpp/src/arrow/util/value_parsing_test.cc: Add unit tests for sub-minute offsets (+004430,+00:44:30,-004430,-00:44:30) and invalid offset validations (+00:00:60,+000060,+240000,+24:00:00).cpp/src/arrow/compute/kernels/scalar_cast_test.cc: Add regression tests casting zoned timestamps inAfrica/Monrovia(-00:44:30) andEurope/Paris(+00:09:21) to string arrays.Are these changes tested?
Yes, verified with standalone compilation and unit tests covering parsing, formatting, validation bounds, and cast kernels.
Are there any user-facing changes?
Formatting zoned timestamps with historical sub-minute UTC offsets now accurately emits the seconds component (
±HH:MM:SSor±HHMMSS) rather than truncating them to minutes, preserving instant fidelity across round-trips.