[Variant] Allow appending raw object/list bytes to variant builders #8141
Conversation
alamb
left a comment
There was a problem hiding this comment.
The code in this PR looks great. I think it just needs some tests
Very nice
| } | ||
| /// Macro to generate the match statement for each append_variant, try_append_variant, and | ||
| /// append_variant_bytes -- they each have slightly different handling for object and list handling. | ||
| macro_rules! variant_append_value { |
|
While adding the tests, it turns out that I need the read-only metadata builder. Otherwise, there's no way to add (or copy) individual object fields across -- even if the set of strings is the same, they would get newly allocated field ids in the output variant, potentially in a different order. |
|
@alamb -- merged up and using read-only metadata builders now cc @codephage2020 and @klion26 in case you want to make a review pass. |
klion26
left a comment
There was a problem hiding this comment.
Thanks for the contribution, LGTM
| /// Macro to generate the match statement for each append_variant, try_append_variant, and | ||
| /// append_variant_bytes -- they each have slightly different handling for object and list handling. | ||
| macro_rules! variant_append_value { | ||
| ($builder:expr, $value:expr, $object_pat:pat => $object_arm:expr, $list_pat:pat => $list_arm:expr) => { |
| } | ||
|
|
||
| #[test] | ||
| fn test_append_variant_bytes_round_trip() { |
There was a problem hiding this comment.
Do we need to add a test case for the Variant::List match arm in append_variant_bytes?
| } | ||
|
|
||
| #[test] | ||
| fn test_complex_nested_filtering_injection() { |
alamb
left a comment
There was a problem hiding this comment.
Looks great to me -- thank you @scovich @klion26 and @codephage2020
| /// The caller must ensure that the metadata dictionary is already built and correct for | ||
| /// any objects or lists being appended, but the value's new field name is handled normally. | ||
| /// | ||
| /// # Note |
There was a problem hiding this comment.
Is this note still valid? I think there is no inserting of keys that happens via this API
There was a problem hiding this comment.
This method creates a new variant object field.
The field's value is raw bytes but the field's key is still inserted normally.
So yes, it can still fail if the key is invalid or duplicate.
|
🚀 |
Which issue does this PR close?
Rationale for this change
When working with shredded variants, we need the ability to copy nested object fields and array elements of one variant to a destination. This is a cheap byte-wise copy that relies on the fact that the new variant being built uses the same metadata dictionary as the source variant it is derived from.
What changes are included in this PR?
Define a helper macro that encapsulates the logic for variant appends, now that we have three very similar methods (differing only in their handling of list/object values and their return type).
Add new methods:
ValueBuilder::append_variant_bytes, which is called by new methodsVariantBuilder::append_value_bytes,ListBuilder::append_value_bytes, andObjectBuilder::[try_]insert_bytes.Are these changes tested?
New unit tests
Are there any user-facing changes?
The new methods are public.