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

Move LZ4 codec from arrow to velox parquet writer #6909

Closed
wants to merge 1 commit into from
Closed

Move LZ4 codec from arrow to velox parquet writer #6909

wants to merge 1 commit into from

Conversation

nmahadevuni
Copy link
Collaborator

Moves LZ4 codec from arrow to velox parquet writer in continuance of #6890

@netlify
Copy link

netlify bot commented Oct 5, 2023 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 04d9c09
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6528e0477f668000096c7e39

@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 Oct 5, 2023
@majetideepak
Copy link
Collaborator

@karteekmurthys, FYI.

@nmahadevuni nmahadevuni force-pushed the move_lz4_from_arrow_to_velox_parquet branch from d0e06c1 to 9711ddf Compare October 9, 2023 05:48
@nmahadevuni
Copy link
Collaborator Author

@majetideepak @yingsu00 Can you review this?

@nmahadevuni nmahadevuni force-pushed the move_lz4_from_arrow_to_velox_parquet branch from 9711ddf to 8bd29e1 Compare October 9, 2023 07:55
@majetideepak
Copy link
Collaborator

@nmahadevuni I want to get GZIP in first since that is the default Presto option. You will have to rebase after that gets merged. I will update here when that lands.

@majetideepak
Copy link
Collaborator

@nmahadevuni You also need to enable LZ4 in velox/dwio/parquet/writer/arrow/tests/ColumnWriterTest.cpp

yingsu00
yingsu00 approved these changes Oct 11, 2023
View reviewed changes
Copy link
Collaborator

@yingsu00 yingsu00 left a comment

Choose a reason for hiding this comment

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

LGTM

@majetideepak
Copy link
Collaborator

@nmahadevuni can you rebase?

@nmahadevuni nmahadevuni force-pushed the move_lz4_from_arrow_to_velox_parquet branch from 8bd29e1 to 04d9c09 Compare October 13, 2023 06:14
@nmahadevuni
Copy link
Collaborator Author

@majetideepak Rebased it. Please have a look.

@majetideepak
Copy link
Collaborator

@mbasmanova, can you please help merge this? Thanks.

@nmahadevuni
Copy link
Collaborator Author

@mbasmanova @pedroerp Can you please help merge this?

@majetideepak majetideepak requested a review from pedroerp October 19, 2023 11:24
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@pedroerp merged this pull request in c3b9053.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit c3b9053e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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

@yingsu00 yingsu00 yingsu00 approved these changes

@majetideepak majetideepak majetideepak approved these changes

@mbasmanova mbasmanova Awaiting requested review from mbasmanova

@pedroerp pedroerp Awaiting requested review from pedroerp

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nmahadevuni @majetideepak @facebook-github-bot @yingsu00