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


##########
parquet-variant/src/builder.rs:
##########
@@ -2861,8 +2888,7 @@ mod tests {
         // Only the second attempt should appear in the final variant
         let (metadata, value) = builder.finish();
         let metadata = VariantMetadata::try_new(&metadata).unwrap();
-        assert_eq!(metadata.len(), 1);
-        assert_eq!(&metadata[0], "name"); // not rolled back
+        assert!(metadata.is_empty()); // rolled back

Review Comment:
   nice!



##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
 
     /// Finalizes this list and appends it to its parent, which otherwise 
remains unmodified.
     pub fn finish(mut self) {
-        let data_size = self.buffer.offset();
+        let buffer = self.parent_state.buffer();
+
+        let data_size = buffer.offset() - self.parent_value_offset_base;

Review Comment:
   should we do a checked sub here to avoid underflow? An underflow would only 
happen with a bug in the implementation so this is probably fine



##########
parquet-variant/src/builder.rs:
##########
@@ -1121,16 +1099,27 @@ impl VariantBuilder {
 pub struct ListBuilder<'a> {
     parent_state: ParentState<'a>,
     offsets: Vec<usize>,
-    buffer: ValueBuffer,
+    /// The starting offset in the parent's buffer where this list starts
+    parent_value_offset_base: usize,
+    /// The starting offset in the parent's metadata buffer where this list 
starts
+    /// used to truncate the written fields in `drop` if the current list has 
not been finished
+    parent_metadata_offset_base: usize,

Review Comment:
   this is a good idea



-- 
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