fix: Read large_binary and binary_view storage in shredded variant readers - #449
Merged
CurtHagenlocher merged 3 commits intoSep 27, 2026
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Updates shredded variant readers to support large binary, binary view, and large UTF-8 storage representations.
Changes:
- Adds representation-independent binary and string helpers.
- Replaces concrete array casts in shredded readers.
- Adds nested coverage for alternate storage types.
| File | Summary |
|---|---|
test/Apache.Arrow.Operations.Tests/Shredding/ShreddedVariantStorageTypeTests.cs |
Tests alternate storage across nesting levels. |
src/Apache.Arrow.Operations/Shredding/ShreddingHelpers.cs |
Adds binary and string representation helpers. |
src/Apache.Arrow.Operations/Shredding/ShreddedVariant.cs |
Uses helpers for residual and typed values. |
src/Apache.Arrow.Operations/Shredding/ShreddedObject.cs |
Supports alternate residual storage. |
src/Apache.Arrow.Operations/Shredding/ShreddedArray.cs |
Supports alternate array residual storage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…aders The shredded readers cast value columns to BinaryArray and string/binary typed_value columns to StringArray/BinaryArray, throwing for the other storage types VariantArray and ShredSchema accept. Read through helpers that handle every binary and string representation instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CurtHagenlocher
force-pushed
the
variant-shredded-binary-types
branch
from
September 27, 2026 16:16
6cacbd3 to
d85b12f
Compare
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
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.
What's Changed
VariantArrayacceptsbinary,large_binaryorbinary_viewformetadataandvalueat every nesting level, andShredSchema.FromArrowTypemapslarge_utf8/large_binarytyped_valuecolumns. The shredded readers (ShreddedVariant,ShreddedObject,ShreddedArray) cast those columns toBinaryArray/StringArray, so any other representation threwInvalidCastException. This even affectedGetLogicalVariantValueon unshreddedlarge_binarycolumns.This replaces the casts with two small helpers in
ShreddingHelpers,GetBytesandGetString, that read from any binary or string representation. (IBinaryArraywould have been the natural abstraction, but it's internal toApache.Arrow.)The new tests shred a column that has residuals at the top level, in a partially shredded object, in an object field, and in a list element. They then convert every binary field, recursively, to
large_binaryorbinary_view(and string/binarytyped_valuecolumns tolarge_utf8/large_binary), and read everything back, both directly and through theShred/Reassembleentry points added in #447. All of these tests fail without the fix.Performance:
GetLogicalVariantValueover 100k rows (BenchmarkDotNet, medium job, two runs) is indistinguishable from the old cast for both shredded and unshreddedbinarycolumns. An inlinableBinaryArrayfast path was also tried and measured slower, so it isn't included.Closes #448.
🤖 Generated with Claude Code