sdf-jkl opened a new issue, #10315:
URL: https://github.com/apache/arrow-rs/issues/10315
### Describe the bug
The Variant spec has two distinct requirements on the `value` column of a
variant group:
1. **Presence**: the group must contain a field named `value` (tracked by
#10306, which covers omitting the column entirely).
2. **Annotation**: per [VariantEncoding.md]:
> The `value` field must be annotated as `required` for unshredded
Variant values, or `optional` if parts of the value are shredded as typed
Parquet columns.
This issue tracks a violation of the second requirement:
`VariantArray::from_parts` hardcodes the `value` field as nullable, so kernels
that produce **unshredded** output through it annotate `value` as
nullable/`optional` even though the data never contains an unmasked null there:
| Producer | Output | `value` annotation | Spec |
|---|---|---|---|
| `VariantArrayBuilder::build` | unshredded | non-nullable | ✅ |
| `json_to_variant` / `cast_to_variant` (via builder) | unshredded |
non-nullable | ✅ |
| `unshred_variant` (via `from_parts`) | unshredded | nullable | ❌ must be
`required` |
| `variant_get` with `as_type: None`
(`VariantToBinaryVariantArrowRowBuilder::finish`, via `from_parts`) |
unshredded | nullable | ❌ must be `required` |
| `shred_variant` | shredded | nullable | ✅ (`optional` is legal when
`typed_value` exists) |
Notably, `VariantValueArrayBuilder::append_null` already documents the rule
("It is only valid to call this method when building the `value` field of a
shredded variant column...") — the callers respect it at the data level
(value-nulls only under struct-null rows, i.e. masked), but `from_parts` then
discards that guarantee at the schema level.
Consequences:
- Writing `unshred_variant` output to Parquet produces `optional group v
(VARIANT) { required binary metadata; optional binary value; }`, which is out
of spec for unshredded variants — the annotation-level sibling of #10306.
- Arrow C++'s `VariantExtensionType::IsSupportedStorageType` explicitly
rejects nullable-`value` storage for unshredded variants (`nullable_value` is
one of the invalid cases in `cpp/src/parquet/arrow/variant_test.cc`), so C++
cannot attach the variant extension type to such data.
- `VariantArray::eq` compares the inner `StructArray` including data types,
so a `shred_variant` → `unshred_variant` round trip yields an array that is not
equal to the `VariantArrayBuilder`-produced original purely because of the
annotation.
### To Reproduce
```rust
use arrow_schema::DataType;
use parquet_variant::Variant;
use parquet_variant_compute::{VariantArrayBuilder, shred_variant,
unshred_variant};
let mut builder = VariantArrayBuilder::new(1);
builder.append_variant(Variant::from(42i64));
let original = builder.build();
// spec-correct: the builder annotates `value` as non-nullable
let (idx, field) = original.inner().fields().find("value").unwrap();
assert!(!field.is_nullable());
// round trip through shredding
let shredded = shred_variant(&original, &DataType::Int64).unwrap();
let unshredded = unshred_variant(&shredded).unwrap();
// out of spec: the output is unshredded but `value` is annotated nullable
let (idx, field) = unshredded.inner().fields().find("value").unwrap();
assert!(field.is_nullable());
```
### Expected behavior
Unshredded `VariantArray`s produced by `unshred_variant` and `variant_get`
(binary-variant output path) should annotate the `value` field as non-nullable,
matching `VariantArrayBuilder` and the spec.
Note a blanket fix in `from_parts` (e.g. `nullable = typed_value.is_none()`)
is not correct: `variant_get`'s internal `NotShredded` intermediates are
unshredded arrays whose `value` column legitimately contains unmasked nulls
(rows whose data lived in `typed_value` before the path step, later surfaced as
`Variant::Null` by `try_value`), and `StructArray::new` rejects unmasked nulls
under a non-nullable field. Options:
1. Caller-directed (preferred): the two affected producers build the `value`
field non-nullable; their value-nulls are always masked by the parent null
buffer, which Arrow permits.
2. Data-driven in `from_parts`: annotate non-nullable when
`typed_value.is_none()` and all value-nulls are covered by the parent null
buffer.
3. Normalize annotations at the Parquet write boundary only.
### Additional context
- Presence-level sibling issue: #10306
- Spec discussion around `value` requirements: apache/parquet-format#591
- Arrow C++ variant storage validation (rejects nullable `value`):
`cpp/src/arrow/extension/parquet_variant.cc` (`IsSupportedStorageType`);
shredding support tracked in apache/arrow#45948
[VariantEncoding.md]:
https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#variant-in-parquet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
