[Variant] List and object builders have no effect until finalized #7865

scovich · 2025-07-03T23:44:31Z

Which issue does this PR close?

Closes [Variant] If ObjectBuilder::finalize is not called, the resulting Variant object is malformed. #7863
Closes [Variant] Make it harder to forget to finish a pending parent i n ObjectBuilder #7798

Rationale for this change

Reviews and testing on #7843 exposed the fact that creating a variant list or object builder has side effects that leave the parent in an inconsistent/invalid state if the child builder is never finalized. Rework the finalization logic to be more direct so that child builders have no effect on their parents before their finish method is called.

What changes are included in this PR?

Define a new ParentState enum that tracks the necessary information for a child to fully finalize its parent.
Remove the pending machinery from builders

Are these changes tested?

Existing unit tests mostly cover this change.

Added new tests to verify that failing to call finish leaves the parent unmodified.

Are there any user-facing changes?

No.

scovich · 2025-07-04T00:34:42Z

parquet-variant/src/builder.rs

            cur_offset += key.len();
        }
        // Write final offset
        write_offset(&mut metadata, cur_offset, offset_size);


NOTE: We can't use append_offset_array because the final offset isn't known until after we've written all the other offsets. Also, because metadata isn't a ValueBuffer.

friendlymatthew

This is really nice. The pending machinery was definitely too eager.

Plus, the offset handling looks much smoother

alamb

I think this looks great -- thank you @scovich and @friendlymatthew

FYI @viirya

alamb · 2025-07-04T18:56:41Z

parquet-variant/src/builder.rs

+        let obj = variant.as_object().unwrap();
+        assert_eq!(obj.len(), 2);
+        assert_eq!(obj.get("first"), Some(Variant::Int8(1)));
+        assert_eq!(obj.get("second"), Some(Variant::Int8(2)));


alamb · 2025-07-04T19:03:50Z

parquet-variant/src/builder.rs

+        let (parent_state, validate_unique_fields) = self.parent_state();
+        ListBuilder::new(parent_state, validate_unique_fields)


If you want to avoid returningvalidate_unique_fields in a local field you could probably copy it by value with something like

Suggested change

let (parent_state, validate_unique_fields) = self.parent_state();

ListBuilder::new(parent_state, validate_unique_fields)

let validate_unique_fields = self.validate_unique_fields;

let parent_state = self.parent_state();

ListBuilder::new(parent_state, validate_unique_fields)

That's what I was doing at first, but it was extra code duplicated at every call site.

I also tried introducing a ProvidesParentState type trait, that the builder new methods could take &mut impl of... but that also didn't work great because parent_state for object builder takes a param while this one does not.

So yeah... the current PR state is the least bad option I thought of.

alamb · 2025-07-04T19:04:25Z

parquet-variant/src/builder.rs

-        let obj_builder = ObjectBuilder::new(&mut self.buffer, self.metadata_builder)
-            .with_validate_unique_fields(self.validate_unique_fields);
-        self.pending = true;
+    // Returns validate_unique_fields because we can no longer reference self once this method returns.


same comment as above -- I think you can remove the need to return validate_unique_fields if you want

parquet-variant/src/builder.rs

viirya · 2025-07-04T19:26:38Z

parquet-variant/src/builder.rs

-        self.pending = Some((key, field_start));
-
-        obj_builder
+    // Returns validate_unique_fields because we can no longer reference self once this method returns.


Hmm, I didn't run this code, but why cannot reference self after this returns? I don't see parent_state consume self.

Oh, it is mutably borrowed by the parent state.

viirya · 2025-07-04T19:35:19Z

parquet-variant/src/builder.rs

+    }
+
+    #[test]
+    fn test_variant_builder_to_object_builder_no_finish() {


Hmm what the difference before and after this change? Without this PR, this test also passes without issue.

If a builder didn't finish, the next append to its parent would finalize the orphaned pending stuff. For a list, that meant an extra offset (as if there were a zero-sized value there, without even a header). For an object, it would associate two different fields with the same value bytes.

That's a good question, why the tests pass without these fixes tho?

I just tried cherry picking the tests to main branch and all four failed in the expected ways (empty values for lists; extra fields for objects)?

% cargo test --no-fail-fast -- no_finish ... failures: ---- builder::tests::test_list_builder_to_list_builder_no_finish stdout ---- thread 'builder::tests::test_list_builder_to_list_builder_no_finish' panicked at parquet-variant/src/builder.rs:1545:59: called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Received empty bytes") note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- builder::tests::test_list_builder_to_object_builder_no_finish stdout ---- thread 'builder::tests::test_list_builder_to_object_builder_no_finish' panicked at parquet-variant/src/builder.rs:1566:59: called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Received empty bytes") ---- builder::tests::test_object_builder_to_object_builder_no_finish stdout ---- thread 'builder::tests::test_object_builder_to_object_builder_no_finish' panicked at parquet-variant/src/builder.rs:1610:9: assertion `left == right` failed left: 3 right: 2 ---- builder::tests::test_object_builder_to_list_builder_no_finish stdout ---- thread 'builder::tests::test_object_builder_to_list_builder_no_finish' panicked at parquet-variant/src/builder.rs:1589:9: assertion `left == right` failed left: 3 right: 2 failures: builder::tests::test_list_builder_to_list_builder_no_finish builder::tests::test_list_builder_to_object_builder_no_finish builder::tests::test_object_builder_to_list_builder_no_finish builder::tests::test_object_builder_to_object_builder_no_finish test result: FAILED. 2 passed; 4 failed; 0 ignored; 0 measured; 90 filtered out; finished in 0.00s

