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.

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

refactor(parquet): Move arrow RleEncodingInternal to common #11795

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wypb
Copy link
Contributor

@wypb wypb commented Dec 9, 2024

Follow-up PR: #11711

Summary:
Both Parquet reader and writer depend on RleEncodingInternal.
This currently resides in parquet/writer/arrow which introduces a dependency of the writer to the reader.
No functional changes.

CC: @majetideepak @pedroerp

@wypb wypb requested a review from majetideepak as a code owner December 9, 2024 09:49
@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 9, 2024
Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit bdfe824
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/675abf8e1aeaa700081f6480

@wypb wypb force-pushed the move_arrow_lib branch 3 times, most recently from 8a43928 to 15e25de Compare December 9, 2024 11:42
@majetideepak
Copy link
Collaborator

@wypb Let's also change the variable names to the Velox coding convention which uses camel case. Thanks.

@wypb wypb force-pushed the move_arrow_lib branch 6 times, most recently from c19fa13 to 06e0896 Compare December 10, 2024 05:48
@wypb
Copy link
Contributor Author

wypb commented Dec 10, 2024

Hi @majetideepak I have made changes according to the comments. Please help me take another look when you have time. Thank you!

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.

@wypb thanks for all the cleanup! I found some comments.

velox/dwio/parquet/common/BitStreamUtilsInternal.h Outdated Show resolved Hide resolved
velox/dwio/parquet/common/BitStreamUtilsInternal.h Outdated Show resolved Hide resolved
velox/dwio/parquet/common/BitStreamUtilsInternal.h Outdated Show resolved Hide resolved
velox/dwio/parquet/common/BitStreamUtilsInternal.h Outdated Show resolved Hide resolved
velox/dwio/parquet/common/BitStreamUtilsInternal.h Outdated Show resolved Hide resolved
velox/dwio/parquet/common/RleEncodingInternal.h Outdated Show resolved Hide resolved
velox/dwio/parquet/common/RleEncodingInternal.h Outdated Show resolved Hide resolved
velox/dwio/parquet/common/RleEncodingInternal.h Outdated Show resolved Hide resolved
velox/dwio/parquet/common/RleEncodingInternal.h Outdated Show resolved Hide resolved
@wypb
Copy link
Contributor Author

wypb commented Dec 10, 2024

Hi @majetideepak Thank you for your review. I have addressed all the comments.

@majetideepak
Copy link
Collaborator

@wypb CI is failing. Can you take a look?

@wypb wypb force-pushed the move_arrow_lib branch 5 times, most recently from 9669b1d to 3a2e249 Compare December 11, 2024 08:56
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.

Thanks, @wypb

@majetideepak
Copy link
Collaborator

@wypb Is the MacOS build failure related?

@wypb
Copy link
Contributor Author

wypb commented Dec 11, 2024

@majetideepak The error is Linking lib/libvelox.dylib. I’m not sure if it’s related to the current PR. I’m still looking into it.

@wypb wypb closed this Dec 12, 2024
@wypb wypb reopened this Dec 12, 2024
@wypb
Copy link
Contributor Author

wypb commented Dec 12, 2024

Hi @majetideepak I checked and found that the MacOS build failure should be related to this modification, but I haven't found the reason yet. Can you help me find the reason?

@wypb
Copy link
Contributor Author

wypb commented Dec 12, 2024

I introduced #include <glog/logging.h> in the Encoding.cpp file to see if it works.

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.

Thanks @wypb. CI seems to be passing now.

@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 Dec 12, 2024
@facebook-github-bot
Copy link
Contributor

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

@wypb
Copy link
Contributor Author

wypb commented Dec 13, 2024

@majetideepak Yes, the BitStreamUtilsInternal.h and RleEncodingInternal.h files in this MR deleted some arrow headers (like logging.h and ubsan.h etc.), which caused the Encoding.cpp file to lack the glog/logging.h dependency, so an error was reported during compilation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. 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
Development

Successfully merging this pull request may close these issues.

3 participants