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

Push ChunkReader into SerializedPageReader #2463

Closed
tustvold opened this issue Aug 16, 2022 · 0 comments · Fixed by #2464
Closed

Push ChunkReader into SerializedPageReader #2463

tustvold opened this issue Aug 16, 2022 · 0 comments · Fixed by #2464
Assignees
Labels
enhancement Any new improvement worthy of a entry in the changelog parquet Changes to the parquet crate

Comments

@tustvold
Copy link
Contributor

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Unlike SerializedFileReader, or SerializedRowGroupReader, SerializedPageReader is created with a T: Read instead of R: ChunkReader.

In order to preserve this when creating the async reader, I created a separate InMemoryColumnChunkReader.

Similarly when adding page skipping support in #2044 a VecDeque<T> was passed into the constructor, to avoid changing the signature.

Whilst working on #2460 we find ourselves in the same position where the lack of a ChunkReader complicates matters

Describe the solution you'd like

I think we have reached a point where introducing a breaking change to push the ChunkReader into SerializedPageReader is justified, as it will allow unifying InMemoryColumnChunkReader with SerializedPageReader and eliminating a load of additional complexity

Describe alternatives you've considered

Additional context

FYI @Ted-Jiang @thinkharderdev

@tustvold tustvold added the enhancement Any new improvement worthy of a entry in the changelog label Aug 16, 2022
@tustvold tustvold self-assigned this Aug 16, 2022
tustvold added a commit to tustvold/arrow-rs that referenced this issue Aug 16, 2022
@alamb alamb changed the title Push ChunkReader into SerializedPageReader Push ChunkReader into SerializedPageReader Aug 17, 2022
@alamb alamb added the parquet Changes to the parquet crate label Aug 17, 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 a pull request may close this issue.

2 participants