ORC-1151: [C++] Fix ColumnWriter for non-UTC Timestamp columns
#1088
Conversation
| @@ -1837,7 +1837,7 @@ namespace orc { | |||
| // TimestampVectorBatch already stores data in UTC | |||
| int64_t millsUTC = secs[i] * 1000 + nanos[i] / 1000000; | |||
| if (!isUTC) { | |||
| millsUTC = timezone.convertToUTC(millsUTC); | |||
| millsUTC = timezone.convertToUTC(secs[i]) * 1000 + nanos[i] / 1000000; | |||
There was a problem hiding this comment.
Thank you, @noirello . Could you make a test case for this fix?
There was a problem hiding this comment.
Sure, added a UTC and a non UTC test case.
There was a problem hiding this comment.
Please, double check the timestamps in the test cases. Time zones always can be confusing.
There was a problem hiding this comment.
Yes, it's a little counter-intuitive because we only convert secs[i] only. So, Timezone.convertToUTC doesn't care nanos part properly and we should not put it here?
Lines 604 to 606 in f4c7cc1
If then, can we fix it in Timezone.convertToUTC instead? Is there a side-effect?
There was a problem hiding this comment.
cc @wgtmac , @stiga-huang too because this is a correctness issue.
There was a problem hiding this comment.
If we have set nanosecond in the test case WriterTest.writeTimestampWithTimezone, this issue may be fixed earlier.
There was a problem hiding this comment.
Yes, it's a little counter-intuitive because we only convert
secs[i]only. So,Timezone.convertToUTCdoesn't carenanospart properly and we should not put it here?Lines 604 to 606 in f4c7cc1
If then, can we fix it in
Timezone.convertToUTCinstead? Is there a side-effect?
I think the current fix is good enough. As time_t uses second internally, we'd better keep the contract of Timezone.convertToUTC. We may add some comment above the fix to help understanding.
There was a problem hiding this comment.
Got it. +1 for @wgtmac 's final decision.
dongjoon-hyun
changed the title
dongjoon-hyun
changed the title
ColumnWriter for non-UTC Timestamp columns
| EXPECT_EQ(stripeColStats->getUpperBound(), expectedMaxMillis + 1); | ||
| } | ||
|
|
||
| TEST(TestTimestampStatistics, testTimezoneNonUTC) { |
There was a problem hiding this comment.
Thank you for adding this.
There was a problem hiding this comment.
+1, LGTM from my side. It would be great if we can get @wgtmac or @stiga-huang 's final sign-off.
Yes, I think this should be included in the 1.7 branch. |
|
+1 for backporting. |
### What changes were proposed in this pull request? Fix converting non UTC timestamps for statistics. ### Why are the changes needed? Currently, the statistics for timestamp columns are incorrect, when the writer's time zone is not UTC. ### How was this patch tested? Ran the existing test cases. (cherry picked from commit 9042421) Signed-off-by: Dongjoon Hyun <[email protected]>
|
Hi All. I backported this to branch-1.7. |
…e#1088) ### What changes were proposed in this pull request? Fix converting non UTC timestamps for statistics. ### Why are the changes needed? Currently, the statistics for timestamp columns are incorrect, when the writer's time zone is not UTC. ### How was this patch tested? Ran the existing test cases.
What changes were proposed in this pull request?
Fix converting non UTC timestamps for statistics.
Why are the changes needed?
Currently, the statistics for timestamp columns are incorrect, when the writer's time zone is not UTC.
How was this patch tested?
Ran the existing test cases.