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

Support reading plain encoded INT96 timestamp from Parquet file #10850

Closed
wants to merge 3 commits into from
Closed

Support reading plain encoded INT96 timestamp from Parquet file #10850

wants to merge 3 commits into from

Conversation

rui-mo
Copy link
Collaborator

@rui-mo rui-mo commented Aug 27, 2024 •
edited
Loading

Follow-up for: #4680.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 27, 2024
@netlify Netlify
Copy link

netlify bot commented Aug 27, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 316bf01
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/673164d2db5126000895f227

@rui-mo
Copy link
Collaborator Author

rui-mo commented Aug 27, 2024

Hi @mskapilks, since Gluten customer has been asking for this feature, I created this PR to add support for reading plain encoded INT96 timestamp with your commit kept. If you would like to continue this work in your PR, please let us know. Thanks!

@mskapilks
Copy link

mskapilks commented Aug 27, 2024 •
edited
Loading

Hi @mskapilks, since Gluten customer has been asking for this feature, I created this PR to add support for reading plain encoded INT96 timestamp with your commit kept. If you would like to continue this work in your PR, please let us know. Thanks!

Thanks for the PR
You can continue. I am still looking into INT64 pr

@Yuhta Yuhta self-requested a review August 27, 2024 14:56
@rui-mo rui-mo force-pushed the wip_ts branch 3 times, most recently from a93770b to 07bfce3 Compare September 23, 2024 06:59
@rui-mo rui-mo force-pushed the wip_ts branch 2 times, most recently from 709e15e to b8ef8f2 Compare September 26, 2024 10:02
@Yuhta Yuhta linked an issue Oct 3, 2024 that may be closed by this pull request
Parquet Int96 timestamp support #11160
Open
@rui-mo rui-mo requested a review from majetideepak as a code owner October 10, 2024 06:28
@rui-mo rui-mo requested a review from Yuhta October 18, 2024 08:27
@rui-mo rui-mo force-pushed the wip_ts branch 2 times, most recently from fd26a4a to fdfc09b Compare November 6, 2024 01:45
majetideepak
majetideepak reviewed Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

@rui-mo some more comments.

velox/dwio/parquet/reader/TimestampColumnReader.h Outdated Show resolved Hide resolved
velox/dwio/parquet/reader/TimestampColumnReader.h Outdated Show resolved Hide resolved
velox/dwio/parquet/tests/reader/ParquetTableScanTest.cpp Outdated Show resolved Hide resolved
velox/dwio/parquet/tests/reader/ParquetTableScanTest.cpp Outdated Show resolved Hide resolved
@rui-mo rui-mo force-pushed the wip_ts branch 3 times, most recently from f455d40 to 5f7a1cb Compare November 8, 2024 06:17
majetideepak
majetideepak approved these changes Nov 8, 2024
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

@rui-mo This looks great! Thanks!
@Yuhta do you have any other comments!

@majetideepak majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 8, 2024
@facebook-github-bot
Copy link
Contributor

@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@rui-mo
Copy link
Collaborator Author

rui-mo commented Nov 11, 2024

@kevinwilfong This PR might need to be re-imported before merging. Thanks for your help.

@majetideepak majetideepak mentioned this pull request Nov 11, 2024
Open
@facebook-github-bot
Copy link
Contributor

@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kevinwilfong merged this pull request in da39954.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit da39954c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@rui-mo
Copy link
Collaborator Author

rui-mo commented Nov 13, 2024

@Yuhta @majetideepak Thank you for helping review this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yuhta Yuhta Yuhta left review comments

@majetideepak majetideepak majetideepak approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parquet Int96 timestamp support
5 participants
@rui-mo @mskapilks @facebook-github-bot @Yuhta @majetideepak