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

refactor(parquet): Int96 timestamp column reader #11719

Closed
wants to merge 1 commit into from
Closed

refactor(parquet): Int96 timestamp column reader #11719

wants to merge 1 commit into from

Conversation

zuyu
Copy link
Contributor

@zuyu zuyu commented Dec 2, 2024 •
edited
Loading

Use toPrecision from #8325, which will also be used for Int64 Timestamp.

@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 Dec 2, 2024
@netlify Netlify
Copy link

netlify bot commented Dec 2, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4339b8b
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6753182c97eba90008ea4b33

@zuyu zuyu marked this pull request as ready for review December 2, 2024 23:46
@zuyu zuyu requested a review from majetideepak as a code owner December 2, 2024 23:46
rui-mo
rui-mo reviewed Dec 3, 2024
View reviewed changes
Copy link
Collaborator

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks!

velox/dwio/parquet/reader/TimestampColumnReader.h Outdated Show resolved Hide resolved
@zuyu zuyu force-pushed the int96-ts-refactor branch 3 times, most recently from 7e324fc to 60bffa2 Compare December 4, 2024 04:42
@zuyu zuyu requested review from yingsu00 and rui-mo December 4, 2024 04:42
rui-mo
rui-mo approved these changes Dec 4, 2024
View reviewed changes
Copy link
Collaborator

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good! Just two nits on comments.

velox/dwio/parquet/reader/TimestampColumnReader.h Outdated Show resolved Hide resolved
velox/dwio/parquet/reader/TimestampColumnReader.h Outdated Show resolved Hide resolved
@zuyu zuyu force-pushed the int96-ts-refactor branch 3 times, most recently from 041c2a0 to cbeb1a7 Compare December 5, 2024 18:04
@zuyu
Copy link
Contributor Author

zuyu commented Dec 5, 2024

Encountered flaky test TaskPauseTest.resumeFuture per #11550: https://github.com/facebookincubator/velox/actions/runs/12185448584/job/33991723237?pr=11719

@zation99 zation99 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 Dec 6, 2024
@zuyu zuyu force-pushed the int96-ts-refactor branch from cbeb1a7 to f6b7fd2 Compare December 6, 2024 05:45
@zuyu zuyu force-pushed the int96-ts-refactor branch from f6b7fd2 to 4339b8b Compare December 6, 2024 15:28
@facebook-github-bot
Copy link
Contributor

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

@zuyu zuyu mentioned this pull request Dec 6, 2024
Open
@facebook-github-bot
Copy link
Contributor

@bikramSingh91 merged this pull request in 422fcee.

@zuyu zuyu deleted the int96-ts-refactor branch December 6, 2024 21:44
@zuyu zuyu mentioned this pull request Dec 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rui-mo rui-mo rui-mo approved these changes

@yingsu00 yingsu00 yingsu00 approved these changes

@zation99 zation99 zation99 approved these changes

@Yuhta Yuhta Yuhta approved these changes

@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak is a code owner

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.
6 participants
@zuyu @facebook-github-bot @Yuhta @zation99 @yingsu00 @rui-mo