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