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
|
|
|
|
cf3dc8c to
48e0b42
Compare
|
Thank you for the detailed feedback @dmitry-chirkov-dremio! I have updated the PR accordingly:
|
48e0b42 to
092de95
Compare
dmitry-chirkov-dremio
left a comment
dmitry-chirkov-dremio
left a comment
There was a problem hiding this comment.
I'm ok with the current state.
Update the description - date.h is unchanged now. Values with sub-minute offsets are written in UTC with Z, not with offset seconds.
Ideally I'd like to see round-trip test for all four timestamp units. Cast to string and back to UTC, and check the stored value. The new historical-zone cases only check formatted output at microsecond precision.
| const auto timepoint = sys_time<Duration>(Duration{v}); | ||
| bool subminute_offset = false; | ||
| auto format_zoned_time = [&](auto&& zt) { | ||
| if (ARROW_PREDICT_FALSE(zt.get_info().offset.count() % 60 != 0)) { |
There was a problem hiding this comment.
zt.get_info() formats an abbreviation for fixed-offset zones, and to_stream() calls it again. Can we skip the extra call for OffsetZone, which stores whole minutes?
In local tests, a 1M-row, six-column in-memory CSV export took 0.812 s on parent fb767a7 and 0.954 s on this head (092de95). Skipping only this call for fixed-offset zones brought it back to 0.820 s, with identical CSV output.
There was a problem hiding this comment.
Good catch, thanks! I updated ConvertZoned to branch on OffsetZone (and UTC) upfront so fixed-offset zones take the fast path and avoid the extra get_info() call and allocation entirely.
…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 offset contains non-zero seconds (sub-minute offset), while preserving fast-path formatting for OffsetZone and UTC to avoid redundant get_info() formatting calls. 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 round-trip regression tests across all timestamp units (s, ms, us, ns) in scalar_cast_test.cc and value_parsing_test.cc. Closes apache#54534. Signed-off-by: SHIVANSH-ux-ys <singaser78@gmail.com>
092de95 to
ab97171
Compare
|
I have pushed an update addressing the review feedback:
|
|
|
dmitry-chirkov-dremio
left a comment
dmitry-chirkov-dremio
left a comment
There was a problem hiding this comment.
LGTM. Thank you addressing the comments.
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), formatting with%zwould truncate the offset to whole minutes, dropping remaining seconds and causing instant drift. To preserve instant fidelity without violating the%z/%Ezformatting contract (whole minutes±HHMM/±HH:MM), timestamps with sub-minute offsets are formatted in UTC (...Z) during timestamp-to-string cast. Additionally,ParseTimestampISO8601is updated to support parsing 6-digit sub-minute offsets ([+-]HHMMSSand[+-]HH:MM:SS), and%Ezis recognized acrossStrftimeandGetZone.What changes are included in this PR?
cpp/src/arrow/compute/kernels/scalar_cast_string.cc: InConvertZoned, if a timestamp instant has non-zero sub-minute seconds (offset % 60 != 0), format it in UTC (...Z) to preserve the exact stored instant without violating the%zminute contract. Fixed-offset zones (OffsetZone) and UTC take the fast path without redundantget_info()calls.cpp/src/arrow/util/value_parsing.h: Adddetail::ParseHHMMSSand 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: Unify zone format validation inStrftime::MakeandGetZoneviaFormatHasZoneto recognize%Ezalongside%zand%Z, properly rejecting%Ezwhen the input timestamp has no timezone.cpp/src/arrow/compute/kernels/scalar_cast_test.cc: Add round-trip tests across all four timestamp units (s,ms,us,ns) for historical sub-minute zones (Africa/MonroviaandEurope/Paris), verifying cast to string and back to UTC matches the exact stored value.cpp/src/arrow/compute/kernels/scalar_temporal_test.cc&cpp/src/arrow/util/value_parsing_test.cc: Add unit tests for%Ez(with and without timezones) and sub-minute offset parsing/validation bounds.Are these changes tested?
Yes, covered by unit tests in
scalar_cast_test.cc,scalar_temporal_test.cc, andvalue_parsing_test.cc.Are there any user-facing changes?
...Z), preventing truncation errors and preserving round-trip instant accuracy.[+-]HHMMSSand[+-]HH:MM:SS).strftimenow recognizes%Ezand validates timezone presence identically to%zand%Z.