parquet-variant-compute: shred_variant panics on an object with duplicate field names
- Dominant language
- Rust
- Stars
- 3.6k
- Forks
- 1.3k
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 169
Description
# `shred_variant` panics on an object with duplicate field names
## Describe the bug
`shred_variant` (and the whole `shred_variant_with_options` path) panics — rather than
returning `Err` — when an input object contains two fields that resolve to the same
dictionary name. The panic surfaces deep inside `arrow-array`, so it is not obvious that
the cause is malformed variant input.
`Variant::try_new` correctly rejects such an object, but `VariantArray::try_new` only
performs shallow validation of its elements, so a duplicate-field object read from a file
reaches the shredder unchecked.
## To Reproduce
```rust
use arrow::array::{ArrayRef, BinaryViewArray, StructArray};
use arrow::datatypes::{DataType, Field, Fields};
use parquet_variant_compute::{shred_variant, VariantArray};
use std::sync::Arc;
// Metadata dictionary with a single entry: ["a"]
let metadata: &[u8] = &[0x01, 0x01, 0x00, 0x01, b'a'];
// Object with TWO fields, both field-id 0 -> both named "a"
let value: &[u8] = &[
0x02, 0x02, // object header, 2 elements
0x00, 0x00, // field ids: [0, 0]
0x00, 0x02, 0x04, // value offsets
12, 0, // Int8(0)
12, 1, // Int8(1)
];
// Variant::try_new rejects this...
assert!(parquet_variant::Variant::try_new(metadata, value).is_err());
// ...but VariantArray::try_new only shallow-validates, so it reaches the shredder.
let meta_col: ArrayRef = Arc::new(BinaryViewArray::from(vec![Some(metadata)]));
let value_col: ArrayRef = Arc::new(BinaryViewArray::from(vec![Some(value)]));
let sa = StructArray::try_new(
Fields::from(vec![
Field::new("metadata", DataType::BinaryView, false),
Field::new("value", DataType::BinaryView, true),
]),
vec![meta_col, value_col],
None,
).unwrap();
let array = VariantArray::try_new(&sa).unwrap();
let as_type = DataType::Struct(Fields::from(vec![Field::new("a", DataType::Int8, true)]));
let _ = shred_variant(&array, &as_type); // panics
```
Panic:
```
thread '...' panicked at arrow-array/src/array/struct_array.rs:91:
called `Result::unwrap()` on an `Err` value: InvalidArgumentError(
"Incorrect array length for StructArray field \"typed_value\", expected 1 got 2")
```
## Root cause
In `VariantToShreddedObjectVariantRowBuilder::append_value` (`shred_variant.rs`, ~L418), each
object field is routed by name into a per-field builder:
```rust
for (field_name, value) in obj.iter() {
match self.typed_value_builders.get_mut(field_name) {
Some(typed_value_builder) => {
typed_value_builder.append_value(value)?; // called once PER matching field
seen.insert(field_name);
}
None => {
object_builder.insert_bytes(field_name, value);
partially_shredded = true;
}
}
}
```
Two fields named `"a"` cause `append_value` to run twice on that child builder, while the row's
null buffers (`typed_value_nulls`, `nulls`) advance only once at the end. The `"a"` child ends
up with 2 rows against 1 → the length check in `StructArray::try_new` (called from `finish`)
fails and is `unwrap`-ed into a panic.
## Expected behavior
`shred_variant` should return an `InvalidArgumentError`, consistent with `Variant::try_new`
and with the existing collision guard in `unshred_variant` (`unshred_variant.rs:627-631`,
"Field '...' appears in both typed_value and value").
## Proposed fix
The builder already maintains a `seen` set (currently only used for the missing-fields pass).
Promote it to reject duplicates before the append:
```rust
let mut seen = std::collections::HashSet::new();
let mut partially_shredded = false;
for (field_name, value) in obj.iter() {
// A field name must be unique within an object. A duplicate would append twice to one
// typed_value builder while the row's null buffers advance only once, leaving the child
// arrays with mismatched lengths and panicking in `finish`. Reject it as invalid input,
// mirroring the collision check in `unshred_variant`.
if !seen.insert(field_name) {
return Err(ArrowError::InvalidArgumentError(format!(
"duplicate field name '{field_name}' in variant object",
)));
}
match self.typed_value_builders.get_mut(field_name) {
Some(typed_value_builder) => {
typed_value_builder.append_value(value)?;
}
None => {
object_builder.insert_bytes(field_name, value);
partially_shredded = true;
}
}
}
```
The guard runs before the `match`, so a duplicate in the unshredded branch is caught too. The
missing-fields pass below (`!seen.contains(...)`) is unchanged and remains correct. Verified:
the repro then returns `Err("duplicate field name 'a' in variant object")`, the existing suite
passes, and reverting the guard reproduces the exact `struct_array.rs:91` panic.
(Deliberately scoped narrowly to `shred_variant`. Whether `VariantArray::try_new` should fully
validate its elements — which would close a broader class of "bad object reaches a downstream
kernel" issues — is a separate perf/design question.)
## Additional context
Found while adding a `proptest` fuzzing harness to `parquet-variant` in #10352; this defect is
downstream of that PR's scope and not fixed by it. Investigation was assisted by an
agent-driven proptest harness.
Contributor guide
Research direction
Start in shred_variant.rs at VariantToShreddedObjectVariantRowBuilder::append_value, then compare the collision guard in unshred_variant.rs:627-631. Run the existing test suite and reproduce the supplied duplicate-field example. Done means shred_variant returns an InvalidArgumentError for duplicate field names instead of panicking, while existing behavior remains intact.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data-engineering
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 84/100