Oh! You meant the other two tests, that exercise the top-level variant builder:

running 6 tests test builder::tests::test_variant_builder_to_list_builder_no_finish ... ok test builder::tests::test_variant_builder_to_object_builder_no_finish ... ok test builder::tests::test_object_builder_to_object_builder_no_finish ... FAILED test builder::tests::test_list_builder_to_list_builder_no_finish ... FAILED test builder::tests::test_list_builder_to_object_builder_no_finish ... FAILED test builder::tests::test_object_builder_to_list_builder_no_finish ... FAILED

The variant builder didn't have any pending mechanism, and failing to finish its child builder already had no side effects. We just didn't have a test that verified this before.

viirya · 2025-07-04T20:16:58Z

parquet-variant/src/builder.rs


+    /// Finalizes this object and appends it to its parent, which otherwise remains unmodified.
+    pub fn finish(mut self) -> Result<(), ArrowError> {
+        let metadata_builder = self.parent_state.metadata_builder();


Correct me if I miss anything. The only major difference after this change is the metadata builder and buffer of parent builder are pulled into parent state.

But I took at the current codebase, parent_buffer is also only modified in finish. So for this modification, before and after this change has no difference.

For metadata_builder, looks like it is the major change. Now check_pending_field is removed so any pending nested builder won't be updated to metadata builder when we create another new nested builder.

But seems it is only limited to if the pending nested builder hasn't insert any field yet? If we has inserted fields to the pending nested builder, metadata builder is already updated even the pending nested builder is not finalized.

Maybe it is why all un-finialized nested builders in these new "no finish" tests are without any "insert` calls?

If so, this seems not resolve the issue @alamb raised at the beginning? Because I guess the case @alamb concern is like

let mut builder = VariantBuilder::new(); ... { let _object_builder = builder.new_object(); _object_builder.insert(xxx); _object_builder.insert(yyy); _object_builder.insert(zzz); // _object_builder doesn't been finalized. } ... builder.finish();

It's easy enough to add the nested insert(s), but I don't think I understand "the issue" you refer to, that the current code does not fix?

The new code doesn't insert a new object field's name to the metadata dictionary until the builder's finishmethod is called. You are correct, that a scenario like the above could leave extra/unused field names in the metadata dictionary (because the field names were added when the field finished inserting, but then the object that contains those fields was never actually added to its parent). However, spurious names in the metadata dictionary are harmless -- the spec does not require that every metadata entry be referenced as a field id in the corresponding value. In fact, the variant shredding spec requires the metadata dictionary to have "extra" field names, corresponding to fields that were shredded out. That way, a reader who wants to unshred the those fields back to normal variant values doesn't have to rewrite the metadata dictionary.

Hmm... I split the four inner/outer builder tests to not finish exactly one of outer or inner... and the outer not-finish cases all break with "Received empty bytes". So it looks like there IS some problem still.

ohh.... this relates to

[Variant] VariantBuilder (wrongly?) accepts 0+ variant values #7870

If we didn't finish the outer builder, the top-level variant builder didn't contain any value at all, and the resulting value buffer is empty.

If so, this seems not resolve the issue @alamb raised at the beginning? Because I guess the case @alamb concern is like

let mut builder = VariantBuilder::new(); ... { let _object_builder = builder.new_object(); _object_builder.insert(xxx); _object_builder.insert(yyy); _object_builder.insert(zzz); // _object_builder doesn't been finalized. } ... builder.finish();

My initial concern was that it was too easy to forget to call finish and thus lose the structure

Along the way @scovich convinced me that this was acceptable (and actually preferrable) behavior rather than auto completing builders on drop

THe rationale is that there are other ways that drop gets called other than forgetting to call build -- for example an exit on error when using ? where the object probably shouldn't get dropped

I think after this PR the behavior makes much more sense and is now documented

No, I don't argue that we should do finalization in drop. I meant, is this "no effect until finalized" only effective for builders which are added but are called "insert" yet?

Oh, I saw these tests are updated to include insert, looks like they are good now?

The new code doesn't insert a new object field's name to the metadata dictionary until the builder's finishmethod is called.

Doesn't it? I saw that insert already modify the metadata builder. It is happened before finish is called, right?

pub fn insert<'m, 'd, T: Into<Variant<'m, 'd>>>(&mut self, key: &str, value: T) { let metadata_builder = self.parent_state.metadata_builder(); let field_id = metadata_builder.upsert_field_name(key); // <- it is inserted into the metadata builder }

The only difference to previous code, is check_pending_field is removed now so if it is not finished, the field won't be updated to fields and no effect to finish.

scovich · 2025-07-04T20:58:24Z

Hmm, when trying to rebase on top of #7843, I'm getting compilation failures such as:

error[E0509]: cannot move out of type `builder::ListBuilder<'_>`, which implements the `Drop` trait
   --> parquet-variant/src/builder.rs:697:43
    |
697 |         parent_buffer.append_offset_array(self.offsets, Some(data_size), offset_size);
    |                                           ^^^^^^^^^^^^
    |                                           |
    |                                           cannot move out of here
    |                                           move occurs because `self.offsets` has type `std::vec::Vec<usize>`, which does not implement the `Copy` trait

That's not such a nice side effect of the empty impl Drop... but I guess easy enough to solve with std::mem::take.

viirya · 2025-07-05T17:21:49Z

parquet-variant/src/builder.rs

    pub fn insert<'m, 'd, T: Into<Variant<'m, 'd>>>(&mut self, key: &str, value: T) {
-        self.check_pending_field();
+        // Get metadata_builder from parent state
+        let metadata_builder = self.parent_state.metadata_builder();


Hmm, so modification to metadata builder from a nested builder won't affect the parent builder if the nested builder doesn't finalize in the end ?

Seems so, looks like the metadata builder doesn't affect the finalization too much. It is just used to provide unique field id.

alamb · 2025-07-07T18:05:15Z

Thanks again @scovich and @viirya and @friendlymatthew

scovich added 2 commits July 3, 2025 16:30

[Variant] List and object builders have no effect until finalized

fb5d6e3

add tests

c8ba561

github-actions bot added the parquet Changes to the parquet crate label Jul 3, 2025

misc cleanups

850eaca

scovich commented Jul 4, 2025

View reviewed changes

fix

ba6d389

friendlymatthew approved these changes Jul 4, 2025

View reviewed changes

alamb mentioned this pull request Jul 4, 2025

[Variant] Make sure ObjectBuilder and ListBuilder to be finalized before its parent builder #7843

Merged

alamb approved these changes Jul 4, 2025

View reviewed changes

viirya reviewed Jul 4, 2025

View reviewed changes

scovich added 2 commits July 4, 2025 13:35

docs typo

4946d01

Merge remote-tracking branch 'oss/main' into rollback-safe-builders

f82aec8

expanded testing

6f443ec

alamb mentioned this pull request Jul 5, 2025

[Variant] Make it harder to forget to finish a pending parent i n ObjectBuilder #7798

Closed

viirya reviewed Jul 5, 2025

View reviewed changes

viirya approved these changes Jul 5, 2025

View reviewed changes

scovich added 2 commits July 5, 2025 13:30

Merge remote-tracking branch 'oss/main' into rollback-safe-builders

7d5de48

remove stray debugging panic

6eb2b87

alamb merged commit 213d3be into apache:main Jul 7, 2025
12 checks passed

klion26 mentioned this pull request Jul 20, 2025

[Variant] Avoid extra allocation in object builder #7935

Merged

		let (parent_state, validate_unique_fields) = self.parent_state();
		ListBuilder::new(parent_state, validate_unique_fields)

[Variant] List and object builders have no effect until finalized #7865

[Variant] List and object builders have no effect until finalized #7865

Uh oh!

Conversation

scovich commented Jul 3, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friendlymatthew left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scovich Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scovich Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scovich commented Jul 4, 2025

Uh oh!

viirya Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scovich commented Jul 3, 2025 •

edited by alamb

Loading

alamb Jul 4, 2025 •

edited

Loading

viirya Jul 4, 2025 •

edited

Loading

scovich Jul 4, 2025 •

edited

Loading

scovich Jul 4, 2025 •

edited

Loading

viirya Jul 5, 2025 •

edited

Loading

viirya Jul 5, 2025 •

edited

Loading

alamb commented Jul 7, 2025 •

edited

Loading