sdf-jkl commented on code in PR #10882:
URL: https://github.com/apache/arrow-rs/pull/10882#discussion_r3908745384
##########
parquet-variant/src/builder/metadata.rs:
##########
@@ -101,6 +103,15 @@ impl<'m> ReadOnlyMetadataBuilder<'m> {
impl MetadataBuilder for ReadOnlyMetadataBuilder<'_> {
fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32,
ArrowError> {
+ // Callers that copy fields out of an object and back into the same
metadata dictionary
+ // (unshredding, shredding, and projection all do this) pass field
names that are slices of
+ // the dictionary itself. Those resolve without hashing or any string
comparison, which
+ // matters because this builder is often created per row and so its
`known_field_names`
+ // cache would otherwise be populated and discarded without ever
serving a lookup.
+ if let Some(field_id) = self.metadata.borrowed_field_id(field_name) {
+ return Ok(field_id);
+ }
Review Comment:
This can lead to multiple object fields with the same name which is illegal
for `Variant::Object`
MRE:
```rust
#[test]
fn duplicate_names_with_distinct_ids_bypass_validation() {
// Valid unsorted metadata dictionary: ["a", "a"]
let bytes = [0x01, 0x02, 0x00, 0x01, 0x02, b'a', b'a'];
let metadata = VariantMetadata::try_new(&bytes).unwrap();
let mut values = VariantValueArrayBuilder::new(1);
let mut builder = values.builder_ext(&metadata);
let mut object = builder
.try_new_object()
.unwrap()
.with_validate_unique_fields(true);
object.try_insert(metadata.get(0).unwrap(), 1_i8).unwrap();
// Expected: Err, because both IDs resolve to the object key "a".
// Actual on #10882: Ok, because the builder compares IDs 0 and 1.
assert!(object
.try_insert(metadata.get(1).unwrap(), 2_i8)
.is_err());
}
```
--
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]
