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

Make rle decoder public under experimental feature #1271

Merged
merged 3 commits into from
Feb 6, 2022

Conversation

zeevm
Copy link
Contributor

@zeevm zeevm commented Feb 4, 2022

Closes #1270

Are there any user-facing changes?

'rle' decoder is exposed outside the crate.

Make 'encodings' mod public
Make the 'rle' mod public outside the crate.
@github-actions github-actions bot added the parquet Changes to the parquet crate label Feb 4, 2022
Copy link
Contributor

@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 for the contribution @zeevm

This is fine with me, though I wonder what your expectation is for the stability of the API within the rle module? I don't think it has changed much recently but it may in the upcoming releases.

What would you think if we made it experimental as a signal that we may change it (meaning you would have to enable the experimental arrow feature )

cc @tustvold @nevi-me @sunchao

Copy link
Member

@sunchao sunchao left a comment

Choose a reason for hiding this comment

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

LGTM too, I'd also suggest to consider putting it under experimental flag.

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

I think this would be a perfect fit for what the experimental feature flag was created for (#1032) - namely lower-level APIs that may change, and that we don't want to overburden the average user who "just wants to read a parquet file" with the cognitive load of 👍

FWIW I do hope to at some point do a cleanup of some of the lower-level APIs in the parquet crate, in particular a lot of the trait plumbing currently leaks out, but who knows when I'll find time for that 😅

@@ -18,4 +18,4 @@
pub mod decoding;
pub mod encoding;
pub mod levels;
pub(crate) mod rle;
pub mod rle;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps if you're only interested in rle we could make the above modules pub(crate)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tustvold I'd like to use 'decoding' as well, it makes it really easy to decode RLE encoded pages I get from a column page reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍

@zeevm
Copy link
Contributor Author

zeevm commented Feb 5, 2022

@alamb @sunchao I've changed it to be "experimental" as you suggested.

@zeevm zeevm closed this Feb 5, 2022
@zeevm zeevm reopened this Feb 5, 2022
@zeevm
Copy link
Contributor Author

zeevm commented Feb 5, 2022

What's the process for completing the PR?

@codecov-commenter
Copy link

Codecov Report

Merging #1271 (9c1bb05) into master (fa72873) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1271      +/-   ##
==========================================
- Coverage   83.02%   83.02%   -0.01%     
==========================================
  Files         180      180              
  Lines       52269    52269              
==========================================
- Hits        43398    43396       -2     
- Misses       8871     8873       +2     
Impacted Files Coverage Δ
arrow/src/array/transform/mod.rs 84.51% <0.00%> (-0.26%) ⬇️
parquet_derive/src/parquet_field.rs 66.21% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa72873...9c1bb05. Read the comment docs.

@zeevm
Copy link
Contributor Author

zeevm commented Feb 5, 2022

@tustvold @alamb @sunchao I'm not too versed in github workflow, is there anything else I should do to complete the PR?

@sunchao
Copy link
Member

sunchao commented Feb 6, 2022

LGTM, merging to master.

@sunchao sunchao merged commit ce15d0c into apache:master Feb 6, 2022
@alamb alamb added the enhancement Any new improvement worthy of a entry in the changelog label Feb 16, 2022
@alamb alamb changed the title Make rle decoder public Make rle decoder public under experimental feature Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose Dictionary to reader
5 participants