alamb commented on code in PR #8135:
URL: https://github.com/apache/arrow-rs/pull/8135#discussion_r2276429069
##########
parquet-variant/src/builder.rs:
##########
@@ -689,7 +689,7 @@ impl ParentState<'_> {
}
| ParentState::List {
metadata_builder, ..
- } => metadata_builder.metadata_buffer.len(),
+ } => metadata_builder.field_names.len(),
Review Comment:
Given this function now returns something different, I think we should also
rename it to reflect the new behavior
Perhaps instead of `metadata_current_offset` something like
`metadata_num_fields` ?
##########
parquet-variant/src/builder.rs:
##########
@@ -1021,6 +1021,28 @@ impl VariantBuilder {
self
}
+ /// Builder-style API for appending a value to the list and returning self
to enable method chaining.
+ ///
+ /// # Panics
+ ///
+ /// This method will panic if the variant contains duplicate field names
in objects
+ /// when validation is enabled. For a fallible version, use
[`ListBuilder::try_with_value`].
+ pub fn with_value<'m, 'd, T: Into<Variant<'m, 'd>>>(mut self, value: T) ->
Self {
Review Comment:
nice!
##########
parquet-variant/src/variant.rs:
##########
@@ -1286,6 +1286,59 @@ impl TryFrom<(i128, u8)> for Variant<'_, '_> {
}
}
+impl std::fmt::Debug for Variant<'_, '_> {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ Variant::Null => write!(f, "null"),
Review Comment:
I think for the debug printing we should also include the variant enum,
otherwise one wouldn't be able to tell the difference between
`Variant::Int8(42)` and `Variant::Int16(42)` from the debug output, as they
would both be rendered as `42`
So perhaps we can keep
```suggestion
Variant::Null => write!(f, "Variant::Null"),
```
And then apply some nicer formatting to the binary / object ones
##########
parquet-variant/src/builder.rs:
##########
@@ -3121,4 +3151,265 @@ mod tests {
builder.finish()
}
+
+ #[test]
+ fn test_append_list_object() {
Review Comment:
I agree -- let's keep only `test_append_list_object_list_object`
It may also be worth adding a comment or two highlighting what it is trying
to cover (namely nested object and list construction when some of the nested
objects are not finished)
##########
parquet-variant/src/builder.rs:
##########
@@ -2979,13 +3005,17 @@ mod tests {
// The parent object should only contain the original fields
object_builder.finish().unwrap();
let (metadata, value) = builder.finish();
+ println!("value: {:#?}", Variant::new(&metadata, &value));
Review Comment:
do you think we should leave the `println` in the test? Or can we remove it
now that you have fixed the tests?
##########
parquet-variant/src/builder.rs:
##########
@@ -2928,13 +2950,17 @@ mod tests {
// The parent object should only contain the original fields
object_builder.finish().unwrap();
let (metadata, value) = builder.finish();
+ println!("value: {:#?}", Variant::new(&metadata, &value));
Review Comment:
do we still need the `println`?
--
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
