Repository navigation
feat: Support SQL-NULL rows when shredding variant columns - #445
Conversation
Add Infer and Shred overloads taking IEnumerable<VariantValue?>, where a null entry is a SQL-NULL row (distinct from a present VariantValue.Null). ShreddedVariantArrayBuilder.Build now turns null entries in its rows into null elements of the storage struct. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical API ambiguities and null-row round-trip incompatibility remain unresolved.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
Adds SQL-NULL row support to variant shredding while preserving the distinction from present variant nulls.
Changes:
- Adds nullable schema inference and shredding support.
- Emits top-level validity for null rows.
- Adds null-row behavior tests.
| File | Reviewed change |
|---|---|
test/Apache.Arrow.Operations.Tests/Shredding/ShredNullRowTests.cs |
Tests nullable shredding behavior. |
src/Apache.Arrow.Operations/Shredding/VariantShredder.cs |
Supports nullable rows and shared metadata. |
src/Apache.Arrow.Operations/Shredding/ShredSchemaInferer.cs |
Ignores SQL-NULL rows during inference. |
src/Apache.Arrow.Operations/Shredding/ShreddedVariantArrayBuilder.cs |
Emits validity for null elements. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// <param name="values">The variant values to analyze.</param> | ||
| /// <param name="options">Options controlling depth, frequency, and type consistency thresholds.</param> | ||
| /// <returns>An inferred <see cref="ShredSchema"/>.</returns> | ||
| public ShredSchema Infer(IEnumerable<VariantValue?> values, ShredOptions options = null) |
There was a problem hiding this comment.
Keeping the overload. The only calls that become ambiguous are an untyped null or default literal for values, and those calls could only ever throw ArgumentNullException, so nothing that does useful work is affected. A null in a typed variable, arrays and collection expressions (including [v, null]) all still resolve correctly, and already-compiled callers stay bound to the original method. VariantArray.Builder.AppendRange already uses the same VariantValue / VariantValue? overload pair, so a separate name would be less consistent with the rest of the library. I've updated the PR description to explain this.
| public static (byte[] Metadata, IReadOnlyList<ShredResult> Rows) Shred( | ||
| IEnumerable<VariantValue?> values, | ||
| ShredSchema schema) |
There was a problem hiding this comment.
Keeping the overload. The only calls that become ambiguous are an untyped null or default literal for values, and those calls could only ever throw ArgumentNullException, so nothing that does useful work is affected. A null in a typed variable, arrays and collection expressions (including [v, null]) all still resolve correctly, and already-compiled callers stay bound to the original method. VariantArray.Builder.AppendRange already uses the same VariantValue / VariantValue? overload pair, so a separate name would be less consistent with the rest of the library. I've updated the PR description to explain this.
| for (int i = 0; i < rows.Count; i++) | ||
| { | ||
| VariantValue? row = rows[i]; | ||
| results[i] = row.HasValue ? Shred(row.Value, schema, metadata, idRemap) : null; |
There was a problem hiding this comment.
Good catch, fixed in 9155656. Reconstruct now returns null for a null result instead of throwing. That matches its existing contract, where null already means "no value". A top-level row can't legitimately be missing, so at the top level null can only mean a SQL-NULL row. Added Reconstruct_RoundTripsNullRows, which shreds rows containing both a SQL-NULL row and a present VariantValue.Null and checks that each reconstructs correctly.
The nullable Shred overload returns a null ShredResult for SQL-NULL rows; Reconstruct now returns null for those instead of throwing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
## What's Changed The shredding APIs only worked one value at a time, so every caller holding a `VariantArray` had 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 on `VariantArray` in `VariantArrayShreddingExtensions`: - `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 (with `shredded` set 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 calls `Shred(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](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
What's Changed
The shredding pipeline had no way to represent a SQL-NULL row: every entry point took
VariantValue, andShreddedVariantArrayBuilder.Buildalways produced a storage struct with no validity. Callers had to useVariantValue.Nullas a placeholder, which is stored as a present variant null and changes whatIS NULLmeans.This follows the
VariantValue?convention already used byVariantArray.Builder:ShredSchemaInferer.Infer(IEnumerable<VariantValue?>, ShredOptions)ignores null rows, so they don't count toward frequency thresholds. An all-null column infers as unshredded.VariantShredder.Shred(IEnumerable<VariantValue?>, ShredSchema)leaves null rows out of the shared metadata and returns anullentry for each one.ShreddedVariantArrayBuilder.Buildtreats anullentry inrowsas a null element of the storage struct. Its children are emitted as missing, andmetadatais still populated because that field is required. No validity buffer is allocated when there are no nulls.VariantUnshredder.Reconstructreturnsnullfor anullresult instead of throwing, so rows from the nullableShredoverload can be unshredded one at a time.Existing signatures are unchanged. Passing an untyped
nullordefaultliteral forvalues(e.g.Shred(null, schema)) is now ambiguous between the overloads. Such a call can only throwArgumentNullException, so this is unlikely to matter in practice. Typed null variables, arrays and collection expressions all still resolve as before, andVariantArray.Builder.AppendRangealready uses the same overload pair.This lays the groundwork for the array-level entry points proposed in #399, which can read validity directly from the input
VariantArray.Closes #398.
🤖 Generated with Claude Code