This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-11351-4249c2990d7e2764b9379e26b4f0c31aa2cac50a in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit fedfd0dbb888d963e0cbb524430abc5ba73876b4 Author: Abhishek <[email protected]> AuthorDate: Thu Oct 15 18:53:55 2026 +0000 fix(variant): reject duplicate object field names in shred_variant (#11351) # Which issue does this PR close? - Closes #10359. # Rationale for this change When an object repeats a field name, `shred_variant` appends both values to that field's builder. That field's column ends up one row longer than the others, and arrow-array panics. If the duplicate field is not shredded, there is no panic: only the last value is kept and the earlier ones are silently dropped. `Variant::try_new` rejects such objects, but `shred_variant` reads rows with shallow validation only, so they reach the shredder. # What changes are included in this PR? A repeated field name now returns `InvalidArgumentError` ("Duplicate field name: ..."), as the issue requests: - Shredded fields: checked with the existing `seen` scratch buffer. The issue's proposed `HashSet` fix predates #10882, which replaced the per-row set with this buffer. - Unshredded fields: the object builder now validates unique fields, and `insert_bytes` becomes `try_insert_bytes`, so the error is returned instead of panicking. Valid input is unaffected: both checks fire only when a field name repeats within one object. Unshredded repeats are detected by field id, so an object whose dictionary is (invalidly) flagged sorted yet repeats a name still passes through unchanged, as before. Catching it would need a name-based check per row, like the per-row HashSet that #10882 removed for performance. # Are these changes tested? Yes. `test_object_with_duplicate_field_names_errors` covers a duplicate referenced by the same field id and by two ids naming the same string, each with the field shredded and unshredded. The test fails if either half of the fix is removed. # Are there any significant user-facing changes? No API changes. An object with a repeated field name is invalid input; it now returns an error instead of panicking or returning `Ok` with only the last value kept. --- parquet-variant-compute/src/shred_variant.rs | 44 ++++++++++++++++++++++++++-- parquet-variant/src/builder.rs | 4 +-- parquet-variant/src/builder/object.rs | 5 ++-- 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/parquet-variant-compute/src/shred_variant.rs b/parquet-variant-compute/src/shred_variant.rs index c48df074d2..fa7e126933 100644 --- a/parquet-variant-compute/src/shred_variant.rs +++ b/parquet-variant-compute/src/shred_variant.rs @@ -423,16 +423,23 @@ impl<'a> VariantToShreddedObjectVariantRowBuilder<'a> { } = self; seen.fill(false); let mut builder = value_builder.builder_ext(value.metadata()); - let mut object_builder = builder.try_new_object()?; + // Error on a repeated unshredded field instead of overwriting it + let mut object_builder = builder.try_new_object()?.with_validate_unique_fields(true); let mut partially_shredded = false; for (field_name, value) in obj.iter() { match typed_value_builders.get_full_mut(field_name) { Some((index, _, typed_value_builder)) => { + // A repeated field would append a second row to this builder + if seen[index] { + return Err(ArrowError::InvalidArgumentError(format!( + "Duplicate field name: {field_name}" + ))); + } typed_value_builder.append_value(value)?; seen[index] = true; } None => { - object_builder.insert_bytes(field_name, value); + object_builder.try_insert_bytes(field_name, value)?; partially_shredded = true; } } @@ -2352,6 +2359,39 @@ mod tests { Ok(()) } + #[test] + fn test_object_with_duplicate_field_names_errors() -> Result<()> { + // Unsorted dictionaries ["a"] and ["a", "a"]; only an unsorted dictionary may repeat a name + let metadata_a: &[u8] = &[0x01, 1, 0, 1, b'a']; + let metadata_aa: &[u8] = &[0x01, 2, 0, 1, 2, b'a', b'a']; + // {"a": 0i8, "a": 1i8}, via field ids [0, 0] and [0, 1] + let same_ids: &[u8] = &[0x02, 2, 0, 0, 0, 2, 4, 0x0C, 0, 0x0C, 1]; + let distinct_ids: &[u8] = &[0x02, 2, 0, 1, 0, 2, 4, 0x0C, 0, 0x0C, 1]; + + for (metadata, value) in [(metadata_a, same_ids), (metadata_aa, distinct_ids)] { + // Full validation rejects the duplicate, but shredding only validates shallowly + assert!(Variant::try_new(metadata, value).is_err()); + let input = VariantArray::from_parts( + Arc::new(BinaryViewArray::from_iter_values([metadata])), + Arc::new(BinaryViewArray::from_iter_values([value])), + None, + None, + ); + // The duplicate is either shredded ("a") or left unshredded ("b") + for path in ["a", "b"] { + let as_type = ShreddedSchemaBuilder::default() + .with_path(path, &DataType::Int8)? + .build(); + let err = shred_variant(&input, &as_type).unwrap_err(); + assert_eq!( + err.to_string(), + "Invalid argument error: Duplicate field name: a" + ); + } + } + Ok(()) + } + #[test] fn test_uuid_shredding_in_objects() -> Result<()> { let mock_uuid_1 = Uuid::new_v4(); diff --git a/parquet-variant/src/builder.rs b/parquet-variant/src/builder.rs index 2624cafcdb..af13980fdd 100644 --- a/parquet-variant/src/builder.rs +++ b/parquet-variant/src/builder.rs @@ -769,8 +769,8 @@ impl VariantBuilder { /// Enables validation of unique field keys in nested objects. /// /// This setting is propagated to all [`ObjectBuilder`]s created through this [`VariantBuilder`] - /// (including via any [`ListBuilder`]), and causes [`ObjectBuilder::finish()`] to return - /// an error if duplicate keys were inserted. + /// (including via any [`ListBuilder`]); see [`ObjectBuilder::with_validate_unique_fields`] + /// for how a duplicate key is reported. pub fn with_validate_unique_fields(mut self, validate_unique_fields: bool) -> Self { self.validate_unique_fields = validate_unique_fields; self diff --git a/parquet-variant/src/builder/object.rs b/parquet-variant/src/builder/object.rs index 907cc47d00..ca2c8b6624 100644 --- a/parquet-variant/src/builder/object.rs +++ b/parquet-variant/src/builder/object.rs @@ -185,8 +185,9 @@ impl<'a, S: BuilderSpecificState> ObjectBuilder<'a, S> { /// Enables validation for unique field keys when inserting into this object. /// - /// When this is enabled, calling [`ObjectBuilder::finish`] will return an error - /// if any duplicate field keys were added using [`ObjectBuilder::insert`]. + /// When enabled, inserting a duplicate field key fails: the `try_*` methods + /// (e.g. [`ObjectBuilder::try_insert`]) return an error, and the others + /// (e.g. [`ObjectBuilder::insert`]) panic. pub fn with_validate_unique_fields(mut self, validate_unique_fields: bool) -> Self { self.validate_unique_fields = validate_unique_fields; self
