alamb commented on code in PR #7865:
URL: https://github.com/apache/arrow-rs/pull/7865#discussion_r2187106825


##########
parquet-variant/src/builder.rs:
##########
@@ -668,43 +739,41 @@ impl<'a, 'b> ObjectBuilder<'a, 'b> {
         self
     }
 
-    /// Return a new [`ObjectBuilder`] to add a nested object with the 
specified
-    /// key to the object.
-    pub fn new_object(&mut self, key: &'b str) -> ObjectBuilder {
-        self.check_pending_field();
-
-        let field_start = self.buffer.offset();
-        let obj_builder = ObjectBuilder::new(&mut self.buffer, 
self.metadata_builder)
-            .with_validate_unique_fields(self.validate_unique_fields);
-        self.pending = Some((key, field_start));
-
-        obj_builder
+    // Returns validate_unique_fields because we can no longer reference self 
once this method returns.
+    fn parent_state<'b>(&'b mut self, key: &'b str) -> (ParentState<'b>, bool) 
{
+        let state = ParentState::Object {
+            buffer: &mut self.buffer,
+            metadata_builder: self.parent_state.metadata_builder(),
+            fields: &mut self.fields,
+            field_name: key,
+        };
+        (state, self.validate_unique_fields)
     }
 
-    /// Return a new [`ListBuilder`] to add a list with the specified key to 
the
-    /// object.
-    pub fn new_list(&mut self, key: &'b str) -> ListBuilder {
-        self.check_pending_field();
-
-        let field_start = self.buffer.offset();
-        let list_builder = ListBuilder::new(&mut self.buffer, 
self.metadata_builder)
-            .with_validate_unique_fields(self.validate_unique_fields);
-        self.pending = Some((key, field_start));
-
-        list_builder
+    /// Returns an object builder that can be used to append a new (nested) 
object to this list.
+    ///
+    /// WARNING: The builder will have no effect unless/until 
[`ObjectBuilder::finish`] is called.
+    pub fn new_object<'b>(&'b mut self, key: &'b str) -> ObjectBuilder<'b> {
+        let (parent_state, validate_unique_fields) = self.parent_state(key);
+        ObjectBuilder::new(parent_state, validate_unique_fields)
     }
 
-    /// Finalize object
+    /// Returns a list builder that can be used to append a new (nested) list 
to this list.
     ///
-    /// This consumes self and writes the object to the parent buffer.
-    pub fn finish(mut self) -> Result<(), ArrowError> {
-        self.check_pending_field();
+    /// WARNING: The builder will have no effect unless/until 
[`ListBuilder::finish`] is called.
+    pub fn new_list<'b>(&'b mut self, key: &'b str) -> ListBuilder<'b> {
+        let (parent_state, validate_unique_fields) = self.parent_state(key);
+        ListBuilder::new(parent_state, validate_unique_fields)
+    }
 
+    /// 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();

Review Comment:
   
   > 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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to