alamb commented on code in PR #8360:
URL: https://github.com/apache/arrow-rs/pull/8360#discussion_r2353358365
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -205,6 +208,155 @@ impl VariantBuilderExt for VariantArrayBuilder {
}
}
+/// A builder for creating only the value column of a [`VariantArray`]
+///
+/// This builder is used when you have existing metadata and only need to build
+/// the value column. It's useful for scenarios like variant unshredding, data
+/// transformation, or filtering where you want to reuse existing metadata.
+///
+/// The builder produces a [`BinaryViewArray`] that can be combined with
existing
Review Comment:
makes sense
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -205,6 +208,155 @@ impl VariantBuilderExt for VariantArrayBuilder {
}
}
+/// A builder for creating only the value column of a [`VariantArray`]
+///
+/// This builder is used when you have existing metadata and only need to build
+/// the value column. It's useful for scenarios like variant unshredding, data
+/// transformation, or filtering where you want to reuse existing metadata.
+///
+/// The builder produces a [`BinaryViewArray`] that can be combined with
existing
+/// metadata to create a complete [`VariantArray`].
+///
+/// # Example:
+/// ```
+/// # use arrow::array::Array;
+/// # use parquet_variant::{Variant};
+/// # use parquet_variant_compute::VariantValueArrayBuilder;
+/// // Create a variant value builder for 10 rows
+/// let mut builder = VariantValueArrayBuilder::new(10);
+///
+/// // Append some values with their corresponding metadata
+/// // In practice, some of the variant values would be objects with internal
metadata.
+/// builder.append_value(Variant::from(42));
+/// builder.append_null();
+/// builder.append_value(Variant::from("hello"));
+///
+/// // Build the final value array
+/// let value_array = builder.build().unwrap();
+/// assert_eq!(value_array.len(), 3);
+/// ```
+#[derive(Debug)]
+pub struct VariantValueArrayBuilder {
+ value_builder: ValueBuilder,
+ value_offsets: Vec<usize>,
+ nulls: NullBufferBuilder,
+}
+
+impl VariantValueArrayBuilder {
+ /// Create a new `VariantValueArrayBuilder` with the specified row capacity
+ pub fn new(row_capacity: usize) -> Self {
+ Self {
+ value_builder: ValueBuilder::new(),
+ value_offsets: Vec::with_capacity(row_capacity),
+ nulls: NullBufferBuilder::new(row_capacity),
+ }
+ }
+
+ /// Build the final value array
+ ///
+ /// Returns a [`BinaryViewArray`] containing the serialized variant values.
+ /// This can be combined with existing metadata to create a complete
[`VariantArray`].
+ pub fn build(mut self) -> Result<BinaryViewArray, ArrowError> {
+ let value_buffer = self.value_builder.into_inner();
+ let mut array = binary_view_array_from_buffers(value_buffer,
self.value_offsets);
+ if let Some(nulls) = self.nulls.finish() {
+ let (views, buffers, _) = array.into_parts();
+ array = BinaryViewArray::try_new(views, buffers, Some(nulls))?;
+ }
+ Ok(array)
+ }
+
+ /// Append a null row to the builder
+ ///
+ /// WARNING: It is only safe to call this method when building the `value`
field of a shredded
Review Comment:
The term "safe" is somewhat overloaded in Rust. Maybe this should be
something more like
```suggestion
/// WARNING: It is only valid to call this method when building the
`value` field of a shredded
```
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -205,6 +208,155 @@ impl VariantBuilderExt for VariantArrayBuilder {
}
}
+/// A builder for creating only the value column of a [`VariantArray`]
+///
+/// This builder is used when you have existing metadata and only need to build
+/// the value column. It's useful for scenarios like variant unshredding, data
+/// transformation, or filtering where you want to reuse existing metadata.
+///
+/// The builder produces a [`BinaryViewArray`] that can be combined with
existing
+/// metadata to create a complete [`VariantArray`].
+///
+/// # Example:
+/// ```
+/// # use arrow::array::Array;
+/// # use parquet_variant::{Variant};
+/// # use parquet_variant_compute::VariantValueArrayBuilder;
+/// // Create a variant value builder for 10 rows
+/// let mut builder = VariantValueArrayBuilder::new(10);
+///
+/// // Append some values with their corresponding metadata
+/// // In practice, some of the variant values would be objects with internal
metadata.
Review Comment:
I think the metadata would be pre-existing (not "internal")?
```suggestion
/// // In practice, some of the variant values would be objects with
pre-existing metadata.
```
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -288,4 +441,46 @@ mod test {
let list = variant.as_list().expect("variant to be a list");
assert_eq!(list.len(), 2);
}
+
+ #[test]
+ fn test_variant_value_array_builder_basic() {
+ let mut builder = VariantValueArrayBuilder::new(10);
+
+ // Append some values
+ builder.append_value(Variant::from(42i32));
+ builder.append_null();
+ builder.append_value(Variant::from("hello"));
+
+ let value_array = builder.build().unwrap();
+ assert_eq!(value_array.len(), 3);
+ }
+
+ #[test]
+ fn test_variant_value_array_builder_with_objects() {
+ // Create metadata with field names
+ let mut metadata_builder = WritableMetadataBuilder::default();
+ metadata_builder.upsert_field_name("name");
+ metadata_builder.upsert_field_name("age");
+ metadata_builder.finish();
+ let metadata_bytes = metadata_builder.into_inner();
+ let metadata = VariantMetadata::try_new(&metadata_bytes).unwrap();
+
+ // Create a variant with an object using the same metadata
+ let mut variant_builder =
VariantBuilder::new().with_metadata(metadata);
+ variant_builder
+ .new_object()
+ .with_field("name", "Alice")
+ .with_field("age", 30i32)
+ .finish();
+ let (_, value_bytes) = variant_builder.finish();
Review Comment:
Can we also test that trying to make an object with a field that is not in
the existing metadata errors?
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -288,4 +441,46 @@ mod test {
let list = variant.as_list().expect("variant to be a list");
assert_eq!(list.len(), 2);
}
+
+ #[test]
+ fn test_variant_value_array_builder_basic() {
+ let mut builder = VariantValueArrayBuilder::new(10);
+
+ // Append some values
+ builder.append_value(Variant::from(42i32));
+ builder.append_null();
+ builder.append_value(Variant::from("hello"));
+
+ let value_array = builder.build().unwrap();
+ assert_eq!(value_array.len(), 3);
+ }
+
+ #[test]
+ fn test_variant_value_array_builder_with_objects() {
+ // Create metadata with field names
+ let mut metadata_builder = WritableMetadataBuilder::default();
+ metadata_builder.upsert_field_name("name");
+ metadata_builder.upsert_field_name("age");
+ metadata_builder.finish();
+ let metadata_bytes = metadata_builder.into_inner();
+ let metadata = VariantMetadata::try_new(&metadata_bytes).unwrap();
+
+ // Create a variant with an object using the same metadata
+ let mut variant_builder =
VariantBuilder::new().with_metadata(metadata);
+ variant_builder
+ .new_object()
+ .with_field("name", "Alice")
+ .with_field("age", 30i32)
+ .finish();
+ let (_, value_bytes) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata_bytes, &value_bytes).unwrap();
+
+ // Now use the value array builder
+ let mut builder = VariantValueArrayBuilder::new(10);
+ builder.append_value(variant);
+ builder.append_null();
+
+ let value_array = builder.build().unwrap();
+ assert_eq!(value_array.len(), 2);
Review Comment:
I think it might be good to verify that you can read the same variantback
out of the array
Something like
```rust
// validate the variant read back from the array is the same
let variant_from_array = Variant::try_new(&metadata_bytes,
value_array.value(0)).unwrap();
assert_eq!(variant_from_array, variant);
```
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -205,6 +208,155 @@ impl VariantBuilderExt for VariantArrayBuilder {
}
}
+/// A builder for creating only the value column of a [`VariantArray`]
+///
+/// This builder is used when you have existing metadata and only need to build
+/// the value column. It's useful for scenarios like variant unshredding, data
+/// transformation, or filtering where you want to reuse existing metadata.
+///
+/// The builder produces a [`BinaryViewArray`] that can be combined with
existing
+/// metadata to create a complete [`VariantArray`].
+///
+/// # Example:
+/// ```
+/// # use arrow::array::Array;
+/// # use parquet_variant::{Variant};
+/// # use parquet_variant_compute::VariantValueArrayBuilder;
+/// // Create a variant value builder for 10 rows
+/// let mut builder = VariantValueArrayBuilder::new(10);
+///
+/// // Append some values with their corresponding metadata
+/// // In practice, some of the variant values would be objects with internal
metadata.
Review Comment:
Eventually it would be nice to show an example of appending a
pre-pre-existing `Variant` value, but I don't think it is necessary here
--
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
