From 498c18a121065f46bf24c1f809404c23a9f640e7 Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Sun, 24 May 2026 21:17:14 -0400 Subject: [PATCH 1/8] Add VariantBuilder values check --- .../benches/variant_kernels.rs | 2 +- .../src/cast_to_variant.rs | 32 ++--- parquet-variant-compute/src/from_json.rs | 6 +- parquet-variant-compute/src/shred_variant.rs | 2 +- parquet-variant-compute/src/variant_get.rs | 32 ++--- parquet-variant-json/src/from_json.rs | 24 ++-- parquet-variant-json/src/to_json.rs | 22 +-- parquet-variant/benches/variant_builder.rs | 18 +-- parquet-variant/benches/variant_validation.rs | 6 +- parquet-variant/src/builder.rs | 135 ++++++++++++------ parquet-variant/src/builder/list.rs | 24 ++-- parquet-variant/src/builder/metadata.rs | 18 +-- parquet-variant/src/builder/object.rs | 37 ++--- parquet-variant/src/variant.rs | 12 +- parquet-variant/src/variant/list.rs | 8 +- parquet-variant/src/variant/metadata.rs | 4 +- parquet-variant/src/variant/object.rs | 30 ++-- parquet-variant/tests/variant_interop.rs | 17 +-- 18 files changed, 244 insertions(+), 185 deletions(-) diff --git a/parquet-variant-compute/benches/variant_kernels.rs b/parquet-variant-compute/benches/variant_kernels.rs index 383697ab8cc6..348bd4b7d69d 100644 --- a/parquet-variant-compute/benches/variant_kernels.rs +++ b/parquet-variant-compute/benches/variant_kernels.rs @@ -187,7 +187,7 @@ fn create_primitive_variant_array(size: usize) -> VariantArray { for _ in 0..size { let mut builder = VariantBuilder::new(); builder.append_value(rng.random::()); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); variant_builder.append_variant(Variant::try_new(&metadata, &value).unwrap()); } diff --git a/parquet-variant-compute/src/cast_to_variant.rs b/parquet-variant-compute/src/cast_to_variant.rs index 1b26ffe07d2a..3789fc143dc1 100644 --- a/parquet-variant-compute/src/cast_to_variant.rs +++ b/parquet-variant-compute/src/cast_to_variant.rs @@ -1342,7 +1342,7 @@ mod tests { list.append_value(1); list.append_value(2); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant = Variant::new(&metadata, &value); @@ -1367,7 +1367,7 @@ mod tests { list.append_value(4); list.append_value(5); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant = Variant::new(&metadata, &value); @@ -1388,7 +1388,7 @@ mod tests { list.append_value(1i64); list.append_value(2i64); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant = Variant::new(&metadata, &value); @@ -1413,7 +1413,7 @@ mod tests { list.append_value(4i64); list.append_value(5i64); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant = Variant::new(&metadata, &value); @@ -1441,7 +1441,7 @@ mod tests { list.append_null(); list.append_value(2i32); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant0 = Variant::new(&metadata, &value); @@ -1451,7 +1451,7 @@ mod tests { list.append_value(3i32); list.append_value(4i32); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant1 = Variant::new(&metadata, &value); @@ -1461,7 +1461,7 @@ mod tests { list.append_null(); list.append_null(); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant3 = Variant::new(&metadata, &value); @@ -1487,7 +1487,7 @@ mod tests { list.append_value(3i32); list.append_null(); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant = Variant::new(&metadata, &value); @@ -1515,7 +1515,7 @@ mod tests { list.append_null(); list.append_value(2i64); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant0 = Variant::new(&metadata, &value); @@ -1525,7 +1525,7 @@ mod tests { list.append_value(3i64); list.append_value(4i64); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant1 = Variant::new(&metadata, &value); @@ -1535,7 +1535,7 @@ mod tests { list.append_null(); list.append_null(); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant3 = Variant::new(&metadata, &value); @@ -1561,7 +1561,7 @@ mod tests { list.append_value(3i64); list.append_null(); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant = Variant::new(&metadata, &value); @@ -1598,7 +1598,7 @@ mod tests { list.append_value(0i32); list.append_value(1i32); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant0 = Variant::new(&metadata, &value); @@ -1608,7 +1608,7 @@ mod tests { list.append_null(); list.append_value(3i32); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant1 = Variant::new(&metadata, &value); @@ -1618,7 +1618,7 @@ mod tests { list.append_null(); list.append_null(); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant3 = Variant::new(&metadata, &value); @@ -1653,7 +1653,7 @@ mod tests { list.append_null(); list.append_value(3i64); list.finish(); - builder.finish() + builder.finish().unwrap() }; let variant = Variant::new(&metadata, &value); diff --git a/parquet-variant-compute/src/from_json.rs b/parquet-variant-compute/src/from_json.rs index 0983147132a2..1a20231aa057 100644 --- a/parquet-variant-compute/src/from_json.rs +++ b/parquet-variant-compute/src/from_json.rs @@ -101,7 +101,7 @@ mod test { let mut ob = vb.new_object(); ob.insert("a", Variant::Int8(32)); ob.finish(); - let (object_metadata, object_value) = vb.finish(); + let (object_metadata, object_value) = vb.finish().unwrap(); let expected = Variant::new(&object_metadata, &object_value); assert_eq!(variant_array.value(2), expected); } @@ -150,7 +150,7 @@ mod test { let mut ob = vb.new_object(); ob.insert("a", Variant::Int8(32)); ob.finish(); - let (object_metadata, object_value) = vb.finish(); + let (object_metadata, object_value) = vb.finish().unwrap(); let expected = Variant::new(&object_metadata, &object_value); assert_eq!(variant_array.value(2), expected); } @@ -199,7 +199,7 @@ mod test { let mut ob = vb.new_object(); ob.insert("a", Variant::Int8(32)); ob.finish(); - let (object_metadata, object_value) = vb.finish(); + let (object_metadata, object_value) = vb.finish().unwrap(); let expected = Variant::new(&object_metadata, &object_value); assert_eq!(variant_array.value(2), expected); } diff --git a/parquet-variant-compute/src/shred_variant.rs b/parquet-variant-compute/src/shred_variant.rs index 440f4b716521..1f275cef50bb 100644 --- a/parquet-variant-compute/src/shred_variant.rs +++ b/parquet-variant-compute/src/shred_variant.rs @@ -2105,7 +2105,7 @@ mod tests { .new_object() .with_field("email", "bob@example.com") .finish(); - let (m, v) = builder.finish(); + let (m, v) = builder.finish().unwrap(); let expected_value = Variant::new(&m, &v); expect( diff --git a/parquet-variant-compute/src/variant_get.rs b/parquet-variant-compute/src/variant_get.rs index 774da0e72e8c..34983b75bd47 100644 --- a/parquet-variant-compute/src/variant_get.rs +++ b/parquet-variant-compute/src/variant_get.rs @@ -1796,7 +1796,7 @@ mod test { obj.insert("x", Variant::Int32(42)); obj.insert("y", Variant::from("foo")); obj.finish(); - builder.finish() + builder.finish().unwrap() }; // Create metadata array (same for both rows) @@ -1810,7 +1810,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; @@ -2175,7 +2175,7 @@ mod test { let mut obj = builder.new_object(); obj.insert("x", Variant::from("foo")); obj.finish(); - builder.finish() + builder.finish().unwrap() }; // Metadata array (same for both rows) @@ -2188,7 +2188,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; @@ -2247,7 +2247,7 @@ mod test { obj.insert("b", Variant::Int32(42)); obj.finish(); - builder.finish() + builder.finish().unwrap() }; let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2)); @@ -2258,7 +2258,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; @@ -2269,7 +2269,7 @@ mod test { let mut obj = builder.new_object(); obj.insert("fallback", Variant::from("data")); obj.finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; @@ -2295,7 +2295,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; let a_value_array = BinaryViewArray::from(vec![ @@ -2360,7 +2360,7 @@ mod test { a_obj.finish(); obj.finish(); - builder.finish() + builder.finish().unwrap() }; let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 3)); @@ -2370,7 +2370,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; @@ -2396,7 +2396,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; let b_value_array = BinaryViewArray::from(vec![ @@ -2425,7 +2425,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; let a_value_array = BinaryViewArray::from(vec![ @@ -3173,7 +3173,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - builder.finish() + builder.finish().unwrap() }; // Create null buffer for top-level nulls @@ -3309,7 +3309,7 @@ mod test { let mut obj = builder.new_object(); obj.insert("y", Variant::from(42)); obj.finish(); - builder.finish() + builder.finish().unwrap() }; let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 4)); @@ -3323,14 +3323,14 @@ mod test { let empty_object_value = { let mut builder = parquet_variant::VariantBuilder::new(); builder.new_object().finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; let y_null_value = { let mut builder = parquet_variant::VariantBuilder::new(); builder.new_object().with_field("y", Variant::Null).finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); value }; diff --git a/parquet-variant-json/src/from_json.rs b/parquet-variant-json/src/from_json.rs index 4c22785ef106..aa35f67588ac 100644 --- a/parquet-variant-json/src/from_json.rs +++ b/parquet-variant-json/src/from_json.rs @@ -47,7 +47,7 @@ use serde_json::{Number, Value}; /// + "\"additional_info\": null}"; /// variant_builder.append_json(&person_string)?; /// -/// let (metadata, value) = variant_builder.finish(); +/// let (metadata, value) = variant_builder.finish().unwrap(); /// /// let variant = parquet_variant::Variant::try_new(&metadata, &value)?; /// @@ -147,7 +147,7 @@ mod test { fn run(self) -> Result<(), ArrowError> { let mut variant_builder = VariantBuilder::new(); variant_builder.append_json(self.json)?; - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; assert_eq!(variant, self.expected); Ok(()) @@ -451,7 +451,7 @@ mod test { list_builder.append_value(Variant::Int16(128)); list_builder.append_value(Variant::Int32(-32767431)); list_builder.finish(); - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { @@ -471,7 +471,7 @@ mod test { list_builder.append_value(Variant::Int16(128)); list_builder.append_value(Variant::BooleanFalse); list_builder.finish(); - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { @@ -491,7 +491,7 @@ mod test { } list_builder.append_value(Variant::BooleanTrue); list_builder.finish(); - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { @@ -514,7 +514,7 @@ mod test { list_builder_inner.finish(); } list_builder.finish(); - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let intermediate = format!("[{}]", vec!["null"; 255].join(", ")); let json = format!("[{}]", vec![intermediate; 256].join(", ")); @@ -532,7 +532,7 @@ mod test { object_builder.insert("a", Variant::Int8(3)); object_builder.insert("b", Variant::Int8(2)); object_builder.finish(); - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { json: "{\"b\": 2, \"a\": 1, \"a\": 3}", @@ -556,7 +556,7 @@ mod test { inner_list_builder.append_value(Variant::Double(1001e-3)); inner_list_builder.finish(); object_builder.finish(); - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { json: "{\"numbers\": [4, -3e0, 1001e-3], \"null\": null, \"booleans\": [true, false]}", @@ -598,7 +598,7 @@ mod test { // Manually verify raw JSON value size let mut variant_builder = VariantBuilder::new(); variant_builder.append_json(&json)?; - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let v = Variant::try_new(&metadata, &value)?; let output_string = v.to_json_string()?; assert_eq!(output_string, json); @@ -624,7 +624,7 @@ mod test { inner_object_builder.finish(); }); object_builder.finish(); - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { @@ -639,7 +639,7 @@ mod test { let json = "{\"爱\":\"अ\",\"a\":1}"; let mut variant_builder = VariantBuilder::new(); variant_builder.append_json(json)?; - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let v = Variant::try_new(&metadata, &value)?; let output_string = v.to_json_string()?; assert_eq!(output_string, "{\"a\":1,\"爱\":\"अ\"}"); @@ -648,7 +648,7 @@ mod test { object_builder.insert("a", Variant::Int8(1)); object_builder.insert("爱", Variant::ShortString(ShortString::try_new("अ")?)); object_builder.finish(); - let (metadata, value) = variant_builder.finish(); + let (metadata, value) = variant_builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; assert_eq!( diff --git a/parquet-variant-json/src/to_json.rs b/parquet-variant-json/src/to_json.rs index 707b1fe0a38f..a61deca82cd8 100644 --- a/parquet-variant-json/src/to_json.rs +++ b/parquet-variant-json/src/to_json.rs @@ -68,7 +68,7 @@ pub trait VariantToJson { /// object_builder.insert("last_name", "Li"); /// object_builder.finish(); /// // Finish the builder to get the metadata and value - /// let (metadata, value) = builder.finish(); + /// let (metadata, value) = builder.finish().unwrap(); /// // Create the Variant and convert to JSON /// let variant = Variant::try_new(&metadata, &value)?; /// let mut writer = Vec::new(); @@ -126,7 +126,7 @@ pub trait VariantToJson { /// object_builder.insert("last_name", "Li"); /// object_builder.finish(); /// // Finish the builder to get the metadata and value - /// let (metadata, value) = builder.finish(); + /// let (metadata, value) = builder.finish().unwrap(); /// // Create the Variant and convert to JSON /// let variant = Variant::try_new(&metadata, &value)?; /// let json = variant.to_json_string()?; @@ -968,7 +968,7 @@ mod tests { .with_field("score", 95.5f64) .finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -999,7 +999,7 @@ mod tests { obj.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; assert_eq!(json, "{}"); @@ -1023,7 +1023,7 @@ mod tests { .with_field("unicode", "😀 Smiley") .finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -1054,7 +1054,7 @@ mod tests { .with_value(5i32) .finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; assert_eq!(json, "[1,2,3,4,5]"); @@ -1079,7 +1079,7 @@ mod tests { list.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; assert_eq!(json, "[]"); @@ -1105,7 +1105,7 @@ mod tests { .with_value(std::f64::consts::PI) .finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -1136,7 +1136,7 @@ mod tests { obj.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -1168,7 +1168,7 @@ mod tests { .with_value(100i64) .finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -1203,7 +1203,7 @@ mod tests { obj.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; diff --git a/parquet-variant/benches/variant_builder.rs b/parquet-variant/benches/variant_builder.rs index 420fa583ee1a..60f98404d1b8 100644 --- a/parquet-variant/benches/variant_builder.rs +++ b/parquet-variant/benches/variant_builder.rs @@ -78,7 +78,7 @@ fn bench_object_field_names_reverse_order(c: &mut Criterion) { } object_builder.finish(); - hint::black_box(variant.finish()); + hint::black_box(variant.finish().unwrap()); }) }); } @@ -115,7 +115,7 @@ fn bench_object_same_schema(c: &mut Criterion) { inner_list_builder.finish(); object_builder.finish(); - hint::black_box(variant.finish()); + hint::black_box(variant.finish().unwrap()); } }) }); @@ -158,7 +158,7 @@ fn bench_object_list_same_schema(c: &mut Criterion) { } list_builder.finish(); - hint::black_box(variant.finish()); + hint::black_box(variant.finish().unwrap()); }) }); } @@ -203,7 +203,7 @@ fn bench_object_unknown_schema(c: &mut Criterion) { inner_list_builder.finish(); } object_builder.finish(); - hint::black_box(variant.finish()); + hint::black_box(variant.finish().unwrap()); } }) }); @@ -258,7 +258,7 @@ fn bench_object_list_unknown_schema(c: &mut Criterion) { } list_builder.finish(); - hint::black_box(variant.finish()); + hint::black_box(variant.finish().unwrap()); }) }); } @@ -318,7 +318,7 @@ fn bench_object_partially_same_schema(c: &mut Criterion) { } object_builder.finish(); - hint::black_box(variant.finish()); + hint::black_box(variant.finish().unwrap()); } }) }); @@ -383,7 +383,7 @@ fn bench_object_list_partially_same_schema(c: &mut Criterion) { } list_builder.finish(); - hint::black_box(variant.finish()); + hint::black_box(variant.finish().unwrap()); }) }); } @@ -409,7 +409,7 @@ fn bench_validation_validated_vs_unvalidated(c: &mut Criterion) { list.finish(); obj.finish(); - test_data.push(builder.finish()); + test_data.push(builder.finish().unwrap()); } let mut group = c.benchmark_group("validation"); @@ -466,7 +466,7 @@ fn bench_iteration_performance(c: &mut Criterion) { } list.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let validated = Variant::try_new(&metadata, &value).unwrap(); let unvalidated = Variant::new(&metadata, &value); diff --git a/parquet-variant/benches/variant_validation.rs b/parquet-variant/benches/variant_validation.rs index dcf7681a76ed..2a82924c0fe0 100644 --- a/parquet-variant/benches/variant_validation.rs +++ b/parquet-variant/benches/variant_validation.rs @@ -44,7 +44,7 @@ fn generate_large_object() -> (Vec, Vec) { } outer_object.finish(); - variant_builder.finish() + variant_builder.finish().unwrap() } fn generate_complex_object() -> (Vec, Vec) { @@ -76,7 +76,7 @@ fn generate_complex_object() -> (Vec, Vec) { object_builder.finish(); - variant_builder.finish() + variant_builder.finish().unwrap() } fn generate_large_nested_list() -> (Vec, Vec) { @@ -97,7 +97,7 @@ fn generate_large_nested_list() -> (Vec, Vec) { list_builder_inner.finish(); } list_builder.finish(); - variant_builder.finish() + variant_builder.finish().unwrap() } // Generates a large object and performs full validation diff --git a/parquet-variant/src/builder.rs b/parquet-variant/src/builder.rs index e6122f062c38..c4c9af5ed17f 100644 --- a/parquet-variant/src/builder.rs +++ b/parquet-variant/src/builder.rs @@ -483,7 +483,7 @@ impl Drop for ParentState<'_, S> { /// let mut builder = VariantBuilder::new(); /// builder.append_value(Variant::Int8(42)); /// // Finish the builder to get the metadata and value -/// let (metadata, value) = builder.finish(); +/// let (metadata, value) = builder.finish().unwrap(); /// // use the Variant API to verify the result /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// assert_eq!(variant, Variant::Int8(42)); @@ -508,7 +508,7 @@ impl Drop for ParentState<'_, S> { /// object_builder.insert("last_name", "Li"); /// object_builder.finish(); // call finish to finalize the object /// // Finish the builder to get the metadata and value -/// let (metadata, value) = builder.finish(); +/// let (metadata, value) = builder.finish().unwrap(); /// // use the Variant API to verify the result /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_object = variant.as_object().unwrap(); @@ -533,7 +533,7 @@ impl Drop for ParentState<'_, S> { /// .with_field("first_name", "Jiaying") /// .with_field("last_name", "Li") /// .finish(); -/// let (metadata, value) = builder.finish(); +/// let (metadata, value) = builder.finish().unwrap(); /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_object = variant.as_object().unwrap(); /// assert_eq!( @@ -559,7 +559,7 @@ impl Drop for ParentState<'_, S> { /// // call finish to finalize the list /// list_builder.finish(); /// // Finish the builder to get the metadata and value -/// let (metadata, value) = builder.finish(); +/// let (metadata, value) = builder.finish().unwrap(); /// // use the Variant API to verify the result /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_list = variant.as_list().unwrap(); @@ -579,7 +579,7 @@ impl Drop for ParentState<'_, S> { /// .with_value(2i8) /// .with_value(3i8) /// .finish(); -/// let (metadata, value) = builder.finish(); +/// let (metadata, value) = builder.finish().unwrap(); /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_list = variant.as_list().unwrap(); /// assert_eq!(variant_list.get(0).unwrap(), Variant::Int8(1)); @@ -625,7 +625,7 @@ impl Drop for ParentState<'_, S> { /// /// list_builder.finish(); /// // Finish the builder to get the metadata and value -/// let (metadata, value) = builder.finish(); +/// let (metadata, value) = builder.finish().unwrap(); /// // use the Variant API to verify the result /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_list = variant.as_list().unwrap(); @@ -689,7 +689,7 @@ impl Drop for ParentState<'_, S> { /// obj.insert("score", 95.5); /// obj.finish(); /// -/// let (metadata, value) = builder.finish(); +/// let (metadata, value) = builder.finish().unwrap(); /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// ``` /// @@ -707,7 +707,7 @@ impl Drop for ParentState<'_, S> { /// obj.insert("score", 88.0); /// obj.finish(); /// -/// let (metadata, value) = builder.finish(); +/// let (metadata, value) = builder.finish().unwrap(); /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// ``` #[derive(Default, Debug)] @@ -792,10 +792,41 @@ impl VariantBuilder { self.metadata_builder.upsert_field_name(field_name); } + /// Returns true if a top-level variant value has already been written to this builder. + /// + /// A [`VariantBuilder`] holds exactly one top-level variant value. Any committed top-level + /// value leaves bytes in the value buffer; a child builder dropped without `finish()` has + /// its bytes rolled back by [`ParentState`], so the offset is a faithful indicator. + fn has_top_level_value(&self) -> bool { + self.value_builder.offset() != 0 + } + + fn ensure_no_top_level_value(&self) { + assert!( + !self.has_top_level_value(), + "VariantBuilder already contains a top-level variant value; only one is allowed" + ); + } + + fn check_no_top_level_value(&self) -> Result<(), ArrowError> { + if self.has_top_level_value() { + return Err(ArrowError::InvalidArgumentError( + "VariantBuilder already contains a top-level variant value; only one is allowed" + .into(), + )); + } + Ok(()) + } + /// Create an [`ListBuilder`] for creating [`Variant::List`] values. /// /// See the examples on [`VariantBuilder`] for usage. + /// + /// # Panics + /// + /// Panics if a top-level variant value has already been written to this builder. pub fn new_list(&mut self) -> ListBuilder<'_, ()> { + self.ensure_no_top_level_value(); let parent_state = ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); ListBuilder::new(parent_state, self.validate_unique_fields) @@ -804,7 +835,12 @@ impl VariantBuilder { /// Create an [`ObjectBuilder`] for creating [`Variant::Object`] values. /// /// See the examples on [`VariantBuilder`] for usage. + /// + /// # Panics + /// + /// Panics if a top-level variant value has already been written to this builder. pub fn new_object(&mut self) -> ObjectBuilder<'_, ()> { + self.ensure_no_top_level_value(); let parent_state = ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); ObjectBuilder::new(parent_state, self.validate_unique_fields) @@ -814,8 +850,9 @@ impl VariantBuilder { /// /// # Panics /// - /// This method will panic if the variant contains duplicate field names in objects - /// when validation is enabled. For a fallible version, use [`VariantBuilder::try_append_value`] + /// Panics if a top-level variant value has already been written to this builder, or if the + /// variant contains duplicate field names in objects when validation is enabled. For a + /// fallible version, use [`VariantBuilder::try_append_value`]. /// /// # Example /// ``` @@ -825,15 +862,19 @@ impl VariantBuilder { /// builder.append_value(42i8); /// ``` pub fn append_value<'m, 'd, T: Into>>(&mut self, value: T) { + self.ensure_no_top_level_value(); let state = ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); ValueBuilder::append_variant(state, value.into()) } /// Append a value to the builder. + /// + /// Returns an error if a top-level variant value has already been written to this builder. pub fn try_append_value<'m, 'd, T: Into>>( &mut self, value: T, ) -> Result<(), ArrowError> { + self.check_no_top_level_value()?; let state = ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); ValueBuilder::try_append_variant(state, value.into()) } @@ -846,18 +887,30 @@ impl VariantBuilder { /// /// The caller must ensure that the metadata dictionary entries are already built and correct for /// any objects or lists being appended. + /// + /// # Panics + /// + /// Panics if a top-level variant value has already been written to this builder. pub fn append_value_bytes<'m, 'd>(&mut self, value: impl Into>) { + self.ensure_no_top_level_value(); let state = ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); ValueBuilder::append_variant_bytes(state, value.into()); } /// Finish the builder and return the metadata and value buffers. - pub fn finish(mut self) -> (Vec, Vec) { + /// + /// Returns an error if no top-level variant value has been appended. + pub fn finish(mut self) -> Result<(Vec, Vec), ArrowError> { + if !self.has_top_level_value() { + return Err(ArrowError::InvalidArgumentError( + "VariantBuilder is empty; append a top-level value before calling finish()".into(), + )); + } self.metadata_builder.finish(); - ( + Ok(( self.metadata_builder.into_inner(), self.value_builder.into_inner(), - ) + )) } } @@ -915,10 +968,12 @@ impl VariantBuilderExt for VariantBuilder { } fn try_new_list(&mut self) -> Result>, ArrowError> { + self.check_no_top_level_value()?; Ok(self.new_list()) } fn try_new_object(&mut self) -> Result>, ArrowError> { + self.check_no_top_level_value()?; Ok(self.new_object()) } } @@ -957,7 +1012,7 @@ mod tests { fn test_variant_roundtrip<'m, 'd, T: Into>>(input: T, expected: Variant) { let mut builder = VariantBuilder::new(); builder.append_value(input); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap_or_else(|_| { panic!("Failed to create variant from metadata and value: {metadata:?}, {value:?}") }); @@ -994,7 +1049,7 @@ mod tests { outer_object_builder.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_object = variant.as_object().unwrap(); @@ -1044,20 +1099,18 @@ mod tests { variant2.add_field_name("a"); assert!(!variant2.metadata_builder.is_sorted); - // per the spec, make sure the variant will fail to build if only metadata is provided - let (m, v) = variant2.finish(); - let res = Variant::try_new(&m, &v); - assert!(res.is_err()); - - // since it is not sorted, make sure the metadata says so - let header = VariantMetadata::try_new(&m).unwrap(); - assert!(!header.is_sorted()); + // per the spec, a variant must have a top-level value; finish() rejects empty + let err = variant2.finish().unwrap_err(); + assert!( + err.to_string().contains("empty"), + "unexpected error: {err}" + ); } // write out variant1 and make sure the sorted flag is properly encoded variant1.append_value(false); - let (m, v) = variant1.finish(); + let (m, v) = variant1.finish().unwrap(); let res = Variant::try_new(&m, &v); assert!(res.is_ok()); @@ -1083,7 +1136,7 @@ mod tests { obj.insert("d", 2); obj.finish(); - let (metadata, value) = variant1.finish(); + let (metadata, value) = variant1.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); @@ -1117,7 +1170,7 @@ mod tests { obj.insert("a", 2); obj.finish(); - let (metadata, value) = variant1.finish(); + let (metadata, value) = variant1.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); @@ -1165,7 +1218,7 @@ mod tests { builder.append_value(42i8); // The original builder should be unchanged - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); @@ -1184,7 +1237,7 @@ mod tests { builder.append_value(42i8); // The original builder should be unchanged - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); // rolled back @@ -1207,7 +1260,7 @@ mod tests { // The parent list should only contain the original values list_builder.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); @@ -1235,7 +1288,7 @@ mod tests { builder.append_value(2i8); // Only the second attempt should appear in the final variant - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); @@ -1258,7 +1311,7 @@ mod tests { // The parent list should only contain the original values list_builder.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); @@ -1286,7 +1339,7 @@ mod tests { builder.append_value(2i8); // Only the second attempt should appear in the final variant - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); // rolled back @@ -1309,7 +1362,7 @@ mod tests { // The parent object should only contain the original fields object_builder.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert_eq!(metadata.len(), 2); @@ -1340,7 +1393,7 @@ mod tests { builder.append_value(2i8); // Only the second attempt should appear in the final variant - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); // rolled back @@ -1363,7 +1416,7 @@ mod tests { // The parent object should only contain the original fields object_builder.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert_eq!(metadata.len(), 2); // the fields of nested_object_builder has been rolled back @@ -1394,7 +1447,7 @@ mod tests { builder.append_value(2i8); // Only the second attempt should appear in the final variant - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert_eq!(metadata.len(), 0); // rolled back @@ -1439,10 +1492,10 @@ mod tests { } list.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let v1 = Variant::try_new(&metadata, &value).unwrap(); - let (metadata, value) = VariantBuilder::new().with_value(v1.clone()).finish(); + let (metadata, value) = VariantBuilder::new().with_value(v1.clone()).finish().unwrap(); let v2 = Variant::try_new(&metadata, &value).unwrap(); assert_eq!(format!("{v1:?}"), format!("{v2:?}")); @@ -1471,7 +1524,7 @@ mod tests { } obj.finish(); } - let (metadata, value1) = builder.finish(); + let (metadata, value1) = builder.finish().unwrap(); let variant1 = Variant::try_new(&metadata, &value1).unwrap(); // Copy using the new bytes API @@ -1498,7 +1551,7 @@ mod tests { obj.insert("field4", "value4"); obj.finish(); } - let (metadata1, value1) = builder.finish(); + let (metadata1, value1) = builder.finish().unwrap(); let original_variant = Variant::try_new(&metadata1, &value1).unwrap(); let original_obj = original_variant.as_object().unwrap(); @@ -1601,7 +1654,7 @@ mod tests { root_obj.insert("total_count", 3i32); root_obj.finish(); } - let (metadata1, value1) = builder.finish(); + let (metadata1, value1) = builder.finish().unwrap(); let original_variant = Variant::try_new(&metadata1, &value1).unwrap(); let original_obj = original_variant.as_object().unwrap(); let original_users = original_obj.get("users").unwrap(); diff --git a/parquet-variant/src/builder/list.rs b/parquet-variant/src/builder/list.rs index 5064904ca7de..122aa3619dbc 100644 --- a/parquet-variant/src/builder/list.rs +++ b/parquet-variant/src/builder/list.rs @@ -294,7 +294,7 @@ mod tests { .with_value("test") .finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); assert!(!metadata.is_empty()); assert!(!value.is_empty()); @@ -332,7 +332,7 @@ mod tests { outer_list_builder.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_list = variant.as_list().unwrap(); @@ -382,7 +382,7 @@ mod tests { list_builder1.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let list1 = variant.as_list().unwrap(); @@ -422,7 +422,7 @@ mod tests { list.append_value(1.234f64); list.finish(); } - let (metadata1, value1) = builder.finish(); + let (metadata1, value1) = builder.finish().unwrap(); let original_variant = Variant::try_new(&metadata1, &value1).unwrap(); let original_list = original_variant.as_list().unwrap(); @@ -470,7 +470,7 @@ mod tests { let variant = Variant::new(&m1, &v1); let mut builder = VariantBuilder::new(); builder.append_value(variant.clone()); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); assert_eq!(variant, Variant::new(&metadata, &value)); } @@ -484,7 +484,7 @@ mod tests { .with_value("a string value") .finish(); - builder.finish() + builder.finish().unwrap() } #[test] @@ -493,7 +493,7 @@ mod tests { let variant = Variant::new(&m1, &v1); let mut builder = VariantBuilder::new(); builder.append_value(variant.clone()); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); assert_eq!(variant, Variant::new(&metadata, &value)); } @@ -509,7 +509,7 @@ mod tests { list.finish(); - builder.finish() + builder.finish().unwrap() } #[test] @@ -532,7 +532,7 @@ mod tests { list_builder.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let list = variant.as_list().unwrap(); @@ -571,7 +571,7 @@ mod tests { list_builder.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let list = variant.as_list().unwrap(); @@ -628,7 +628,7 @@ mod tests { list_builder.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let list = variant.as_list().unwrap(); @@ -717,7 +717,7 @@ mod tests { outer_list_builder.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_list = variant.as_list().unwrap(); diff --git a/parquet-variant/src/builder/metadata.rs b/parquet-variant/src/builder/metadata.rs index efccc2e4c63e..1ff5f6da9ba1 100644 --- a/parquet-variant/src/builder/metadata.rs +++ b/parquet-variant/src/builder/metadata.rs @@ -359,11 +359,12 @@ mod test { #[test] fn test_read_only_metadata_builder() { // First create some metadata with a few field names - let mut default_builder = VariantBuilder::new(); - default_builder.add_field_name("name"); - default_builder.add_field_name("age"); - default_builder.add_field_name("active"); - let (metadata_bytes, _) = default_builder.finish(); + let mut default_builder = WritableMetadataBuilder::default(); + default_builder.upsert_field_name("name"); + default_builder.upsert_field_name("age"); + default_builder.upsert_field_name("active"); + default_builder.finish(); + let metadata_bytes = default_builder.into_inner(); // Use the metadata to build new variant values let metadata = VariantMetadata::try_new(&metadata_bytes).unwrap(); @@ -394,9 +395,10 @@ mod test { #[test] fn test_read_only_metadata_builder_fails_on_unknown_field() { // Create metadata with only one field - let mut default_builder = VariantBuilder::new(); - default_builder.add_field_name("known_field"); - let (metadata_bytes, _) = default_builder.finish(); + let mut default_builder = WritableMetadataBuilder::default(); + default_builder.upsert_field_name("known_field"); + default_builder.finish(); + let metadata_bytes = default_builder.into_inner(); // Use the metadata to build new variant values let metadata = VariantMetadata::try_new(&metadata_bytes).unwrap(); diff --git a/parquet-variant/src/builder/object.rs b/parquet-variant/src/builder/object.rs index 876c2e2d4c7c..9158fd30f2a1 100644 --- a/parquet-variant/src/builder/object.rs +++ b/parquet-variant/src/builder/object.rs @@ -431,6 +431,7 @@ impl VariantBuilderExt for ObjectFieldBuilder<'_, '_, ' mod tests { use crate::{ ParentState, ValueBuilder, Variant, VariantBuilder, VariantMetadata, + WritableMetadataBuilder, builder::{metadata::ReadOnlyMetadataBuilder, object::ObjectBuilder}, decoder::VariantBasicType, }; @@ -445,7 +446,7 @@ mod tests { .with_field("age", 42i8) .finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); assert!(!metadata.is_empty()); assert!(!value.is_empty()); } @@ -461,7 +462,7 @@ mod tests { .with_field("banana", "yellow") .finish(); - let (_, value) = builder.finish(); + let (_, value) = builder.finish().unwrap(); let header = value[0]; assert_eq!(header & 0x03, VariantBasicType::Object as u8); @@ -485,7 +486,7 @@ mod tests { .with_field("name", "Metta World Peace") // Duplicate field .finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let obj = variant.as_object().unwrap(); @@ -501,11 +502,12 @@ mod tests { #[test] fn test_read_only_metadata_builder() { // First create some metadata with a few field names - let mut default_builder = VariantBuilder::new(); - default_builder.add_field_name("name"); - default_builder.add_field_name("age"); - default_builder.add_field_name("active"); - let (metadata_bytes, _) = default_builder.finish(); + let mut default_builder = WritableMetadataBuilder::default(); + default_builder.upsert_field_name("name"); + default_builder.upsert_field_name("age"); + default_builder.upsert_field_name("active"); + default_builder.finish(); + let metadata_bytes = default_builder.into_inner(); // Use the metadata to build new variant values let metadata = VariantMetadata::try_new(&metadata_bytes).unwrap(); @@ -543,7 +545,7 @@ mod tests { builder.append_value(variant.clone()); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); assert_eq!(variant, Variant::new(&metadata, &value)); } @@ -556,7 +558,7 @@ mod tests { obj.insert("b", true); obj.insert("a", false); obj.finish(); - builder.finish() + builder.finish().unwrap() } #[test] @@ -568,7 +570,7 @@ mod tests { let mut builder = VariantBuilder::new().with_metadata(VariantMetadata::new(&m1)); builder.append_value(variant.clone()); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let result_variant = Variant::new(&metadata, &value); assert_eq!(variant, result_variant); @@ -590,7 +592,7 @@ mod tests { outer_obj.finish(); } - builder.finish() + builder.finish().unwrap() } #[test] @@ -616,7 +618,7 @@ mod tests { outer_object_builder.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_object = variant.as_object().unwrap(); @@ -659,7 +661,7 @@ mod tests { outer_object_builder.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_object = variant.as_object().unwrap(); @@ -750,7 +752,7 @@ mod tests { outer_object_builder.finish(); } - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); // note, object fields are now sorted lexigraphically by field name /* @@ -850,7 +852,7 @@ mod tests { outer_list.finish(); // Verify the nested object is built correctly -- the nested object "x" should have "won" - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_element = variant.get_list_element(0).unwrap(); let inner_element = outer_element.get_list_element(0).unwrap(); @@ -899,7 +901,8 @@ mod tests { inner_list.finish(); outer_list.finish(); - // Valid object should succeed + // Valid object should succeed (fresh builder — one top-level value per VariantBuilder) + let mut builder = VariantBuilder::new().with_validate_unique_fields(true); let mut list = builder.new_list(); let mut valid_obj = list.new_object(); valid_obj.insert("m", 1); diff --git a/parquet-variant/src/variant.rs b/parquet-variant/src/variant.rs index 58a3f7eeb261..db6f6795e9f0 100644 --- a/parquet-variant/src/variant.rs +++ b/parquet-variant/src/variant.rs @@ -1459,7 +1459,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # let mut obj = builder.new_object(); /// # obj.insert("name", "John"); /// # obj.finish(); - /// # builder.finish() + /// # builder.finish().unwrap() /// # }; /// // object that is {"name": "John"} /// let variant = Variant::new(&metadata, &value); @@ -1487,7 +1487,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # let mut obj = builder.new_object(); /// # obj.insert("name", "John"); /// # obj.finish(); - /// # let (metadata, value) = builder.finish(); + /// # let (metadata, value) = builder.finish().unwrap(); /// // object that is {"name": "John"} /// let variant = Variant::new(&metadata, &value); /// // use the `get_object_field` method to access the object @@ -1519,7 +1519,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # list.append_value("John"); /// # list.append_value("Doe"); /// # list.finish(); - /// # builder.finish() + /// # builder.finish().unwrap() /// # }; /// // list that is ["John", "Doe"] /// let variant = Variant::new(&metadata, &value); @@ -1578,7 +1578,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # list.append_value("John"); /// # list.append_value("Doe"); /// # list.finish(); - /// # let (metadata, value) = builder.finish(); + /// # let (metadata, value) = builder.finish().unwrap(); /// // list that is ["John", "Doe"] /// let variant = Variant::new(&metadata, &value); /// // use the `get_list_element` method to access the list @@ -1616,7 +1616,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # list.append_value("baz"); /// # list.finish(); /// # obj.finish(); - /// # let (metadata, value) = builder.finish(); + /// # let (metadata, value) = builder.finish().unwrap(); /// // given a variant like `{"foo": ["bar", "baz"]}` /// let variant = Variant::new(&metadata, &value); /// // Accessing a non existent path returns None @@ -2070,7 +2070,7 @@ mod tests { root_obj.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::try_new(&metadata, &value).unwrap(); // Test Debug formatter (?) diff --git a/parquet-variant/src/variant/list.rs b/parquet-variant/src/variant/list.rs index 7301d0570645..1100adfd0c2d 100644 --- a/parquet-variant/src/variant/list.rs +++ b/parquet-variant/src/variant/list.rs @@ -624,7 +624,7 @@ mod tests { list_builder.finish(); // Finish the builder to get the metadata and value - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); // use the Variant API to verify the result let variant = Variant::try_new(&metadata, &value).unwrap(); @@ -716,7 +716,7 @@ mod tests { let (metadata3, value3) = make_listi32(10i32..20i32); object_builder.insert("list3", Variant::new(&metadata3, &value3)); object_builder.finish(); - builder.finish() + builder.finish().unwrap() }; let variant = Variant::try_new(&metadata, &value).unwrap(); @@ -733,7 +733,7 @@ mod tests { let mut list_builder = variant_builder.new_list(); list_builder.extend(range); list_builder.finish(); - variant_builder.finish() + variant_builder.finish().unwrap() } /// return metadata/value for a simple variant list with values in a range @@ -742,6 +742,6 @@ mod tests { let mut list_builder = variant_builder.new_list(); list_builder.extend(range); list_builder.finish(); - variant_builder.finish() + variant_builder.finish().unwrap() } } diff --git a/parquet-variant/src/variant/metadata.rs b/parquet-variant/src/variant/metadata.rs index d5d08d204c83..185f6b1eabd6 100644 --- a/parquet-variant/src/variant/metadata.rs +++ b/parquet-variant/src/variant/metadata.rs @@ -621,7 +621,7 @@ mod tests { o.finish(); - let (m, _) = b.finish(); + let (m, _) = b.finish().unwrap(); let m1 = VariantMetadata::new(&m); assert!(m1.is_sorted()); @@ -656,7 +656,7 @@ mod tests { o.finish(); - let (m, _) = b.finish(); + let (m, _) = b.finish().unwrap(); let m1 = VariantMetadata::new(&m); let m2 = VariantMetadata::new(&m); diff --git a/parquet-variant/src/variant/object.rs b/parquet-variant/src/variant/object.rs index bb91584cefa6..8905ebbaf969 100644 --- a/parquet-variant/src/variant/object.rs +++ b/parquet-variant/src/variant/object.rs @@ -551,7 +551,7 @@ mod tests { fn test_variant_object_empty_fields() { let mut builder = VariantBuilder::new(); builder.new_object().with_field("", 42).finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); // Resulting object is valid and has a single empty field let variant = Variant::try_new(&metadata, &value).unwrap(); @@ -677,7 +677,7 @@ mod tests { } obj.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::new(&metadata, &value); if let Variant::Object(obj) = variant { @@ -738,7 +738,7 @@ mod tests { } obj.finish(); - let (metadata, value) = builder.finish(); + let (metadata, value) = builder.finish().unwrap(); let variant = Variant::new(&metadata, &value); if let Variant::Object(obj) = variant { @@ -785,7 +785,7 @@ mod tests { o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v1 = Variant::try_new(&m, &v).unwrap(); let v2 = Variant::try_new(&m, &v).unwrap(); @@ -802,7 +802,7 @@ mod tests { o.insert("b", false); o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v1 = Variant::try_new(&m, &v).unwrap(); @@ -813,7 +813,7 @@ mod tests { o.insert("b", false); o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v2 = Variant::try_new(&m, &v).unwrap(); @@ -830,7 +830,7 @@ mod tests { o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v1 = Variant::try_new(&m, &v).unwrap(); @@ -844,7 +844,7 @@ mod tests { inner_o.finish(); o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v2 = Variant::try_new(&m, &v).unwrap(); @@ -868,7 +868,7 @@ mod tests { o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v1 = Variant::try_new(&m, &v).unwrap(); @@ -881,7 +881,7 @@ mod tests { o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v2 = Variant::try_new(&m, &v).unwrap(); assert_ne!(v1, v2); @@ -897,7 +897,7 @@ mod tests { o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v1 = Variant::try_new(&m, &v).unwrap(); assert!(!v1.metadata().is_sorted()); @@ -912,7 +912,7 @@ mod tests { o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v2 = Variant::try_new(&m, &v).unwrap(); @@ -932,7 +932,7 @@ mod tests { o.finish(); - let (meta1, value1) = b.finish(); + let (meta1, value1) = b.finish().unwrap(); let v1 = Variant::try_new(&meta1, &value1).unwrap(); // v1 is sorted @@ -947,7 +947,7 @@ mod tests { o.finish(); - let (meta2, value2) = b.finish(); + let (meta2, value2) = b.finish().unwrap(); let v2 = Variant::try_new(&meta2, &value2).unwrap(); // v2 is not sorted @@ -971,7 +971,7 @@ mod tests { o.finish(); - let (m, v) = b.finish(); + let (m, v) = b.finish().unwrap(); let v1 = Variant::try_new(&m, &v).unwrap(); diff --git a/parquet-variant/tests/variant_interop.rs b/parquet-variant/tests/variant_interop.rs index 70b42e1f3c28..920b0a773fb2 100644 --- a/parquet-variant/tests/variant_interop.rs +++ b/parquet-variant/tests/variant_interop.rs @@ -310,7 +310,7 @@ fn variant_array_builder() { arr.append_value(9i8); arr.finish(); - let (built_metadata, built_value) = builder.finish(); + let (built_metadata, built_value) = builder.finish().unwrap(); let actual = Variant::try_new(&built_metadata, &built_value).unwrap(); let case = Case::load("array_primitive"); let expected = case.variant(); @@ -339,7 +339,7 @@ fn variant_object_builder() { obj.finish(); - let (built_metadata, built_value) = builder.finish(); + let (built_metadata, built_value) = builder.finish().unwrap(); let actual = Variant::try_new(&built_metadata, &built_value).unwrap(); let case = Case::load("object_primitive"); let expected = case.variant(); @@ -378,7 +378,7 @@ fn test_validation_fuzz_integration() { fn generate_random_variant(rng: &mut StdRng) -> (Vec, Vec) { let mut builder = VariantBuilder::new(); generate_random_value(rng, &mut builder, 3); // Max depth of 3 - builder.finish() + builder.finish().unwrap() } fn generate_random_value(rng: &mut StdRng, builder: &mut VariantBuilder, max_depth: u32) { @@ -466,11 +466,12 @@ fn generate_random_value(rng: &mut StdRng, builder: &mut VariantBuilder, max_dep ) .unwrap(); - // timestamp w/o timezone - builder.append_value(data_time.naive_local()); - - // timestamp with timezone - builder.append_value(data_time.naive_utc().and_utc()); + // randomly pick timestamp with or without timezone + if rng.random_bool(0.5) { + builder.append_value(data_time.naive_local()); + } else { + builder.append_value(data_time.naive_utc().and_utc()); + } } 17 => { builder.append_value(Uuid::new_v4()); From 93c39cf0e41350c39097b2ed8cb35a92d778dd98 Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Tue, 26 May 2026 12:03:06 -0400 Subject: [PATCH 2/8] Add try version of the APIs + cleanup --- .../benches/variant_kernels.rs | 2 +- .../src/cast_to_variant.rs | 32 ++--- parquet-variant-compute/src/from_json.rs | 6 +- parquet-variant-compute/src/shred_variant.rs | 2 +- parquet-variant-compute/src/variant_get.rs | 32 ++--- parquet-variant-json/src/from_json.rs | 24 ++-- parquet-variant-json/src/to_json.rs | 22 ++-- parquet-variant/benches/variant_builder.rs | 18 +-- parquet-variant/benches/variant_validation.rs | 6 +- parquet-variant/src/builder.rs | 114 +++++++++++------- parquet-variant/src/builder/list.rs | 24 ++-- parquet-variant/src/builder/object.rs | 22 ++-- parquet-variant/src/variant.rs | 12 +- parquet-variant/src/variant/list.rs | 8 +- parquet-variant/src/variant/metadata.rs | 4 +- parquet-variant/src/variant/object.rs | 30 ++--- parquet-variant/tests/variant_interop.rs | 6 +- 17 files changed, 194 insertions(+), 170 deletions(-) diff --git a/parquet-variant-compute/benches/variant_kernels.rs b/parquet-variant-compute/benches/variant_kernels.rs index 348bd4b7d69d..383697ab8cc6 100644 --- a/parquet-variant-compute/benches/variant_kernels.rs +++ b/parquet-variant-compute/benches/variant_kernels.rs @@ -187,7 +187,7 @@ fn create_primitive_variant_array(size: usize) -> VariantArray { for _ in 0..size { let mut builder = VariantBuilder::new(); builder.append_value(rng.random::()); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); variant_builder.append_variant(Variant::try_new(&metadata, &value).unwrap()); } diff --git a/parquet-variant-compute/src/cast_to_variant.rs b/parquet-variant-compute/src/cast_to_variant.rs index 3789fc143dc1..1b26ffe07d2a 100644 --- a/parquet-variant-compute/src/cast_to_variant.rs +++ b/parquet-variant-compute/src/cast_to_variant.rs @@ -1342,7 +1342,7 @@ mod tests { list.append_value(1); list.append_value(2); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant = Variant::new(&metadata, &value); @@ -1367,7 +1367,7 @@ mod tests { list.append_value(4); list.append_value(5); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant = Variant::new(&metadata, &value); @@ -1388,7 +1388,7 @@ mod tests { list.append_value(1i64); list.append_value(2i64); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant = Variant::new(&metadata, &value); @@ -1413,7 +1413,7 @@ mod tests { list.append_value(4i64); list.append_value(5i64); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant = Variant::new(&metadata, &value); @@ -1441,7 +1441,7 @@ mod tests { list.append_null(); list.append_value(2i32); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant0 = Variant::new(&metadata, &value); @@ -1451,7 +1451,7 @@ mod tests { list.append_value(3i32); list.append_value(4i32); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant1 = Variant::new(&metadata, &value); @@ -1461,7 +1461,7 @@ mod tests { list.append_null(); list.append_null(); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant3 = Variant::new(&metadata, &value); @@ -1487,7 +1487,7 @@ mod tests { list.append_value(3i32); list.append_null(); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant = Variant::new(&metadata, &value); @@ -1515,7 +1515,7 @@ mod tests { list.append_null(); list.append_value(2i64); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant0 = Variant::new(&metadata, &value); @@ -1525,7 +1525,7 @@ mod tests { list.append_value(3i64); list.append_value(4i64); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant1 = Variant::new(&metadata, &value); @@ -1535,7 +1535,7 @@ mod tests { list.append_null(); list.append_null(); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant3 = Variant::new(&metadata, &value); @@ -1561,7 +1561,7 @@ mod tests { list.append_value(3i64); list.append_null(); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant = Variant::new(&metadata, &value); @@ -1598,7 +1598,7 @@ mod tests { list.append_value(0i32); list.append_value(1i32); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant0 = Variant::new(&metadata, &value); @@ -1608,7 +1608,7 @@ mod tests { list.append_null(); list.append_value(3i32); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant1 = Variant::new(&metadata, &value); @@ -1618,7 +1618,7 @@ mod tests { list.append_null(); list.append_null(); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant3 = Variant::new(&metadata, &value); @@ -1653,7 +1653,7 @@ mod tests { list.append_null(); list.append_value(3i64); list.finish(); - builder.finish().unwrap() + builder.finish() }; let variant = Variant::new(&metadata, &value); diff --git a/parquet-variant-compute/src/from_json.rs b/parquet-variant-compute/src/from_json.rs index 1a20231aa057..0983147132a2 100644 --- a/parquet-variant-compute/src/from_json.rs +++ b/parquet-variant-compute/src/from_json.rs @@ -101,7 +101,7 @@ mod test { let mut ob = vb.new_object(); ob.insert("a", Variant::Int8(32)); ob.finish(); - let (object_metadata, object_value) = vb.finish().unwrap(); + let (object_metadata, object_value) = vb.finish(); let expected = Variant::new(&object_metadata, &object_value); assert_eq!(variant_array.value(2), expected); } @@ -150,7 +150,7 @@ mod test { let mut ob = vb.new_object(); ob.insert("a", Variant::Int8(32)); ob.finish(); - let (object_metadata, object_value) = vb.finish().unwrap(); + let (object_metadata, object_value) = vb.finish(); let expected = Variant::new(&object_metadata, &object_value); assert_eq!(variant_array.value(2), expected); } @@ -199,7 +199,7 @@ mod test { let mut ob = vb.new_object(); ob.insert("a", Variant::Int8(32)); ob.finish(); - let (object_metadata, object_value) = vb.finish().unwrap(); + let (object_metadata, object_value) = vb.finish(); let expected = Variant::new(&object_metadata, &object_value); assert_eq!(variant_array.value(2), expected); } diff --git a/parquet-variant-compute/src/shred_variant.rs b/parquet-variant-compute/src/shred_variant.rs index 1f275cef50bb..440f4b716521 100644 --- a/parquet-variant-compute/src/shred_variant.rs +++ b/parquet-variant-compute/src/shred_variant.rs @@ -2105,7 +2105,7 @@ mod tests { .new_object() .with_field("email", "bob@example.com") .finish(); - let (m, v) = builder.finish().unwrap(); + let (m, v) = builder.finish(); let expected_value = Variant::new(&m, &v); expect( diff --git a/parquet-variant-compute/src/variant_get.rs b/parquet-variant-compute/src/variant_get.rs index 34983b75bd47..774da0e72e8c 100644 --- a/parquet-variant-compute/src/variant_get.rs +++ b/parquet-variant-compute/src/variant_get.rs @@ -1796,7 +1796,7 @@ mod test { obj.insert("x", Variant::Int32(42)); obj.insert("y", Variant::from("foo")); obj.finish(); - builder.finish().unwrap() + builder.finish() }; // Create metadata array (same for both rows) @@ -1810,7 +1810,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; @@ -2175,7 +2175,7 @@ mod test { let mut obj = builder.new_object(); obj.insert("x", Variant::from("foo")); obj.finish(); - builder.finish().unwrap() + builder.finish() }; // Metadata array (same for both rows) @@ -2188,7 +2188,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; @@ -2247,7 +2247,7 @@ mod test { obj.insert("b", Variant::Int32(42)); obj.finish(); - builder.finish().unwrap() + builder.finish() }; let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 2)); @@ -2258,7 +2258,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; @@ -2269,7 +2269,7 @@ mod test { let mut obj = builder.new_object(); obj.insert("fallback", Variant::from("data")); obj.finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; @@ -2295,7 +2295,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; let a_value_array = BinaryViewArray::from(vec![ @@ -2360,7 +2360,7 @@ mod test { a_obj.finish(); obj.finish(); - builder.finish().unwrap() + builder.finish() }; let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 3)); @@ -2370,7 +2370,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; @@ -2396,7 +2396,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; let b_value_array = BinaryViewArray::from(vec![ @@ -2425,7 +2425,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; let a_value_array = BinaryViewArray::from(vec![ @@ -3173,7 +3173,7 @@ mod test { let mut builder = parquet_variant::VariantBuilder::new(); let obj = builder.new_object(); obj.finish(); - builder.finish().unwrap() + builder.finish() }; // Create null buffer for top-level nulls @@ -3309,7 +3309,7 @@ mod test { let mut obj = builder.new_object(); obj.insert("y", Variant::from(42)); obj.finish(); - builder.finish().unwrap() + builder.finish() }; let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 4)); @@ -3323,14 +3323,14 @@ mod test { let empty_object_value = { let mut builder = parquet_variant::VariantBuilder::new(); builder.new_object().finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; let y_null_value = { let mut builder = parquet_variant::VariantBuilder::new(); builder.new_object().with_field("y", Variant::Null).finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); value }; diff --git a/parquet-variant-json/src/from_json.rs b/parquet-variant-json/src/from_json.rs index aa35f67588ac..4c22785ef106 100644 --- a/parquet-variant-json/src/from_json.rs +++ b/parquet-variant-json/src/from_json.rs @@ -47,7 +47,7 @@ use serde_json::{Number, Value}; /// + "\"additional_info\": null}"; /// variant_builder.append_json(&person_string)?; /// -/// let (metadata, value) = variant_builder.finish().unwrap(); +/// let (metadata, value) = variant_builder.finish(); /// /// let variant = parquet_variant::Variant::try_new(&metadata, &value)?; /// @@ -147,7 +147,7 @@ mod test { fn run(self) -> Result<(), ArrowError> { let mut variant_builder = VariantBuilder::new(); variant_builder.append_json(self.json)?; - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; assert_eq!(variant, self.expected); Ok(()) @@ -451,7 +451,7 @@ mod test { list_builder.append_value(Variant::Int16(128)); list_builder.append_value(Variant::Int32(-32767431)); list_builder.finish(); - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { @@ -471,7 +471,7 @@ mod test { list_builder.append_value(Variant::Int16(128)); list_builder.append_value(Variant::BooleanFalse); list_builder.finish(); - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { @@ -491,7 +491,7 @@ mod test { } list_builder.append_value(Variant::BooleanTrue); list_builder.finish(); - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { @@ -514,7 +514,7 @@ mod test { list_builder_inner.finish(); } list_builder.finish(); - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let intermediate = format!("[{}]", vec!["null"; 255].join(", ")); let json = format!("[{}]", vec![intermediate; 256].join(", ")); @@ -532,7 +532,7 @@ mod test { object_builder.insert("a", Variant::Int8(3)); object_builder.insert("b", Variant::Int8(2)); object_builder.finish(); - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { json: "{\"b\": 2, \"a\": 1, \"a\": 3}", @@ -556,7 +556,7 @@ mod test { inner_list_builder.append_value(Variant::Double(1001e-3)); inner_list_builder.finish(); object_builder.finish(); - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { json: "{\"numbers\": [4, -3e0, 1001e-3], \"null\": null, \"booleans\": [true, false]}", @@ -598,7 +598,7 @@ mod test { // Manually verify raw JSON value size let mut variant_builder = VariantBuilder::new(); variant_builder.append_json(&json)?; - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let v = Variant::try_new(&metadata, &value)?; let output_string = v.to_json_string()?; assert_eq!(output_string, json); @@ -624,7 +624,7 @@ mod test { inner_object_builder.finish(); }); object_builder.finish(); - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; JsonToVariantTest { @@ -639,7 +639,7 @@ mod test { let json = "{\"爱\":\"अ\",\"a\":1}"; let mut variant_builder = VariantBuilder::new(); variant_builder.append_json(json)?; - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let v = Variant::try_new(&metadata, &value)?; let output_string = v.to_json_string()?; assert_eq!(output_string, "{\"a\":1,\"爱\":\"अ\"}"); @@ -648,7 +648,7 @@ mod test { object_builder.insert("a", Variant::Int8(1)); object_builder.insert("爱", Variant::ShortString(ShortString::try_new("अ")?)); object_builder.finish(); - let (metadata, value) = variant_builder.finish().unwrap(); + let (metadata, value) = variant_builder.finish(); let variant = Variant::try_new(&metadata, &value)?; assert_eq!( diff --git a/parquet-variant-json/src/to_json.rs b/parquet-variant-json/src/to_json.rs index a61deca82cd8..707b1fe0a38f 100644 --- a/parquet-variant-json/src/to_json.rs +++ b/parquet-variant-json/src/to_json.rs @@ -68,7 +68,7 @@ pub trait VariantToJson { /// object_builder.insert("last_name", "Li"); /// object_builder.finish(); /// // Finish the builder to get the metadata and value - /// let (metadata, value) = builder.finish().unwrap(); + /// let (metadata, value) = builder.finish(); /// // Create the Variant and convert to JSON /// let variant = Variant::try_new(&metadata, &value)?; /// let mut writer = Vec::new(); @@ -126,7 +126,7 @@ pub trait VariantToJson { /// object_builder.insert("last_name", "Li"); /// object_builder.finish(); /// // Finish the builder to get the metadata and value - /// let (metadata, value) = builder.finish().unwrap(); + /// let (metadata, value) = builder.finish(); /// // Create the Variant and convert to JSON /// let variant = Variant::try_new(&metadata, &value)?; /// let json = variant.to_json_string()?; @@ -968,7 +968,7 @@ mod tests { .with_field("score", 95.5f64) .finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -999,7 +999,7 @@ mod tests { obj.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; assert_eq!(json, "{}"); @@ -1023,7 +1023,7 @@ mod tests { .with_field("unicode", "😀 Smiley") .finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -1054,7 +1054,7 @@ mod tests { .with_value(5i32) .finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; assert_eq!(json, "[1,2,3,4,5]"); @@ -1079,7 +1079,7 @@ mod tests { list.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; assert_eq!(json, "[]"); @@ -1105,7 +1105,7 @@ mod tests { .with_value(std::f64::consts::PI) .finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -1136,7 +1136,7 @@ mod tests { obj.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -1168,7 +1168,7 @@ mod tests { .with_value(100i64) .finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; @@ -1203,7 +1203,7 @@ mod tests { obj.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value)?; let json = variant.to_json_string()?; diff --git a/parquet-variant/benches/variant_builder.rs b/parquet-variant/benches/variant_builder.rs index 60f98404d1b8..420fa583ee1a 100644 --- a/parquet-variant/benches/variant_builder.rs +++ b/parquet-variant/benches/variant_builder.rs @@ -78,7 +78,7 @@ fn bench_object_field_names_reverse_order(c: &mut Criterion) { } object_builder.finish(); - hint::black_box(variant.finish().unwrap()); + hint::black_box(variant.finish()); }) }); } @@ -115,7 +115,7 @@ fn bench_object_same_schema(c: &mut Criterion) { inner_list_builder.finish(); object_builder.finish(); - hint::black_box(variant.finish().unwrap()); + hint::black_box(variant.finish()); } }) }); @@ -158,7 +158,7 @@ fn bench_object_list_same_schema(c: &mut Criterion) { } list_builder.finish(); - hint::black_box(variant.finish().unwrap()); + hint::black_box(variant.finish()); }) }); } @@ -203,7 +203,7 @@ fn bench_object_unknown_schema(c: &mut Criterion) { inner_list_builder.finish(); } object_builder.finish(); - hint::black_box(variant.finish().unwrap()); + hint::black_box(variant.finish()); } }) }); @@ -258,7 +258,7 @@ fn bench_object_list_unknown_schema(c: &mut Criterion) { } list_builder.finish(); - hint::black_box(variant.finish().unwrap()); + hint::black_box(variant.finish()); }) }); } @@ -318,7 +318,7 @@ fn bench_object_partially_same_schema(c: &mut Criterion) { } object_builder.finish(); - hint::black_box(variant.finish().unwrap()); + hint::black_box(variant.finish()); } }) }); @@ -383,7 +383,7 @@ fn bench_object_list_partially_same_schema(c: &mut Criterion) { } list_builder.finish(); - hint::black_box(variant.finish().unwrap()); + hint::black_box(variant.finish()); }) }); } @@ -409,7 +409,7 @@ fn bench_validation_validated_vs_unvalidated(c: &mut Criterion) { list.finish(); obj.finish(); - test_data.push(builder.finish().unwrap()); + test_data.push(builder.finish()); } let mut group = c.benchmark_group("validation"); @@ -466,7 +466,7 @@ fn bench_iteration_performance(c: &mut Criterion) { } list.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let validated = Variant::try_new(&metadata, &value).unwrap(); let unvalidated = Variant::new(&metadata, &value); diff --git a/parquet-variant/benches/variant_validation.rs b/parquet-variant/benches/variant_validation.rs index 2a82924c0fe0..dcf7681a76ed 100644 --- a/parquet-variant/benches/variant_validation.rs +++ b/parquet-variant/benches/variant_validation.rs @@ -44,7 +44,7 @@ fn generate_large_object() -> (Vec, Vec) { } outer_object.finish(); - variant_builder.finish().unwrap() + variant_builder.finish() } fn generate_complex_object() -> (Vec, Vec) { @@ -76,7 +76,7 @@ fn generate_complex_object() -> (Vec, Vec) { object_builder.finish(); - variant_builder.finish().unwrap() + variant_builder.finish() } fn generate_large_nested_list() -> (Vec, Vec) { @@ -97,7 +97,7 @@ fn generate_large_nested_list() -> (Vec, Vec) { list_builder_inner.finish(); } list_builder.finish(); - variant_builder.finish().unwrap() + variant_builder.finish() } // Generates a large object and performs full validation diff --git a/parquet-variant/src/builder.rs b/parquet-variant/src/builder.rs index c4c9af5ed17f..a4992fc095e7 100644 --- a/parquet-variant/src/builder.rs +++ b/parquet-variant/src/builder.rs @@ -483,7 +483,7 @@ impl Drop for ParentState<'_, S> { /// let mut builder = VariantBuilder::new(); /// builder.append_value(Variant::Int8(42)); /// // Finish the builder to get the metadata and value -/// let (metadata, value) = builder.finish().unwrap(); +/// let (metadata, value) = builder.finish(); /// // use the Variant API to verify the result /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// assert_eq!(variant, Variant::Int8(42)); @@ -508,7 +508,7 @@ impl Drop for ParentState<'_, S> { /// object_builder.insert("last_name", "Li"); /// object_builder.finish(); // call finish to finalize the object /// // Finish the builder to get the metadata and value -/// let (metadata, value) = builder.finish().unwrap(); +/// let (metadata, value) = builder.finish(); /// // use the Variant API to verify the result /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_object = variant.as_object().unwrap(); @@ -533,7 +533,7 @@ impl Drop for ParentState<'_, S> { /// .with_field("first_name", "Jiaying") /// .with_field("last_name", "Li") /// .finish(); -/// let (metadata, value) = builder.finish().unwrap(); +/// let (metadata, value) = builder.finish(); /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_object = variant.as_object().unwrap(); /// assert_eq!( @@ -559,7 +559,7 @@ impl Drop for ParentState<'_, S> { /// // call finish to finalize the list /// list_builder.finish(); /// // Finish the builder to get the metadata and value -/// let (metadata, value) = builder.finish().unwrap(); +/// let (metadata, value) = builder.finish(); /// // use the Variant API to verify the result /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_list = variant.as_list().unwrap(); @@ -579,7 +579,7 @@ impl Drop for ParentState<'_, S> { /// .with_value(2i8) /// .with_value(3i8) /// .finish(); -/// let (metadata, value) = builder.finish().unwrap(); +/// let (metadata, value) = builder.finish(); /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_list = variant.as_list().unwrap(); /// assert_eq!(variant_list.get(0).unwrap(), Variant::Int8(1)); @@ -625,7 +625,7 @@ impl Drop for ParentState<'_, S> { /// /// list_builder.finish(); /// // Finish the builder to get the metadata and value -/// let (metadata, value) = builder.finish().unwrap(); +/// let (metadata, value) = builder.finish(); /// // use the Variant API to verify the result /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// let variant_list = variant.as_list().unwrap(); @@ -689,7 +689,7 @@ impl Drop for ParentState<'_, S> { /// obj.insert("score", 95.5); /// obj.finish(); /// -/// let (metadata, value) = builder.finish().unwrap(); +/// let (metadata, value) = builder.finish(); /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// ``` /// @@ -707,7 +707,7 @@ impl Drop for ParentState<'_, S> { /// obj.insert("score", 88.0); /// obj.finish(); /// -/// let (metadata, value) = builder.finish().unwrap(); +/// let (metadata, value) = builder.finish(); /// let variant = Variant::try_new(&metadata, &value).unwrap(); /// ``` #[derive(Default, Debug)] @@ -824,12 +824,10 @@ impl VariantBuilder { /// /// # Panics /// - /// Panics if a top-level variant value has already been written to this builder. + /// Panics if a top-level variant value has already been written to this builder. For a + /// fallible version, use [`VariantBuilderExt::try_new_list`]. pub fn new_list(&mut self) -> ListBuilder<'_, ()> { - self.ensure_no_top_level_value(); - let parent_state = - ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); - ListBuilder::new(parent_state, self.validate_unique_fields) + VariantBuilderExt::try_new_list(self).unwrap() } /// Create an [`ObjectBuilder`] for creating [`Variant::Object`] values. @@ -838,12 +836,10 @@ impl VariantBuilder { /// /// # Panics /// - /// Panics if a top-level variant value has already been written to this builder. + /// Panics if a top-level variant value has already been written to this builder. For a + /// fallible version, use [`VariantBuilderExt::try_new_object`]. pub fn new_object(&mut self) -> ObjectBuilder<'_, ()> { - self.ensure_no_top_level_value(); - let parent_state = - ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); - ObjectBuilder::new(parent_state, self.validate_unique_fields) + VariantBuilderExt::try_new_object(self).unwrap() } /// Append a value to the builder. @@ -890,17 +886,41 @@ impl VariantBuilder { /// /// # Panics /// - /// Panics if a top-level variant value has already been written to this builder. + /// Panics if a top-level variant value has already been written to this builder. For a + /// fallible version, use [`VariantBuilder::try_append_value_bytes`]. pub fn append_value_bytes<'m, 'd>(&mut self, value: impl Into>) { - self.ensure_no_top_level_value(); + self.try_append_value_bytes(value).unwrap() + } + + /// Tries to append a variant value to the builder by copying raw bytes when possible. + /// + /// This is the fallible version of [`VariantBuilder::append_value_bytes`]. Returns an error + /// if a top-level variant value has already been written to this builder. + pub fn try_append_value_bytes<'m, 'd>( + &mut self, + value: impl Into>, + ) -> Result<(), ArrowError> { + self.check_no_top_level_value()?; let state = ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); ValueBuilder::append_variant_bytes(state, value.into()); + Ok(()) + } + + /// Finish the builder and return the metadata and value buffers. + /// + /// # Panics + /// + /// Panics if no top-level variant value has been appended. For a fallible version, use + /// [`VariantBuilder::try_finish`]. + pub fn finish(self) -> (Vec, Vec) { + self.try_finish() + .expect("VariantBuilder is empty; append a top-level value before calling finish()") } /// Finish the builder and return the metadata and value buffers. /// /// Returns an error if no top-level variant value has been appended. - pub fn finish(mut self) -> Result<(Vec, Vec), ArrowError> { + pub fn try_finish(mut self) -> Result<(Vec, Vec), ArrowError> { if !self.has_top_level_value() { return Err(ArrowError::InvalidArgumentError( "VariantBuilder is empty; append a top-level value before calling finish()".into(), @@ -969,12 +989,16 @@ impl VariantBuilderExt for VariantBuilder { fn try_new_list(&mut self) -> Result>, ArrowError> { self.check_no_top_level_value()?; - Ok(self.new_list()) + let parent_state = + ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); + Ok(ListBuilder::new(parent_state, self.validate_unique_fields)) } fn try_new_object(&mut self) -> Result>, ArrowError> { self.check_no_top_level_value()?; - Ok(self.new_object()) + let parent_state = + ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); + Ok(ObjectBuilder::new(parent_state, self.validate_unique_fields)) } } @@ -1012,7 +1036,7 @@ mod tests { fn test_variant_roundtrip<'m, 'd, T: Into>>(input: T, expected: Variant) { let mut builder = VariantBuilder::new(); builder.append_value(input); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap_or_else(|_| { panic!("Failed to create variant from metadata and value: {metadata:?}, {value:?}") }); @@ -1049,7 +1073,7 @@ mod tests { outer_object_builder.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_object = variant.as_object().unwrap(); @@ -1099,8 +1123,8 @@ mod tests { variant2.add_field_name("a"); assert!(!variant2.metadata_builder.is_sorted); - // per the spec, a variant must have a top-level value; finish() rejects empty - let err = variant2.finish().unwrap_err(); + // per the spec, a variant must have a top-level value; try_finish() rejects empty + let err = variant2.try_finish().unwrap_err(); assert!( err.to_string().contains("empty"), "unexpected error: {err}" @@ -1110,7 +1134,7 @@ mod tests { // write out variant1 and make sure the sorted flag is properly encoded variant1.append_value(false); - let (m, v) = variant1.finish().unwrap(); + let (m, v) = variant1.finish(); let res = Variant::try_new(&m, &v); assert!(res.is_ok()); @@ -1136,7 +1160,7 @@ mod tests { obj.insert("d", 2); obj.finish(); - let (metadata, value) = variant1.finish().unwrap(); + let (metadata, value) = variant1.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); @@ -1170,7 +1194,7 @@ mod tests { obj.insert("a", 2); obj.finish(); - let (metadata, value) = variant1.finish().unwrap(); + let (metadata, value) = variant1.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); @@ -1218,7 +1242,7 @@ mod tests { builder.append_value(42i8); // The original builder should be unchanged - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); @@ -1237,7 +1261,7 @@ mod tests { builder.append_value(42i8); // The original builder should be unchanged - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); // rolled back @@ -1260,7 +1284,7 @@ mod tests { // The parent list should only contain the original values list_builder.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); @@ -1288,7 +1312,7 @@ mod tests { builder.append_value(2i8); // Only the second attempt should appear in the final variant - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); @@ -1311,7 +1335,7 @@ mod tests { // The parent list should only contain the original values list_builder.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); @@ -1339,7 +1363,7 @@ mod tests { builder.append_value(2i8); // Only the second attempt should appear in the final variant - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); // rolled back @@ -1362,7 +1386,7 @@ mod tests { // The parent object should only contain the original fields object_builder.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert_eq!(metadata.len(), 2); @@ -1393,7 +1417,7 @@ mod tests { builder.append_value(2i8); // Only the second attempt should appear in the final variant - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert!(metadata.is_empty()); // rolled back @@ -1416,7 +1440,7 @@ mod tests { // The parent object should only contain the original fields object_builder.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert_eq!(metadata.len(), 2); // the fields of nested_object_builder has been rolled back @@ -1447,7 +1471,7 @@ mod tests { builder.append_value(2i8); // Only the second attempt should appear in the final variant - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let metadata = VariantMetadata::try_new(&metadata).unwrap(); assert_eq!(metadata.len(), 0); // rolled back @@ -1492,10 +1516,10 @@ mod tests { } list.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let v1 = Variant::try_new(&metadata, &value).unwrap(); - let (metadata, value) = VariantBuilder::new().with_value(v1.clone()).finish().unwrap(); + let (metadata, value) = VariantBuilder::new().with_value(v1.clone()).finish(); let v2 = Variant::try_new(&metadata, &value).unwrap(); assert_eq!(format!("{v1:?}"), format!("{v2:?}")); @@ -1524,7 +1548,7 @@ mod tests { } obj.finish(); } - let (metadata, value1) = builder.finish().unwrap(); + let (metadata, value1) = builder.finish(); let variant1 = Variant::try_new(&metadata, &value1).unwrap(); // Copy using the new bytes API @@ -1551,7 +1575,7 @@ mod tests { obj.insert("field4", "value4"); obj.finish(); } - let (metadata1, value1) = builder.finish().unwrap(); + let (metadata1, value1) = builder.finish(); let original_variant = Variant::try_new(&metadata1, &value1).unwrap(); let original_obj = original_variant.as_object().unwrap(); @@ -1654,7 +1678,7 @@ mod tests { root_obj.insert("total_count", 3i32); root_obj.finish(); } - let (metadata1, value1) = builder.finish().unwrap(); + let (metadata1, value1) = builder.finish(); let original_variant = Variant::try_new(&metadata1, &value1).unwrap(); let original_obj = original_variant.as_object().unwrap(); let original_users = original_obj.get("users").unwrap(); diff --git a/parquet-variant/src/builder/list.rs b/parquet-variant/src/builder/list.rs index 122aa3619dbc..5064904ca7de 100644 --- a/parquet-variant/src/builder/list.rs +++ b/parquet-variant/src/builder/list.rs @@ -294,7 +294,7 @@ mod tests { .with_value("test") .finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); assert!(!metadata.is_empty()); assert!(!value.is_empty()); @@ -332,7 +332,7 @@ mod tests { outer_list_builder.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_list = variant.as_list().unwrap(); @@ -382,7 +382,7 @@ mod tests { list_builder1.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let list1 = variant.as_list().unwrap(); @@ -422,7 +422,7 @@ mod tests { list.append_value(1.234f64); list.finish(); } - let (metadata1, value1) = builder.finish().unwrap(); + let (metadata1, value1) = builder.finish(); let original_variant = Variant::try_new(&metadata1, &value1).unwrap(); let original_list = original_variant.as_list().unwrap(); @@ -470,7 +470,7 @@ mod tests { let variant = Variant::new(&m1, &v1); let mut builder = VariantBuilder::new(); builder.append_value(variant.clone()); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); assert_eq!(variant, Variant::new(&metadata, &value)); } @@ -484,7 +484,7 @@ mod tests { .with_value("a string value") .finish(); - builder.finish().unwrap() + builder.finish() } #[test] @@ -493,7 +493,7 @@ mod tests { let variant = Variant::new(&m1, &v1); let mut builder = VariantBuilder::new(); builder.append_value(variant.clone()); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); assert_eq!(variant, Variant::new(&metadata, &value)); } @@ -509,7 +509,7 @@ mod tests { list.finish(); - builder.finish().unwrap() + builder.finish() } #[test] @@ -532,7 +532,7 @@ mod tests { list_builder.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let list = variant.as_list().unwrap(); @@ -571,7 +571,7 @@ mod tests { list_builder.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let list = variant.as_list().unwrap(); @@ -628,7 +628,7 @@ mod tests { list_builder.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let list = variant.as_list().unwrap(); @@ -717,7 +717,7 @@ mod tests { outer_list_builder.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_list = variant.as_list().unwrap(); diff --git a/parquet-variant/src/builder/object.rs b/parquet-variant/src/builder/object.rs index 9158fd30f2a1..670e3f7d768c 100644 --- a/parquet-variant/src/builder/object.rs +++ b/parquet-variant/src/builder/object.rs @@ -446,7 +446,7 @@ mod tests { .with_field("age", 42i8) .finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); assert!(!metadata.is_empty()); assert!(!value.is_empty()); } @@ -462,7 +462,7 @@ mod tests { .with_field("banana", "yellow") .finish(); - let (_, value) = builder.finish().unwrap(); + let (_, value) = builder.finish(); let header = value[0]; assert_eq!(header & 0x03, VariantBasicType::Object as u8); @@ -486,7 +486,7 @@ mod tests { .with_field("name", "Metta World Peace") // Duplicate field .finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let obj = variant.as_object().unwrap(); @@ -545,7 +545,7 @@ mod tests { builder.append_value(variant.clone()); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); assert_eq!(variant, Variant::new(&metadata, &value)); } @@ -558,7 +558,7 @@ mod tests { obj.insert("b", true); obj.insert("a", false); obj.finish(); - builder.finish().unwrap() + builder.finish() } #[test] @@ -570,7 +570,7 @@ mod tests { let mut builder = VariantBuilder::new().with_metadata(VariantMetadata::new(&m1)); builder.append_value(variant.clone()); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let result_variant = Variant::new(&metadata, &value); assert_eq!(variant, result_variant); @@ -592,7 +592,7 @@ mod tests { outer_obj.finish(); } - builder.finish().unwrap() + builder.finish() } #[test] @@ -618,7 +618,7 @@ mod tests { outer_object_builder.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_object = variant.as_object().unwrap(); @@ -661,7 +661,7 @@ mod tests { outer_object_builder.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_object = variant.as_object().unwrap(); @@ -752,7 +752,7 @@ mod tests { outer_object_builder.finish(); } - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); // note, object fields are now sorted lexigraphically by field name /* @@ -852,7 +852,7 @@ mod tests { outer_list.finish(); // Verify the nested object is built correctly -- the nested object "x" should have "won" - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); let outer_element = variant.get_list_element(0).unwrap(); let inner_element = outer_element.get_list_element(0).unwrap(); diff --git a/parquet-variant/src/variant.rs b/parquet-variant/src/variant.rs index db6f6795e9f0..58a3f7eeb261 100644 --- a/parquet-variant/src/variant.rs +++ b/parquet-variant/src/variant.rs @@ -1459,7 +1459,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # let mut obj = builder.new_object(); /// # obj.insert("name", "John"); /// # obj.finish(); - /// # builder.finish().unwrap() + /// # builder.finish() /// # }; /// // object that is {"name": "John"} /// let variant = Variant::new(&metadata, &value); @@ -1487,7 +1487,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # let mut obj = builder.new_object(); /// # obj.insert("name", "John"); /// # obj.finish(); - /// # let (metadata, value) = builder.finish().unwrap(); + /// # let (metadata, value) = builder.finish(); /// // object that is {"name": "John"} /// let variant = Variant::new(&metadata, &value); /// // use the `get_object_field` method to access the object @@ -1519,7 +1519,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # list.append_value("John"); /// # list.append_value("Doe"); /// # list.finish(); - /// # builder.finish().unwrap() + /// # builder.finish() /// # }; /// // list that is ["John", "Doe"] /// let variant = Variant::new(&metadata, &value); @@ -1578,7 +1578,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # list.append_value("John"); /// # list.append_value("Doe"); /// # list.finish(); - /// # let (metadata, value) = builder.finish().unwrap(); + /// # let (metadata, value) = builder.finish(); /// // list that is ["John", "Doe"] /// let variant = Variant::new(&metadata, &value); /// // use the `get_list_element` method to access the list @@ -1616,7 +1616,7 @@ impl<'m, 'v> Variant<'m, 'v> { /// # list.append_value("baz"); /// # list.finish(); /// # obj.finish(); - /// # let (metadata, value) = builder.finish().unwrap(); + /// # let (metadata, value) = builder.finish(); /// // given a variant like `{"foo": ["bar", "baz"]}` /// let variant = Variant::new(&metadata, &value); /// // Accessing a non existent path returns None @@ -2070,7 +2070,7 @@ mod tests { root_obj.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::try_new(&metadata, &value).unwrap(); // Test Debug formatter (?) diff --git a/parquet-variant/src/variant/list.rs b/parquet-variant/src/variant/list.rs index 1100adfd0c2d..7301d0570645 100644 --- a/parquet-variant/src/variant/list.rs +++ b/parquet-variant/src/variant/list.rs @@ -624,7 +624,7 @@ mod tests { list_builder.finish(); // Finish the builder to get the metadata and value - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); // use the Variant API to verify the result let variant = Variant::try_new(&metadata, &value).unwrap(); @@ -716,7 +716,7 @@ mod tests { let (metadata3, value3) = make_listi32(10i32..20i32); object_builder.insert("list3", Variant::new(&metadata3, &value3)); object_builder.finish(); - builder.finish().unwrap() + builder.finish() }; let variant = Variant::try_new(&metadata, &value).unwrap(); @@ -733,7 +733,7 @@ mod tests { let mut list_builder = variant_builder.new_list(); list_builder.extend(range); list_builder.finish(); - variant_builder.finish().unwrap() + variant_builder.finish() } /// return metadata/value for a simple variant list with values in a range @@ -742,6 +742,6 @@ mod tests { let mut list_builder = variant_builder.new_list(); list_builder.extend(range); list_builder.finish(); - variant_builder.finish().unwrap() + variant_builder.finish() } } diff --git a/parquet-variant/src/variant/metadata.rs b/parquet-variant/src/variant/metadata.rs index 185f6b1eabd6..d5d08d204c83 100644 --- a/parquet-variant/src/variant/metadata.rs +++ b/parquet-variant/src/variant/metadata.rs @@ -621,7 +621,7 @@ mod tests { o.finish(); - let (m, _) = b.finish().unwrap(); + let (m, _) = b.finish(); let m1 = VariantMetadata::new(&m); assert!(m1.is_sorted()); @@ -656,7 +656,7 @@ mod tests { o.finish(); - let (m, _) = b.finish().unwrap(); + let (m, _) = b.finish(); let m1 = VariantMetadata::new(&m); let m2 = VariantMetadata::new(&m); diff --git a/parquet-variant/src/variant/object.rs b/parquet-variant/src/variant/object.rs index 8905ebbaf969..bb91584cefa6 100644 --- a/parquet-variant/src/variant/object.rs +++ b/parquet-variant/src/variant/object.rs @@ -551,7 +551,7 @@ mod tests { fn test_variant_object_empty_fields() { let mut builder = VariantBuilder::new(); builder.new_object().with_field("", 42).finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); // Resulting object is valid and has a single empty field let variant = Variant::try_new(&metadata, &value).unwrap(); @@ -677,7 +677,7 @@ mod tests { } obj.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::new(&metadata, &value); if let Variant::Object(obj) = variant { @@ -738,7 +738,7 @@ mod tests { } obj.finish(); - let (metadata, value) = builder.finish().unwrap(); + let (metadata, value) = builder.finish(); let variant = Variant::new(&metadata, &value); if let Variant::Object(obj) = variant { @@ -785,7 +785,7 @@ mod tests { o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v1 = Variant::try_new(&m, &v).unwrap(); let v2 = Variant::try_new(&m, &v).unwrap(); @@ -802,7 +802,7 @@ mod tests { o.insert("b", false); o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v1 = Variant::try_new(&m, &v).unwrap(); @@ -813,7 +813,7 @@ mod tests { o.insert("b", false); o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v2 = Variant::try_new(&m, &v).unwrap(); @@ -830,7 +830,7 @@ mod tests { o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v1 = Variant::try_new(&m, &v).unwrap(); @@ -844,7 +844,7 @@ mod tests { inner_o.finish(); o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v2 = Variant::try_new(&m, &v).unwrap(); @@ -868,7 +868,7 @@ mod tests { o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v1 = Variant::try_new(&m, &v).unwrap(); @@ -881,7 +881,7 @@ mod tests { o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v2 = Variant::try_new(&m, &v).unwrap(); assert_ne!(v1, v2); @@ -897,7 +897,7 @@ mod tests { o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v1 = Variant::try_new(&m, &v).unwrap(); assert!(!v1.metadata().is_sorted()); @@ -912,7 +912,7 @@ mod tests { o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v2 = Variant::try_new(&m, &v).unwrap(); @@ -932,7 +932,7 @@ mod tests { o.finish(); - let (meta1, value1) = b.finish().unwrap(); + let (meta1, value1) = b.finish(); let v1 = Variant::try_new(&meta1, &value1).unwrap(); // v1 is sorted @@ -947,7 +947,7 @@ mod tests { o.finish(); - let (meta2, value2) = b.finish().unwrap(); + let (meta2, value2) = b.finish(); let v2 = Variant::try_new(&meta2, &value2).unwrap(); // v2 is not sorted @@ -971,7 +971,7 @@ mod tests { o.finish(); - let (m, v) = b.finish().unwrap(); + let (m, v) = b.finish(); let v1 = Variant::try_new(&m, &v).unwrap(); diff --git a/parquet-variant/tests/variant_interop.rs b/parquet-variant/tests/variant_interop.rs index 920b0a773fb2..4c7c10d51a15 100644 --- a/parquet-variant/tests/variant_interop.rs +++ b/parquet-variant/tests/variant_interop.rs @@ -310,7 +310,7 @@ fn variant_array_builder() { arr.append_value(9i8); arr.finish(); - let (built_metadata, built_value) = builder.finish().unwrap(); + let (built_metadata, built_value) = builder.finish(); let actual = Variant::try_new(&built_metadata, &built_value).unwrap(); let case = Case::load("array_primitive"); let expected = case.variant(); @@ -339,7 +339,7 @@ fn variant_object_builder() { obj.finish(); - let (built_metadata, built_value) = builder.finish().unwrap(); + let (built_metadata, built_value) = builder.finish(); let actual = Variant::try_new(&built_metadata, &built_value).unwrap(); let case = Case::load("object_primitive"); let expected = case.variant(); @@ -378,7 +378,7 @@ fn test_validation_fuzz_integration() { fn generate_random_variant(rng: &mut StdRng) -> (Vec, Vec) { let mut builder = VariantBuilder::new(); generate_random_value(rng, &mut builder, 3); // Max depth of 3 - builder.finish().unwrap() + builder.finish() } fn generate_random_value(rng: &mut StdRng, builder: &mut VariantBuilder, max_depth: u32) { From 168755fb9849b2565bcb08212e9751d64e2f0cd6 Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Thu, 28 May 2026 15:24:32 -0400 Subject: [PATCH 3/8] move `try_new_list`/`try_new_object` to `VariantBuilder` from `VariantBuidlerExt` trait --- parquet-variant/src/builder.rs | 38 +++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/parquet-variant/src/builder.rs b/parquet-variant/src/builder.rs index a4992fc095e7..942b0fa38993 100644 --- a/parquet-variant/src/builder.rs +++ b/parquet-variant/src/builder.rs @@ -825,9 +825,19 @@ impl VariantBuilder { /// # Panics /// /// Panics if a top-level variant value has already been written to this builder. For a - /// fallible version, use [`VariantBuilderExt::try_new_list`]. + /// fallible version, use [`VariantBuilder::try_new_list`]. pub fn new_list(&mut self) -> ListBuilder<'_, ()> { - VariantBuilderExt::try_new_list(self).unwrap() + self.try_new_list().unwrap() + } + + /// Create an [`ListBuilder`] for creating [`Variant::List`] values. + /// + /// Returns an error if a top-level variant value has already been written to this builder. + pub fn try_new_list(&mut self) -> Result, ArrowError> { + self.check_no_top_level_value()?; + let parent_state = + ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); + Ok(ListBuilder::new(parent_state, self.validate_unique_fields)) } /// Create an [`ObjectBuilder`] for creating [`Variant::Object`] values. @@ -837,9 +847,19 @@ impl VariantBuilder { /// # Panics /// /// Panics if a top-level variant value has already been written to this builder. For a - /// fallible version, use [`VariantBuilderExt::try_new_object`]. + /// fallible version, use [`VariantBuilder::try_new_object`]. pub fn new_object(&mut self) -> ObjectBuilder<'_, ()> { - VariantBuilderExt::try_new_object(self).unwrap() + self.try_new_object().unwrap() + } + + /// Create an [`ObjectBuilder`] for creating [`Variant::Object`] values. + /// + /// Returns an error if a top-level variant value has already been written to this builder. + pub fn try_new_object(&mut self) -> Result, ArrowError> { + self.check_no_top_level_value()?; + let parent_state = + ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); + Ok(ObjectBuilder::new(parent_state, self.validate_unique_fields)) } /// Append a value to the builder. @@ -988,17 +1008,11 @@ impl VariantBuilderExt for VariantBuilder { } fn try_new_list(&mut self) -> Result>, ArrowError> { - self.check_no_top_level_value()?; - let parent_state = - ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); - Ok(ListBuilder::new(parent_state, self.validate_unique_fields)) + self.try_new_list() } fn try_new_object(&mut self) -> Result>, ArrowError> { - self.check_no_top_level_value()?; - let parent_state = - ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); - Ok(ObjectBuilder::new(parent_state, self.validate_unique_fields)) + self.try_new_object() } } From ca42e239ec1ab0550c732d40563abce753b935ca Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Thu, 28 May 2026 16:20:56 -0400 Subject: [PATCH 4/8] add inline for perf --- parquet-variant/src/builder.rs | 47 ++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/parquet-variant/src/builder.rs b/parquet-variant/src/builder.rs index 942b0fa38993..0d55ef848433 100644 --- a/parquet-variant/src/builder.rs +++ b/parquet-variant/src/builder.rs @@ -52,6 +52,31 @@ pub(crate) fn int_size(v: usize) -> OffsetSizeBytes { } } +const ONE_TOP_LEVEL_VALUE_MSG: &str = + "VariantBuilder already contains a top-level variant value; only one is allowed"; +const EMPTY_BUILDER_MSG: &str = + "VariantBuilder is empty; append a top-level value before calling finish()"; + +// Error/panic construction is kept out-of-line and cold so the per-call guards on the +// builder hot paths stay small enough to inline as a single predictable branch. +#[cold] +#[inline(never)] +fn top_level_value_panic() -> ! { + panic!("{ONE_TOP_LEVEL_VALUE_MSG}"); +} + +#[cold] +#[inline(never)] +fn top_level_value_error() -> ArrowError { + ArrowError::InvalidArgumentError(ONE_TOP_LEVEL_VALUE_MSG.into()) +} + +#[cold] +#[inline(never)] +fn empty_builder_error() -> ArrowError { + ArrowError::InvalidArgumentError(EMPTY_BUILDER_MSG.into()) +} + /// Wrapper around a `Vec` that provides methods for appending /// primitive values, variant types, and metadata. /// @@ -797,23 +822,22 @@ impl VariantBuilder { /// A [`VariantBuilder`] holds exactly one top-level variant value. Any committed top-level /// value leaves bytes in the value buffer; a child builder dropped without `finish()` has /// its bytes rolled back by [`ParentState`], so the offset is a faithful indicator. + #[inline] fn has_top_level_value(&self) -> bool { self.value_builder.offset() != 0 } + #[inline] fn ensure_no_top_level_value(&self) { - assert!( - !self.has_top_level_value(), - "VariantBuilder already contains a top-level variant value; only one is allowed" - ); + if self.has_top_level_value() { + top_level_value_panic(); + } } + #[inline] fn check_no_top_level_value(&self) -> Result<(), ArrowError> { if self.has_top_level_value() { - return Err(ArrowError::InvalidArgumentError( - "VariantBuilder already contains a top-level variant value; only one is allowed" - .into(), - )); + return Err(top_level_value_error()); } Ok(()) } @@ -933,8 +957,7 @@ impl VariantBuilder { /// Panics if no top-level variant value has been appended. For a fallible version, use /// [`VariantBuilder::try_finish`]. pub fn finish(self) -> (Vec, Vec) { - self.try_finish() - .expect("VariantBuilder is empty; append a top-level value before calling finish()") + self.try_finish().expect(EMPTY_BUILDER_MSG) } /// Finish the builder and return the metadata and value buffers. @@ -942,9 +965,7 @@ impl VariantBuilder { /// Returns an error if no top-level variant value has been appended. pub fn try_finish(mut self) -> Result<(Vec, Vec), ArrowError> { if !self.has_top_level_value() { - return Err(ArrowError::InvalidArgumentError( - "VariantBuilder is empty; append a top-level value before calling finish()".into(), - )); + return Err(empty_builder_error()); } self.metadata_builder.finish(); Ok(( From b5aeaaf46171bed6d2dc4030765e836738223128 Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Thu, 28 May 2026 16:24:12 -0400 Subject: [PATCH 5/8] fmt --- parquet-variant/src/builder.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/parquet-variant/src/builder.rs b/parquet-variant/src/builder.rs index 0d55ef848433..a6a33eafd4d9 100644 --- a/parquet-variant/src/builder.rs +++ b/parquet-variant/src/builder.rs @@ -883,7 +883,10 @@ impl VariantBuilder { self.check_no_top_level_value()?; let parent_state = ParentState::variant(&mut self.value_builder, &mut self.metadata_builder); - Ok(ObjectBuilder::new(parent_state, self.validate_unique_fields)) + Ok(ObjectBuilder::new( + parent_state, + self.validate_unique_fields, + )) } /// Append a value to the builder. @@ -1160,10 +1163,7 @@ mod tests { // per the spec, a variant must have a top-level value; try_finish() rejects empty let err = variant2.try_finish().unwrap_err(); - assert!( - err.to_string().contains("empty"), - "unexpected error: {err}" - ); + assert!(err.to_string().contains("empty"), "unexpected error: {err}"); } // write out variant1 and make sure the sorted flag is properly encoded From f372e3ae4ef4d50ef6ae3be83effaae4caa30faf Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Thu, 28 May 2026 16:54:09 -0400 Subject: [PATCH 6/8] clippy --- parquet-variant/src/builder/metadata.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parquet-variant/src/builder/metadata.rs b/parquet-variant/src/builder/metadata.rs index 1ff5f6da9ba1..802d0904c1c8 100644 --- a/parquet-variant/src/builder/metadata.rs +++ b/parquet-variant/src/builder/metadata.rs @@ -268,7 +268,7 @@ impl> Extend for WritableMetadataBuilder { #[cfg(test)] mod test { use crate::{ - ParentState, ValueBuilder, Variant, VariantBuilder, VariantMetadata, + ParentState, ValueBuilder, Variant, VariantMetadata, builder::{ metadata::{ReadOnlyMetadataBuilder, WritableMetadataBuilder}, object::ObjectBuilder, From 196255fb46a9830028c00e01022002bd5a66d40f Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Wed, 10 Jun 2026 09:23:49 -0400 Subject: [PATCH 7/8] remove duplicate test --- parquet-variant/src/builder/metadata.rs | 38 +------------------------ 1 file changed, 1 insertion(+), 37 deletions(-) diff --git a/parquet-variant/src/builder/metadata.rs b/parquet-variant/src/builder/metadata.rs index 802d0904c1c8..10b34ced03a6 100644 --- a/parquet-variant/src/builder/metadata.rs +++ b/parquet-variant/src/builder/metadata.rs @@ -268,7 +268,7 @@ impl> Extend for WritableMetadataBuilder { #[cfg(test)] mod test { use crate::{ - ParentState, ValueBuilder, Variant, VariantMetadata, + ParentState, ValueBuilder, VariantMetadata, builder::{ metadata::{ReadOnlyMetadataBuilder, WritableMetadataBuilder}, object::ObjectBuilder, @@ -356,42 +356,6 @@ mod test { assert_eq!(metadata.num_field_names(), 3); } - #[test] - fn test_read_only_metadata_builder() { - // First create some metadata with a few field names - let mut default_builder = WritableMetadataBuilder::default(); - default_builder.upsert_field_name("name"); - default_builder.upsert_field_name("age"); - default_builder.upsert_field_name("active"); - default_builder.finish(); - let metadata_bytes = default_builder.into_inner(); - - // Use the metadata to build new variant values - let metadata = VariantMetadata::try_new(&metadata_bytes).unwrap(); - let mut metadata_builder = ReadOnlyMetadataBuilder::new(&metadata); - let mut value_builder = ValueBuilder::new(); - - { - let state = ParentState::variant(&mut value_builder, &mut metadata_builder); - let mut obj = ObjectBuilder::new(state, false); - - // These should succeed because the fields exist in the metadata - obj.insert("name", "Alice"); - obj.insert("age", 30i8); - obj.insert("active", true); - obj.finish(); - } - - let value = value_builder.into_inner(); - - // Verify the variant was built correctly - let variant = Variant::try_new(&metadata_bytes, &value).unwrap(); - let obj = variant.as_object().unwrap(); - assert_eq!(obj.get("name"), Some(Variant::from("Alice"))); - assert_eq!(obj.get("age"), Some(Variant::Int8(30))); - assert_eq!(obj.get("active"), Some(Variant::from(true))); - } - #[test] fn test_read_only_metadata_builder_fails_on_unknown_field() { // Create metadata with only one field From 63724871d46326c2a4b8bb1ae4bdc6588d13695a Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Thu, 18 Jun 2026 17:01:05 -0400 Subject: [PATCH 8/8] add unit tests fixing the behavior --- parquet-variant/src/builder.rs | 90 ++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/parquet-variant/src/builder.rs b/parquet-variant/src/builder.rs index a6a33eafd4d9..b304dd810222 100644 --- a/parquet-variant/src/builder.rs +++ b/parquet-variant/src/builder.rs @@ -1081,6 +1081,96 @@ mod tests { assert_eq!(variant, expected); } + #[test] + fn test_try_finish_empty_builder_errors() { + let builder = VariantBuilder::new(); + let err = builder.try_finish().unwrap_err(); + assert!(err.to_string().contains("empty"), "unexpected error: {err}"); + } + + #[test] + #[should_panic(expected = "empty")] + fn test_finish_empty_builder_panics() { + let builder = VariantBuilder::new(); + let _ = builder.finish(); + } + + #[test] + fn test_try_append_value_after_value_errors() { + let mut builder = VariantBuilder::new(); + builder.append_value(1i32); + let err = builder.try_append_value(2i32).unwrap_err(); + assert!( + err.to_string().contains("only one is allowed"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_try_append_value_bytes_after_value_errors() { + let mut builder = VariantBuilder::new(); + builder.append_value(1i32); + let err = builder.try_append_value_bytes(2i32).unwrap_err(); + assert!( + err.to_string().contains("only one is allowed"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_try_new_list_after_value_errors() { + let mut builder = VariantBuilder::new(); + builder.append_value(1i32); + let err = builder.try_new_list().expect_err("expected error"); + assert!( + err.to_string().contains("only one is allowed"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_try_new_object_after_value_errors() { + let mut builder = VariantBuilder::new(); + builder.append_value(1i32); + let err = builder.try_new_object().expect_err("expected error"); + assert!( + err.to_string().contains("only one is allowed"), + "unexpected error: {err}" + ); + } + + #[test] + #[should_panic(expected = "only one is allowed")] + fn test_append_value_after_value_panics() { + let mut builder = VariantBuilder::new(); + builder.append_value(1i32); + builder.append_value(2i32); + } + + #[test] + #[should_panic(expected = "only one is allowed")] + fn test_append_value_bytes_after_value_panics() { + let mut builder = VariantBuilder::new(); + builder.append_value(1i32); + builder.append_value_bytes(2i32); + } + + #[test] + #[should_panic(expected = "only one is allowed")] + fn test_new_list_after_value_panics() { + let mut builder = VariantBuilder::new(); + builder.append_value(1i32); + let _ = builder.new_list(); + } + + #[test] + #[should_panic(expected = "only one is allowed")] + fn test_new_object_after_value_panics() { + let mut builder = VariantBuilder::new(); + builder.append_value(1i32); + let _ = builder.new_object(); + } + #[test] fn test_nested_object_with_lists() { /*