viirya commented on code in PR #7865: URL: https://github.com/apache/arrow-rs/pull/7865#discussion_r2186071888
########## 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: 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 `insert`ed 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(); ``` -- 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