fix(ipc): replace wildcard in skip_field with explicit DataType handling - #9822
Merged
Conversation
Contributor
Author
|
cc @alamb |
alamb
approved these changes
Apr 25, 2026
Contributor
Author
|
thank you @alamb |
Contributor
|
Thank you! |
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
…ing (apache#9822) # Which issue does this PR close? - Closes apache#9821 . # Rationale for this change `skip_field` currently uses a wildcard match (`_`) to skip remaining `DataType` variants by assuming a two-buffer layout. This is not robust, as new variants may have different buffer layouts, leading to incorrect skipping and buffer misalignment. Replacing the wildcard with explicit handling ensures the correct number of buffers are skipped and avoids silent errors for future types. # What changes are included in this PR? * Removed the wildcard (`_`) match arm in `skip_field` * Added explicit handling for all remaining fixed-width and boolean `DataType` variants * Each of these types now explicitly skips: * null buffer * values buffer File updated: * `arrow-ipc/src/reader.rs` # Are these changes tested? Yes. * Added test: `test_projection_skip_fixed_width_types` in `arrow-ipc/src/reader.rs` * The test iterates over all fixed-width and boolean `DataType` variants covered by this change * For each type: * writes a batch with `[skipped_column(type), values_column(Int32)]` * reads only the second column (skipping the first) * verifies whether the returned column exactly matches the original `Int32` values * This directly validates that skipping each of these types consumes the correct number of buffers * Also, all existing `arrow-ipc` tests pass (`cargo test -p arrow-ipc --lib`) # Are there any user-facing changes? No.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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?
skip_fieldwith explicitDataTypehandling #9821 .Rationale for this change
skip_fieldcurrently uses a wildcard match (_) to skip remainingDataTypevariants by assuming a two-buffer layout. This is not robust, as new variants may have different buffer layouts, leading to incorrect skipping and buffer misalignment.Replacing the wildcard with explicit handling ensures the correct number of buffers are skipped and avoids silent errors for future types.
What changes are included in this PR?
Removed the wildcard (
_) match arm inskip_fieldAdded explicit handling for all remaining fixed-width and boolean
DataTypevariantsEach of these types now explicitly skips:
File updated:
arrow-ipc/src/reader.rsAre these changes tested?
Yes.
Added test:
test_projection_skip_fixed_width_typesinarrow-ipc/src/reader.rsThe test iterates over all fixed-width and boolean
DataTypevariants covered by this changeFor each type:
[skipped_column(type), values_column(Int32)]Int32valuesThis directly validates that skipping each of these types consumes the correct number of buffers
Also, all existing
arrow-ipctests pass (cargo test -p arrow-ipc --lib)Are there any user-facing changes?
No.