alamb commented on code in PR #7865:
URL: https://github.com/apache/arrow-rs/pull/7865#discussion_r2185946272
##########
parquet-variant/src/builder.rs:
##########
@@ -479,20 +572,29 @@ impl VariantBuilder {
self
}
+ // Returns validate_unique_fields because we can no longer reference self
once this method returns.
+ fn parent_state(&mut self) -> (ParentState, bool) {
+ let state = ParentState::Variant {
+ buffer: &mut self.buffer,
+ metadata_builder: &mut self.metadata_builder,
+ };
+ (state, self.validate_unique_fields)
+ }
+
/// Create an [`ListBuilder`] for creating [`Variant::List`] values.
///
/// See the examples on [`VariantBuilder`] for usage.
pub fn new_list(&mut self) -> ListBuilder {
- ListBuilder::new(&mut self.buffer, &mut self.metadata_builder)
- .with_validate_unique_fields(self.validate_unique_fields)
+ let (parent_state, validate_unique_fields) = self.parent_state();
+ ListBuilder::new(parent_state, validate_unique_fields)
Review Comment:
If you want to avoid returning`validate_unique_fields` in a local field you
could do something like
```suggestion
let validate_unique_fields = self.validate_unique_fields;
let parent_state = self.parent_state();
ListBuilder::new(parent_state, validate_unique_fields)
```
##########
parquet-variant/src/builder.rs:
##########
@@ -549,107 +635,92 @@ impl<'a> ListBuilder<'a> {
self
}
- pub fn new_object(&mut self) -> ObjectBuilder {
- self.check_new_offset();
-
- let obj_builder = ObjectBuilder::new(&mut self.buffer,
self.metadata_builder)
- .with_validate_unique_fields(self.validate_unique_fields);
- self.pending = true;
+ // Returns validate_unique_fields because we can no longer reference self
once this method returns.
Review Comment:
same comment as above -- I think you can remove the need to return
`validate_unique_fields` if you want
##########
parquet-variant/src/builder.rs:
##########
@@ -1499,4 +1564,116 @@ mod tests {
let valid_result = valid_obj.finish();
assert!(valid_result.is_ok());
}
+
+ #[test]
+ fn test_variant_builder_to_list_builder_no_finish() {
+ // Create a list builder but never finish it
+ let mut builder = VariantBuilder::new();
+ let _list_builder = builder.new_list();
+
+ builder.append_value(42i8);
+
+ // The original builder should be unchanged
+ let (metadata, value) = builder.finish();
+ let variant = Variant::try_new(&metadata, &value).unwrap();
+ assert_eq!(variant, Variant::Int8(42));
+ }
+
+ #[test]
+ fn test_variant_builder_to_object_builder_no_finish() {
+ // Create an object builder but never finish it
+ let mut builder = VariantBuilder::new();
+ let _object_builder = builder.new_object();
+
+ builder.append_value(42i8);
+
+ // The original builder should be unchanged
+ let (metadata, value) = builder.finish();
+ let variant = Variant::try_new(&metadata, &value).unwrap();
+ assert_eq!(variant, Variant::Int8(42));
+ }
+
+ #[test]
+ fn test_list_builder_to_list_builder_no_finish() {
+ let mut builder = VariantBuilder::new();
+ let mut list_builder = builder.new_list();
+ list_builder.append_value(1i8);
+
+ // Create a nested list builder but never finish it
+ let _nested_list_builder = list_builder.new_list();
+
+ list_builder.append_value(2i8);
+
+ // The parent list should only contain the original values
+ list_builder.finish();
+ let (metadata, value) = builder.finish();
+ let variant = Variant::try_new(&metadata, &value).unwrap();
+ let list = variant.as_list().unwrap();
+ assert_eq!(list.len(), 2);
+ assert_eq!(list.get(0).unwrap(), Variant::Int8(1));
+ assert_eq!(list.get(1).unwrap(), Variant::Int8(2));
+ }
+
+ #[test]
+ fn test_list_builder_to_object_builder_no_finish() {
+ let mut builder = VariantBuilder::new();
+ let mut list_builder = builder.new_list();
+ list_builder.append_value(1i8);
+
+ // Create a nested object builder but never finish it
+ let _nested_object_builder = list_builder.new_object();
+
+ list_builder.append_value(2i8);
+
+ // The parent list should only contain the original values
+ list_builder.finish();
+ let (metadata, value) = builder.finish();
+ let variant = Variant::try_new(&metadata, &value).unwrap();
+ let list = variant.as_list().unwrap();
+ assert_eq!(list.len(), 2);
+ assert_eq!(list.get(0).unwrap(), Variant::Int8(1));
+ assert_eq!(list.get(1).unwrap(), Variant::Int8(2));
+ }
+
+ #[test]
+ fn test_object_builder_to_list_builder_no_finish() {
+ let mut builder = VariantBuilder::new();
+ let mut object_builder = builder.new_object();
+ object_builder.insert("first", 1i8);
+
+ // Create a nested list builder but never finish it
+ let _nested_list_builder = object_builder.new_list("nested");
+
+ object_builder.insert("second", 2i8);
+
+ // The parent object should only contain the original fields
+ object_builder.finish().unwrap();
+ let (metadata, value) = builder.finish();
+ let variant = Variant::try_new(&metadata, &value).unwrap();
+ let obj = variant.as_object().unwrap();
+ assert_eq!(obj.len(), 2);
+ assert_eq!(obj.get("first"), Some(Variant::Int8(1)));
+ assert_eq!(obj.get("second"), Some(Variant::Int8(2)));
+ }
+
+ #[test]
+ fn test_object_builder_to_object_builder_no_finish() {
+ let mut builder = VariantBuilder::new();
+ let mut object_builder = builder.new_object();
+ object_builder.insert("first", 1i8);
+
+ // Create a nested object builder but never finish it
+ let _nested_object_builder = object_builder.new_object("nested");
+
+ object_builder.insert("second", 2i8);
+
+ // The parent object should only contain the original fields
+ object_builder.finish().unwrap();
+ let (metadata, value) = builder.finish();
+ let variant = Variant::try_new(&metadata, &value).unwrap();
+ let obj = variant.as_object().unwrap();
+ assert_eq!(obj.len(), 2);
+ assert_eq!(obj.get("first"), Some(Variant::Int8(1)));
+ assert_eq!(obj.get("second"), Some(Variant::Int8(2)));
Review Comment:
👍
--
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
