Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ORC-1151: [C++] Fix ColumnWriter for non-UTC Timestamp columns #1088

Merged
merged 3 commits into from
Apr 19, 2022
Merged

ORC-1151: [C++] Fix ColumnWriter for non-UTC Timestamp columns #1088

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b319e66
Fix converting non UTC timestamps for statistics
noirello Apr 17, 2022
486be68
Add tests for timestamp statistics in different time zones
noirello Apr 17, 2022
254c8c4
Remove unnecessary newline
noirello Apr 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion c++/src/ColumnWriter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @noirello . Could you make a test case for this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, added a UTC and a non UTC test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, double check the timestamps in the test cases. Time zones always can be confusing.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

orc/c++/src/Timezone.cc

Lines 604 to 606 in f4c7cc1

int64_t convertToUTC(int64_t clk) const override {
return clk + getVariant(clk).gmtOffset;
}

If then, can we fix it in Timezone.convertToUTC instead? Is there a side-effect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @wgtmac , @stiga-huang too because this is a correctness issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have set nanosecond in the test case WriterTest.writeTimestampWithTimezone, this issue may be fixed earlier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

orc/c++/src/Timezone.cc

Lines 604 to 606 in f4c7cc1

int64_t convertToUTC(int64_t clk) const override {
return clk + getVariant(clk).gmtOffset;
}

If then, can we fix it in Timezone.convertToUTC instead? 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. +1 for @wgtmac 's final decision.

}
++count;
if (enableBloomFilter) {
Expand Down