Skip to content

[Variant] Add (empty) parquet-variant crate, update parquet-testing pin #7485

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

Merged
merged 2 commits into from
May 11, 2025

Conversation

alamb
Copy link
Contributor

@alamb alamb commented May 8, 2025

Which issue does this PR close?

Rationale for this change

We are adding support for Variant and thus we should add a new crate where the new code will live and that clearly explains the current status.

What changes are included in this PR?

  1. Add a new parquet-variant crate
  2. Add README
  3. Update parquet-testing submodule to include variant test data added in Add example binary variant data and regeneration scripts parquet-testing#76

Are there any user-facing changes?

Not yet

@alamb alamb changed the title Alamb/crate variant crate [Variant] Add (empty) parquet-variant crate May 8, 2025
@alamb alamb force-pushed the alamb/crate_variant_crate branch from 0681190 to cdab392 Compare May 8, 2025 19:26
@alamb alamb marked this pull request as draft May 8, 2025 19:26
@alamb alamb changed the title [Variant] Add (empty) parquet-variant crate [Variant] Add (empty) parquet-variant crate, update parquet-testing pin May 8, 2025
@alamb alamb marked this pull request as ready for review May 8, 2025 19:30
@alamb
Copy link
Contributor Author

alamb commented May 8, 2025

@crepererum or @wjones127 or @mbrobbel I wonder if you have a few minutes to review this PR. I hope it is non controversial.

cc @Weijun-H @tustvold @etseidl @PinkCrow007

Copy link
Member

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Looks good, though I think the readme description could be confusing to some as is.

Cargo.toml Outdated
Comment on lines 103 to 104
strip = false No newline at end of file
strip = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like an unnecessary change

Copy link
Contributor Author

@alamb alamb May 11, 2025

Choose a reason for hiding this comment

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

I think it is something my editor does by default.

Copy link
Contributor Author

@alamb alamb 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 @wjones127, @mbrobbel and @adriangb for the reviews

@alamb
Copy link
Contributor Author

alamb commented May 9, 2025

I just found that the parquet-java implementation also has something called parquet-variant:

https://github.com/apache/parquet-java/tree/master/parquet-variant/src

Which was added a few days ago with @gene-db:

@alamb
Copy link
Contributor Author

alamb commented May 11, 2025

Ok, let's merge this to keep the code flowing. I tried to address @wjones127 's comment, but if it is still confusing I would be more than happy to make a follow on PR to further clarify

@alamb alamb merged commit 016add1 into apache:main May 11, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants