Refactor build_array_reader into a struct
#7521
Merged
+310
−285
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
force-pushed
the
alamb/extract_array_data_builder
branch
from
403a636 to
5f42aac
Compare
alamb
commented
May 16, 2025
alamb
force-pushed
the
alamb/extract_array_data_builder
branch
from
5f42aac to
9690125
Compare
alamb
changed the title
alamb
changed the title
build_array_reader into a struct
alamb
commented
May 16, 2025
| @@ -45,7 +45,7 @@ mod struct_array; | |||
| #[cfg(test)] | |||
| mod test_util; | |||
|
|
|||
| pub use builder::build_array_reader; | |||
| pub(crate) use builder::ArrayReaderBuilder; | |||
There was a problem hiding this comment.
I personally find the use of pub use in non pub modules confusing to reason about what is pub and what is not. I think using pub(crate) to make it explicit this is crate private makes it easier to understand
zhuqi-lucas
approved these changes
May 19, 2025
|
Thanks for the review @zhuqi-lucas |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Note to reviewers
Looking at the whitespace blind diff (https://github.com/apache/arrow-rs/pull/7521/files?w=1) I think makes it easier to see what is changed in this PR (changed the plumbing of how parameters are passed around rather than any logic changes)
Which issue does this PR close?
Rationale for this change
I am trying to avoid potentially decoding arrays twice
Applying
ArrowPredicateis sometimes slower than filtering afterwards. Part of the reason for this is that filter columns are decoded twice.I want a way to inject a pre-calculated filter result into the record batch decoding machinery and one way I found that works well is to provide an
ArrayBuilderinstance that uses the cached result. I found it convenient to have a struct on which to hang the cache rather than a bunch of free functionsYou can see how this is used here:
Also, even if we choose not to go with the result
What changes are included in this PR?
ArrayBuilders into a new structAre there any user-facing changes?
No, this code is entirely internal (e.g. this code is not public: https://docs.rs/parquet/latest/parquet/?search=array_builder)