You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by gi...@apache.org on 2026/10/15 22:22:09 UTC
(arrow-rs) branch gh-readonly-queue/main/pr-11351-4249c2990d7e2764b9379e26b4f0c31aa2cac50a created (now fedfd0dbb8)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-11351-4249c2990d7e2764b9379e26b4f0c31aa2cac50a
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
at fedfd0dbb8 fix(variant): reject duplicate object field names in shred_variant (#11351)
This branch includes the following new commits:
new fedfd0dbb8 fix(variant): reject duplicate object field names in shred_variant (#11351)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(arrow-rs) 01/01: fix(variant): reject duplicate object field names in shred_variant (#11351)
Posted by gi...@apache.org.
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 <16...@users.noreply.github.com>
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