Refactor ipc reading code into methods on ArrayReader
#7006
Conversation
This file contains 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
Jan 21, 2025
| .map(|_| reader.next_buffer()) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| create_primitive_array( | ||
| impl ArrayReader<'_> { |
There was a problem hiding this comment.
This illustrates the point of this PR: remove the need to pass require_alignment as an argument.
It does this by this code into a method on ArrayReader which means require_alignment is now passed as a field.
require_alignment is not a problem, but skip_validation is, for the reasons @tustvold states on #6938 (comment)
| field: &Field, | ||
| variadic_counts: &mut VecDeque<i64>, | ||
| ) -> Result<ArrayRef, ArrowError> { | ||
| let reader = self; |
There was a problem hiding this comment.
I aliased self --> reader to minimize the diff. I can remove this rename as a follow on PR
alamb
commented
Jan 21, 2025
| } | ||
|
|
||
| impl<'a> ArrayReader<'a> { | ||
| /// Create a reader for decoding arrays from an encoded [`RecordBatch`] | ||
| fn try_new( |
There was a problem hiding this comment.
This is refactored code from read_record_batch_impl
| metadata: &MetadataVersion, | ||
| require_alignment: bool, | ||
| ) -> Result<RecordBatch, ArrowError> { | ||
| let buffers = batch.buffers().ok_or_else(|| { |
There was a problem hiding this comment.
the code in this method was split across ArrayReader::try_new and ArrayReader::read_record_batch
alamb
force-pushed
the
alamb/ipc_reader_factor
branch
from
56e9e07 to
81d0146
Compare
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 this PR includes a bunch of whitespace changes that make it look like a much larger change than it is
Viewing the diff by ignoring whitespace makes the structure more clear I think
Which issue does this PR close?
Rationale for this change
@totoroyyb and I are working on adding another flag to the ipc reader code that allows disabling validation during read (see #6938). If the flag is true it will be
unsafe.The current structure of the code as a bunch of free functions makes it impossible to pass the parameter down without having to mark all the inner functions unsafe, which is not correct.
What changes are included in this PR?
This PR refactors the code that currently takes an
ArrayReaderas a parameterand makes it a method on the function. This has the nice property that
we don't have to pass
require_alignmentas a flag anymore and it sets us up very nicelyto be able to add
disable_validationin a follow on PRAre there any user-facing changes?
There are no intended changes in this PR -- all changes are to internal structures only.