cetra3 opened a new issue, #10359:
URL: https://github.com/apache/arrow-rs/issues/10359
# `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.
--
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]
