ORC-2151: [C++] Move the timestamp compensation before timezone conversion to prevent incorrect DST offset lookup - #2609
Closed
lszskye wants to merge 6 commits into
Closed
ORC-2151: [C++] Move the timestamp compensation before timezone conversion to prevent incorrect DST offset lookup#2609lszskye wants to merge 6 commits into
lszskye wants to merge 6 commits into
Conversation
wgtmac
approved these changes
Apr 22, 2026
wgtmac
left a comment
wgtmac
left a comment
Member
There was a problem hiding this comment.
Thanks for fixing this! Is it possible to add a test case?
dongjoon-hyun
approved these changes
Apr 22, 2026
Contributor
Author
OK, let me add a unit test for this change. |
ffacs
changed the title
Contributor
Author
|
@wgtmac All tests have passed. Please help merge. Thanks! |
wgtmac
reviewed
Apr 27, 2026
| // -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) { |
Contributor
Author
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.
Member
There was a problem hiding this comment.
Yes, it would be great if these can be fixed together.
Member
There was a problem hiding this comment.
As discussed offline, I've merged it to move forward. Thanks @lszskye for fixing this!
wgtmac
pushed a commit
that referenced
this pull request
Apr 29, 2026
…rsion to prevent incorrect DST offset lookup ### 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 Closes #2609 from lszskye/fix_ts. Authored-by: lisizhuo.lsz <lisizhuo.lsz@alibaba-inc.com> Signed-off-by: Gang Wu <ustcwg@gmail.com> (cherry picked from commit 7d302eb) Signed-off-by: Gang Wu <ustcwg@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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