ORC-2151: [C++] Move the timestamp compensation before timezone conversion to prevent incorrect DST offset lookup#2609
ORC-2151: [C++] Move the timestamp compensation before timezone conversion to prevent incorrect DST offset lookup#2609lszskye wants to merge 6 commits intoapache:mainfrom
Conversation
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for fixing this! Is it possible to add a test case?
OK, let me add a unit test for this change. |
ffacs
changed the title
|
@wgtmac All tests have passed. Please help merge. Thanks! |
| // -1 BEFORE timezone conversion so that getVariant() sees -5756401 (PDT). | ||
| // If compensation were applied AFTER timezone conversion, getVariant() would | ||
| // see -5756400 (PST), producing a gmtOffset that differs by 3600 seconds. | ||
| TEST_P(WriterTest, DISABLED_writeNegativeTimestampAtDSTBoundary) { |
There was a problem hiding this comment.
Thanks for the feedback! I added DISABLED_ to keep it consistent with the other timestamp-related tests in the same file (DISABLED_writeTimestamp, DISABLED_writeNegativeTimestamp, DISABLED_writeTimestampWithTimezone, DISABLED_writeTimestampInstant), which are all disabled due to [ORC-1976]. Also, I'm happy to remove the DISABLED_ prefix if you prefer — please let me know.
There was a problem hiding this comment.
Yes, it would be great if these can be fixed together.
3 participants
What changes were proposed in this pull request?
Moving the compensation to before the timezone conversion, we ensure that getVariant() always receives the corrected wall-clock time, producing the correct DST offset and an accurate final result.
Why are the changes needed?
Previously, the -1 second compensation for pre-1970 timestamps was applied after the writer-to-reader timezone adjustment. This means writerTimezone.getVariant(writerTime) was called with an uncorrected writerTime that could be 1 second larger than the true value. If this 1-second discrepancy happens to cross a DST (Daylight Saving Time) boundary, the timezone lookup would return an incorrect GMT offset, causing the final timestamp result to be off by the entire DST shift (typically 1 hour) rather than just 1 second.
Was this patch authored or co-authored using generative AI tooling?
No