apache / apache/arrow-rs

parquet-variant-compute: shred_variant panics on an object with duplicate field names

Open Beginner friendly
#10,359 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.