[Variant] Fix broken metadata builder rollback #8135
Conversation
scovich
force-pushed
the
variant-nested-rollback
branch
from
59f9e60 to
27b3cdb
Compare
scovich
changed the title
| /// | ||
| /// This method will panic if the variant contains duplicate field names in objects | ||
| /// when validation is enabled. For a fallible version, use [`ListBuilder::try_with_value`]. | ||
| pub fn with_value<'m, 'd, T: Into<Variant<'m, 'd>>>(mut self, value: T) -> Self { |
parquet-variant/src/variant.rs
Outdated
| impl std::fmt::Debug for Variant<'_, '_> { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Variant::Null => write!(f, "null"), |
There was a problem hiding this comment.
I think for the debug printing we should also include the variant enum, otherwise one wouldn't be able to tell the difference between Variant::Int8(42) and Variant::Int16(42) from the debug output, as they would both be rendered as 42
So perhaps we can keep
| Variant::Null => write!(f, "null"), | |
| Variant::Null => write!(f, "Variant::Null"), |
And then apply some nicer formatting to the binary / object ones
There was a problem hiding this comment.
I pulled out the impl Debug as its own PR that should merge first:
Let's pick up the conversation there as needed, this PR will shrink a lot after rebasing on top of it.
|
FYI @klion26 @abacef and @friendlymatthew as you may be interested in this PR |
| list.finish(); | ||
| } | ||
| } | ||
| if i % skip != 0 { |
There was a problem hiding this comment.
Nice test!
Do we need to make the finish conditions for the inner and outer objects different? so that the field names added by the inner and outer objects are different.
Which issue does this PR close?
Rationale for this change
New unit tests demonstrate that variant builder rollback was broken, producing various validation failures. The problem was subtle -- using buffer length instead of field count when rolling back metadata builder state.
What changes are included in this PR?
Fix the bug, and fix two existing unit tests that expected wrong behavior.
While we're at it, add a human-readable
impl Debug for Variant, which gives a convenient way of comparing two variant values. Also add the missingVariantBuilder::[try_]with_valuemethods that the other two builders already had.Are these changes tested?
New and existing unit tests cover the changes.
Are there any user-facing changes?
Output of
impl Debug for Variantchanged.Two new
VariantBuildermethods.