[Variant] Improve write API in Variant::Object

[friendlymatthew]

Which issue does this PR close?

• Closes [Variant] Variant::Object can contain two fields with the same field name #7730

Rationale for this change

This commit changes the function name ObjectBuilder::append_value to ObjectBuilder::insert.

Right now, calling insert() with a duplicate key results in two fields with the same key in the object, which deviates from the Variant spec. This PR updates the logic such that the second insert() with a duplicate key will update the value. The old value still exists in the backing buffer, but is unreferenced. One side effect from this approach is a larger variant size.

The Parquet Variant spec states:

Field names are case-sensitive. Field names are required to be unique for each object. It is an error for an object to contain two fields with the same name, whether or not they have distinct dictionary IDs.

[friendlymatthew]

Hi, I wanted to get people's thoughts on the current ObjectBuilder::insert API design.

Right now, calling insert() with a duplicate key results in two fields with the same key in the object, which deviates from the Variant spec. We're considering changing this behavior to either return an Err on the second insert() or to update the existing value-- similar to how std::HashMap::insert works.

From a user standpoint, I'd prefer the latter approach. However, it's a relatively expensive operation. Since each insert() encodes the value directly into the backing buffer, updating a key would require rewriting not just the value for that key, but also everything that comes after it in the buffer.

Worst-case, we'd rewrite our entire fields: BTreeMap<u32, usize> since all of our value's start offsets would have differed.

cc/ @alamb @scovich

[alamb]

Right now, calling insert() with a duplicate key results in two fields with the same key in the object, which deviates from the Variant spec. We're considering changing this behavior to either return an Err on the second insert() or to update the existing value-- similar to how std::HashMap::insert works.

From a user standpoint, I'd prefer the latter approach. However, it's a relatively expensive operation. Since each insert() encodes the value directly into the backing buffer, updating a key would require rewriting not just the value for that key, but also everything that comes after it in the buffer.

I also agree updating the existing value is preferable.

My reading of the Variant spec didn't require all bytes in the variant's value to be used

So what i am saying is I think it would be correct for the VariantBuilder to just update the key and leave the old value there (but not referenced) 🤔 That would result in a larger final variant, but I think as long as we documented this behavior it would be ok from the user perspective (I am envisioning many different possible desired optimizations for variant creation)

[alamb]

this is much nicer

[friendlymatthew]

So what i am saying is I think it would be correct for the VariantBuilder to just update the key and leave the old value there (but not referenced) 🤔 That would result in a larger final variant, but I think as long as we documented this behavior it would be ok from the user perspective (I am envisioning many different possible desired optimizations for variant creation)

I wonder if we can add a gc() method to VariantObject. Similar to a StringView, users can call VariantObject::gc to undergo this expensive operation of pruning stale references, etc...

[scovich]

Right now, calling insert() with a duplicate key results in two fields with the same key in the object, which deviates from the Variant spec. We're considering changing this behavior to either return an Err on the second insert() or to update the existing value-- similar to how std::HashMap::insert works.
From a user standpoint, I'd prefer the latter approach. However, it's a relatively expensive operation. Since each insert() encodes the value directly into the backing buffer, updating a key would require rewriting not just the value for that key, but also everything that comes after it in the buffer.

I also agree updating the existing value is preferable.

My reading of the Variant spec didn't require all bytes in the variant's value to be used

So what i am saying is I think it would be correct for the VariantBuilder to just update the key and leave the old value there (but not referenced) 🤔 That would result in a larger final variant, but I think as long as we documented this behavior it would be ok from the user perspective (I am envisioning many different possible desired optimizations for variant creation)

The above would certainly work, in the sense of producing a valid variant object. My only concern would be that the scenario almost certainly arises due to user error (which is quite different from a generic map or set), and silently tolerating that error isn't necessarily doing the user any favors in the long run. They'll just discover at read time that they lost data, instead of fast-failing at write time. We can probably get away with either approach -- silently replacing or loudly complaining -- I just want to be sure we make the choice intentionally.

[alamb]

I wonder if we can add a gc() method to VariantObject. Similar to a StringView, users can call VariantObject::gc to undergo this expensive operation of pruning stale references, etc...

I think maybe we could just show how to do this with an example (rather than having to make a function) prior to anyone actually encountering the error. I think it could be done by recursively rewriting the entire variant

The above would certainly work, in the sense of producing a valid variant object. My only concern would be that the scenario almost certainly arises due to user error (which is quite different from a generic map or set), and silently tolerating that error isn't necessarily doing the user any favors in the long run. They'll just discover at read time that they lost data, instead of fast-failing at write time. We can probably get away with either approach -- silently replacing or loudly complaining -- I just want to be sure we make the choice intentionally.

Maybe we could have some flag that controls the validation behavior? Something like

let mut builder = VariantBuilder::new();
let mut obj = builder.new_object()
  // specify that an error should be thrown on repeated fields
  .with_validate_unique_fields()
...
obj.finish()?; // this throws error if there were repeated fields

That way people could check for errors programatically if they wanted to and could disable the checking if they didn't care 🤔

This is all for a follow on PR I think

[alamb]

@friendlymatthew -- I think this PR has a logical merge conflct now

Here is a proposed fix:

• Fix logical conflict in Variant write API PR pydantic/arrow-rs#1

[alamb]

close/reopen to rerun CI

[friendlymatthew]

I wonder if we can add a gc() method to VariantObject. Similar to a StringView, users can call VariantObject::gc to undergo this expensive operation of pruning stale references, etc...

I think maybe we could just show how to do this with an example (rather than having to make a function) prior to anyone actually encountering the error. I think it could be done by recursively rewriting the entire variant

The above would certainly work, in the sense of producing a valid variant object. My only concern would be that the scenario almost certainly arises due to user error (which is quite different from a generic map or set), and silently tolerating that error isn't necessarily doing the user any favors in the long run. They'll just discover at read time that they lost data, instead of fast-failing at write time. We can probably get away with either approach -- silently replacing or loudly complaining -- I just want to be sure we make the choice intentionally.

Maybe we could have some flag that controls the validation behavior? Something like

let mut builder = VariantBuilder::new();
let mut obj = builder.new_object()
  // specify that an error should be thrown on repeated fields
  .with_validate_unique_fields()
...
obj.finish()?; // this throws error if there were repeated fields

That way people could check for errors programatically if they wanted to and could disable the checking if they didn't care 🤔

This is all for a follow on PR I think

Opened #7777

[friendlymatthew]

@alamb this has been rebased onto main

[alamb]

Thanks @friendlymatthew and @scovich