[Parquet] Refactor InMemoryRowGroup to separate CPU and IO #8022
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alamb
commented
Jul 29, 2025
| projection: &ProjectionMask, | ||
| selection: Option<&RowSelection>, | ||
| ) -> Result<()> { | ||
| let FetchRanges { |
There was a problem hiding this comment.
I just broke up fetch into three functions
alamb
force-pushed
the
alamb/split_in_memory_reader
branch
from
671f698 to
c881d34
Compare
alamb
added a commit
to alamb/arrow-rs
that referenced
this pull request
Oct 29, 2025
# Which issue does this PR close? - Part of apache#8000 - closes apache#7983 # Rationale for this change This PR is the first part of separating IO and decode operations in the rust parquet decoder. Decoupling IO and CPU enables several important usecases: 1. Different IO patterns (e.g. not buffer the entire row group at once) 2. Different IO APIs e.g. use io_uring, or OpenDAL, etc. 3. Deliberate prefetching within a file 4. Avoid code duplication between the `ParquetRecordBatchStreamBuilder` and `ParquetRecordBatchReaderBuilder` # What changes are included in this PR? 1. Add new `ParquetDecoderBuilder`, and `ParquetDecoder` and tests It is effectively an explicit version of the state machine that is used in existing async reader (where the state machine is encoded as Rust `async` / `await` structures) # Are these changes tested? Yes -- there are extensive tests for the new code Note that this PR actually adds a **3rd** path for control flow (when I claim this will remove duplication!) In follow on PRs I will convert the existing readers to use this new pattern, similarly to the sequence I did for the metadata decoder: - apache#8080 - apache#8340 Here is a preview of a PR that consolidates the async reader to use the push decoder internally (and removes one duplicate): - apache#8159 - closes apache#8022 # Are there any user-facing changes? Yes, a new API, but now changes to the existing APIs --------- Co-authored-by: Matthijs Brobbel <[email protected]> Co-authored-by: Adrian Garcia Badaracco <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Right now the InMemoryRowGroup does both the IO and the CPU work to on a call to
fetchin #7997 I am trying to separate the two, so that we can have more control over
when IO happens and when CPU work happens. I figured I could refactor the
InMemoryRowGroup as a separate PR to make it easier to review and convince myself this refactor is correct.
What changes are included in this PR?
Refactor
InMemoryRowGroup::fetchinto three functionsAre these changes tested?
By existing CI
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
Are there any user-facing changes?
No