alamb commented on code in PR #7914:
URL: https://github.com/apache/arrow-rs/pull/7914#discussion_r2202872795
##########
parquet-variant/src/builder.rs:
##########
@@ -2170,4 +2251,45 @@ mod tests {
let variant = Variant::try_new_with_metadata(metadata,
&value).unwrap();
assert_eq!(variant, Variant::Int8(2));
}
+
+ #[test]
+ fn test_append_object() {
+ let (m1, v1) = make_object();
+ let variant = Variant::new(&m1, &v1);
+
+ let mut builder = VariantBuilder::new();
+ builder.append_value(variant.clone());
+ let (metadata, value) = builder.finish();
+ assert_eq!(variant, Variant::new(&metadata, &value));
+ }
+
+ /// make an object variant
+ fn make_object() -> (Vec<u8>, Vec<u8>) {
+ let mut builder = VariantBuilder::new();
+
+ let mut obj = builder.new_object();
+ obj.insert("a", true);
+ obj.finish().unwrap();
+ builder.finish()
+ }
+
+ #[test]
+ fn test_append_list() {
Review Comment:
I suggest also adding a test with more than one level of nesting (aka a list
of objects) to make sure the recursion works correctly
##########
parquet-variant/src/builder.rs:
##########
@@ -213,14 +215,14 @@ impl ValueBuffer {
Variant::Binary(v) => self.append_binary(v),
Variant::String(s) => self.append_string(s),
Variant::ShortString(s) => self.append_short_string(s),
- Variant::Object(_) | Variant::List(_) => {
- unreachable!(
- "Nested values are handled specially by ObjectBuilder and
ListBuilder"
- );
- }
+ _ => unreachable!("Objects and lists must be appended using
VariantBuilder::append_object and VariantBuilder::append_list"),
Review Comment:
WHy not call `self.append_variant_object` and `self.append_variant_list`
here (to avoid the panic?)
##########
parquet-variant/src/builder.rs:
##########
@@ -697,6 +699,79 @@ impl VariantBuilder {
ObjectBuilder::new(parent_state, validate_unique_fields)
}
+ /// Appends a [`VariantObject`] to the builder.
+ ///
+ /// # Panics
+ /// Will panic if the appended object has duplicate field names or any
nested validation fails.
+ /// Use `try_append_object` if you need full validation for untrusted data.
+ pub fn append_object<'m, 'v>(&mut self, object: VariantObject<'m, 'v>) {
Review Comment:
As a user I don't want to have to already have a `VariantObject` when
appending -- I would like to just append the `Variant` directly.
I suggest we keep `append_object, `try_append_object`, `append_list` and
`try_append_list` as implementation detail (aka a non `pub` method)
##########
parquet-variant/src/builder.rs:
##########
@@ -707,7 +782,13 @@ impl VariantBuilder {
/// builder.append_value(42i8);
/// ```
pub fn append_value<'m, 'd, T: Into<Variant<'m, 'd>>>(&mut self, value: T)
{
- self.buffer.append_non_nested_value(value);
+ let variant = value.into();
+
+ match variant {
+ Variant::Object(obj) => self.append_object(obj),
Review Comment:
Yes, I agree -- I thik it would make a lot of sense to have a
`try_append_value` that returns an error (rather than panic'ing) if an invalid
variant is passed in.
Then `append_value` would just call `self try_append_value(v).unwrap()` 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
