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 levelComparison to common #11711

Closed

Conversation

majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Dec 2, 2024

Both Parquet reader and writer depend on levelComparison.
This currently resides in parquet/writer/arrow which introduces a dependency of the writer to the reader.
Refactor to parquet common along with renaming variables to Velox coding convention.

Fixes: #11678

@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
Copy link

netlify bot commented Dec 2, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit cf9fe5a
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6752ee77c41cc50008e47919

@majetideepak majetideepak requested a review from pedroerp December 2, 2024 11:16
@majetideepak majetideepak marked this pull request as draft December 2, 2024 16:09
Copy link
Contributor

@pedroerp pedroerp left a comment

Choose a reason for hiding this comment

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

Not sure if it's in draft mode, but looks good to me. Let's just ensure all CI jobs are healthy (some are still running).

@majetideepak majetideepak changed the title refactor(parquet): Move arrow levelComparison and thrift to common refactor(parquet): Move arrow levelComparison to common Dec 2, 2024
@majetideepak majetideepak marked this pull request as ready for review December 2, 2024 17:19
@majetideepak
Copy link
Collaborator Author

I moved it to draft mode to undo the unrelated thrift refactor. The changes now are minimal.

@pedroerp
Copy link
Contributor

pedroerp commented Dec 2, 2024

Alright, let me know once you work through the CI failures.

@majetideepak majetideepak force-pushed the move-arrow-parquet branch 5 times, most recently from 5bc6b8d to e46837c Compare December 3, 2024 21:34
@majetideepak
Copy link
Collaborator Author

majetideepak commented Dec 4, 2024

@pedroerp this is ready, CI is green. Failing job is unrelated.

Copy link
Contributor

@pedroerp pedroerp left a comment

Choose a reason for hiding this comment

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

Thank you @majetideepak

@pedroerp pedroerp 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 4, 2024
@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.

@bikramSingh91
Copy link
Contributor

Struggling with the build system internally to unblock merge, will continue working on it tomorrow.

@bikramSingh91
Copy link
Contributor

@majetideepak do you mind replacing

#include <external/xxhash/xxhash.h>
with

#include <xxhash.h>

in

#include <external/xxhash/xxhash.h>

This would make it conform to how its being included in other places in velox like in ApproxDistinctAggregate.cpp, BinaryFunctions.h, etc.. and also resolves the include automatically.

@majetideepak
Copy link
Collaborator Author

@bikramSingh91 I addressed this. Thanks again for helping with the merge.

@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.

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 merged this pull request in e983aac.

facebook-github-bot pushed a commit that referenced this pull request Dec 18, 2024
Summary:
Follow-up PR: #11711
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

Pull Request resolved: #11795

Reviewed By: DanielHunte

Differential Revision: D67152291

Pulled By: kgpai

fbshipit-source-id: a4a1cc49718e8258cb9e170c44320e04ed3ac581
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
…bator#11711)

Summary:
Both Parquet reader and writer depend on levelComparison.
This currently resides in parquet/writer/arrow which introduces a dependency of the writer to the reader.
Refactor to parquet common along with renaming variables to Velox coding convention.

Fixes: facebookincubator#11678

Pull Request resolved: facebookincubator#11711

Reviewed By: mbasmanova

Differential Revision: D66792888

Pulled By: bikramSingh91

fbshipit-source-id: 88b2cde1eb652762dcf5abc38508fa202aa143b1
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
…incubator#11795)

Summary:
Follow-up PR: facebookincubator#11711
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

Pull Request resolved: facebookincubator#11795

Reviewed By: DanielHunte

Differential Revision: D67152291

Pulled By: kgpai

fbshipit-source-id: a4a1cc49718e8258cb9e170c44320e04ed3ac581
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. 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
Development

Successfully merging this pull request may close these issues.

Move Parquet Arrow LevelComparison to Parquet common.
4 participants