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-663: [C++] Support timestamp statistics with nanosecond #543

Merged
merged 4 commits into from
Oct 30, 2020
Merged

ORC-663: [C++] Support timestamp statistics with nanosecond #543

merged 4 commits into from
Oct 30, 2020

Conversation

wgtmac
Copy link
Member

@wgtmac wgtmac commented Sep 8, 2020

What changes were proposed in this pull request?

  1. Store nanosecond into TimestampColumnStatistics in C++ writer.
  2. Be aware of nanosecond in the TimestampColumnStatistics.
  3. Adde new functions in TimestampColumnStatistics to get nanosecond while keeping backward compatibility.
  4. Fix PPD to utilize nanosecond or use a default value for timestamp columns.

Why are the changes needed?

To be consistent with the java side.

How was this patch tested?

Several new unit tests have been added to TestPredicateLeaf.cc, TestColumnStatistics.cc, and TestSearchArgument.cc.

@wgtmac wgtmac force-pushed the ORC-611 branch 2 times, most recently from 476bced to 597639c Compare September 8, 2020 13:57
@wgtmac wgtmac mentioned this pull request Sep 8, 2020
Closed
@pgaref
Copy link
Contributor

pgaref commented Sep 8, 2020

Hey @wgtmac thanks for taking care of this.
Would it make sense to create a separate child-ticket for the C++ fix? ORC-611 is already merged and could cause confusion

@wgtmac wgtmac changed the title ORC-611: [C++] Support timestamp statistics with nanosecond ORC-663: [C++] Support timestamp statistics with nanosecond Sep 8, 2020
@wgtmac
Copy link
Member Author

wgtmac commented Sep 8, 2020 •
edited
Loading

Hey @wgtmac thanks for taking care of this.
Would it make sense to create a separate child-ticket for the C++ fix? ORC-611 is already merged and could cause confusion

Thanks for the suggestion. @pgaref Please review this patch if you get the chance. Thanks!

@wgtmac wgtmac requested review from omalley and xndai September 8, 2020 14:35
@wgtmac
Copy link
Member Author

wgtmac commented Oct 23, 2020

@dongjoon-hyun Can you take a look at this? Thanks!

@dongjoon-hyun
Copy link
Member

Thank you for pinging me, @wgtmac . Sure. I'll review this tomorrow.

@dongjoon-hyun dongjoon-hyun self-assigned this Oct 23, 2020
@dongjoon-hyun dongjoon-hyun self-requested a review October 23, 2020 21:53
@dongjoon-hyun dongjoon-hyun removed their assignment Oct 23, 2020
dongjoon-hyun
dongjoon-hyun reviewed Oct 25, 2020
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Hi, @wgtmac . I left a few questions and comments. Could you reply on them, please?

wgtmac
wgtmac commented Oct 27, 2020
View reviewed changes
Copy link
Member Author

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

Thanks @dongjoon-hyun ! I have changed the code according to your feedback, please review it again.

c++/include/orc/sargs/Literal.hh Show resolved Hide resolved
c++/include/orc/Statistics.hh Outdated Show resolved Hide resolved
c++/include/orc/Statistics.hh Outdated Show resolved Hide resolved
c++/src/Statistics.cc Outdated Show resolved Hide resolved
c++/src/Statistics.cc Show resolved Hide resolved
c++/src/Statistics.hh Outdated Show resolved Hide resolved
c++/src/Statistics.hh Outdated Show resolved Hide resolved
c++/src/Statistics.hh Outdated Show resolved Hide resolved
c++/src/sargs/PredicateLeaf.cc Outdated Show resolved Hide resolved
c++/test/TestPredicateLeaf.cc Show resolved Hide resolved
@dongjoon-hyun
Copy link
Member

Thank you for update. Sure, I'll review this tonight~

@dongjoon-hyun
Copy link
Member

BTW, @wgtmac . I fixed the AppVeyor failure in the master branch. Could you rebase this PR to the master branch? I hope to see AppVeyor result because this is C++ PR.

@dongjoon-hyun
Copy link
Member

Hi, @wgtmac
Could you update once more please?

@wgtmac
Copy link
Member Author

wgtmac commented Oct 30, 2020

Hi, @wgtmac
Could you update once more please?

Done. Please check it again. Thanks!

@dongjoon-hyun
Copy link
Member

Thank you! I'll review today.

dongjoon-hyun
dongjoon-hyun approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @wgtmac !

@dongjoon-hyun dongjoon-hyun merged commit 248e78a into apache:master Oct 30, 2020
@wgtmac wgtmac deleted the ORC-611 branch February 23, 2022 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

@omalley omalley Awaiting requested review from omalley

@xndai xndai Awaiting requested review from xndai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wgtmac @pgaref @dongjoon-hyun