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

[alamb]

• Follow on to [Variant] Check pending before VariantObject::insert #7786

Right now all code paths in ObjectBuilder and ListBuilder need to remember to call check_pending and we already have a bug related to this in #7786

It would be nice if we could get the Rust compiler to ensure this was done rather than relying on remembering to do it.

One idea is with an RAII like struct

I wonder if there is some sort of RAAI mechanism we can use to ensure that the pending field is completed automatically when the child ListBuilder or ObjectBuilder is dropped

That way the compiler can ensure this type of bug is not possible

Something like

/// If this is a builder for a nested object or list, on `Drop` this object will finish the
/// in progress field for the parent
enum PendingParent {
...
}

impl Drop for PendingParent {
 List(...),
Object {
  field_name: &str,
  offset: usize
 }
}

Then we could create a field on the builder like this

struct ObjectBuilder {
...
  pending: Option<PendingParent>,
}

I think the biggest challenge would be sorting out the type lifetimes

Originally posted by @alamb in #7786 (comment)

[abacef]

I dont know how set in stone these builder APIs are but in order to prevent there being unfinished objects being appended to objects for example, we can make new_object return a builder but not actually set the offset, then they would call finish which just returns something like a FinalizedBuilder type, and only then would they be able to call a new method on an ObjectBuilder called like add_object which only takes a FinalizedBuilder and sets the offset and puts it in the parent buffer and does the finalization things. Is there something I may not be considering?

[abacef]

Ok I think I see the issue, you are saying for example you have

let mut variant = VariantBuilder::new();

let mut parent_object_builder = variant.new_object();

let mut child_object_builder1 = parent_object_builder.new_object();
...
let mut child_object_builder2 = parent_object_builder.new_object();
...

and lets say we for got to put self.check_pending_field(); as the first line of ObjectBuilder::new_object(), that would mean child_object_builder1 would get put in the parent_object_builder.pending field. Then when the parent_object_builder.new_object() is called to build child_object_builder2 the parent_object_builder.pending is overwritten with the pending field name of the new field for child object 2 so it never gets self.fields.insert() called on it making the child_object1 not get created correctly/missing, so in the same function scope that it gets overwritten, it gets dropped, so we can just run the check_pending_field it when it gets dropped/overwritten

Seems kind of a workaround, but it may work

[abacef]

If I understood it right, then one of the issues with "ObjectBuilder::insert()" is that it does not update the pending field so it cant run drop() automatically to run the check_pending_field code. One option is to manually set the pending field to None on the insert function, but then we are back where we started since any new method that needs the previous pending value to be completed but does not update the field has to remember to manually update the field.

[alamb]

I think @scovich has a PR that will fix this issue:

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

I have linked it here

[alamb]

Thank you for the comments / questions @abacef -- sorry I hadn't made the connection yet

[alamb]

label_issue.py automatically added labels {'parquet'} from #7786