Repository navigation
feat: Add array-level shredding entry points on VariantArray - #447
Merged
CurtHagenlocher merged 1 commit intoSep 27, 2026
Merged
Conversation
Add InferShredSchema, Shred, TryShred and Reassemble extension methods so callers holding a VariantArray don't have to decode and rebuild it row by row. Null elements are carried through as SQL-NULL rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Fix the critical binary-array compatibility issue before approval.
Review effort: Lite
Findings: 1
What changed in this PR
Adds array-level shredding, schema inference, reassembly, and reshredding APIs for VariantArray.
Changes:
- Adds
InferShredSchema,Shred,TryShred, andReassemble. - Preserves nulls and supports shared batch layouts.
- Adds tests for inference, slicing, nulls, and reassembly.
| File | Summary |
|---|---|
src/Apache.Arrow.Operations/Shredding/VariantArrayShreddingExtensions.cs |
Implements array-level shredding APIs; has a critical compatibility issue with supported non-BinaryArray value representations. |
test/Apache.Arrow.Operations.Tests/Shredding/VariantArrayShreddingTests.cs |
Adds coverage for the new APIs and edge cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+182
to
+183
| return new ShreddedVariant(schema, array.GetMetadataBytes(index), valueArr, typedValueArr, index) | ||
| .ToVariantValue(); |
Contributor
Author
There was a problem hiding this comment.
Confirmed, but this predates this PR: the casts are in the existing ShreddedVariant / ShreddedObject / ShreddedArray readers, and GetLogicalVariantValue already throws for these storage types (even on an unshredded large_binary column). It's also a little wider than described, since large_utf8 / large_binary typed_value columns hit the same casts. IBinaryArray is internal to Apache.Arrow, so the fix uses small helpers in Operations instead. Filed as #448 and fixed in #449. Once that lands, the entry points in this PR work for these storage types without further changes.
CurtHagenlocher
added a commit
that referenced
this pull request
Sep 27, 2026
…aders (#449) ## What's Changed `VariantArray` accepts `binary`, `large_binary` or `binary_view` for `metadata` and `value` at every nesting level, and `ShredSchema.FromArrowType` maps `large_utf8` / `large_binary` `typed_value` columns. The shredded readers (`ShreddedVariant`, `ShreddedObject`, `ShreddedArray`) cast those columns to `BinaryArray` / `StringArray`, so any other representation threw `InvalidCastException`. This even affected `GetLogicalVariantValue` on unshredded `large_binary` columns. This replaces the casts with two small helpers in `ShreddingHelpers`, `GetBytes` and `GetString`, that read from any binary or string representation. (`IBinaryArray` would have been the natural abstraction, but it's internal to `Apache.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_binary` or `binary_view` (and string/binary `typed_value` columns to `large_utf8` / `large_binary`), and read everything back, both directly and through the `Shred` / `Reassemble` entry points added in #447. All of these tests fail without the fix. Performance: `GetLogicalVariantValue` over 100k rows (BenchmarkDotNet, medium job, two runs) is indistinguishable from the old cast for both shredded and unshredded `binary` columns. An inlinable `BinaryArray` fast path was also tried and measured slower, so it isn't included. Closes #448. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
The shredding APIs only worked one value at a time, so every caller holding a
VariantArrayhad to write the same loops: decode each row (plus a null mask) to shred it, and rebuild row by row to unshred it. This adds extension methods onVariantArrayinVariantArrayShreddingExtensions:Reassemble(allocator = null)converts a shredded array into its unshredded equivalent. Null elements stay null, and an unshredded input is returned unchanged.Shred(schema, allocator = null)shreds into the given layout. It reads logical values, so an already-shredded input is reshredded rather than losing its typed columns.TryShred(options, out shredded, allocator = null)infers a schema and shreds into it. It returns false (withshreddedset to null) when the inferred schema is unshredded.InferShredSchema(options = null)infers a schema from an array. This isn't in the issue's list, but it's needed for the batch scenario the issue describes: a Parquet writer infers once over a representative batch, then callsShred(schema)on every batch so all row groups share one layout.All four go through one shared loop that resolves the column's schema and child arrays once rather than per row. Null elements pass through the
VariantValue?pipeline added in #445.Closes #399.
🤖 Generated with Claude Code