scovich commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2296013459
##########
parquet-variant/src/builder.rs:
##########
@@ -589,14 +675,14 @@ enum ParentState<'a> {
Variant {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
- metadata_builder: &'a mut MetadataBuilder,
+ metadata_builder: &'a mut dyn MetadataBuilder,
Review Comment:
Yeah I was trying to avoid the "pollution" that generics would cause, which
is clearly visible in my first attempt:
* https://github.com/apache/arrow-rs/pull/7915
It's not just `ParentState`, but also `ListBuilder`, `ObjectBuilder` and
even the `VariantBuilderExt` trait. Plus it requires a bunch of methods that
are only defined for specific combinations of types.
But we may need to go generic now, because
`VariantArrayVariantBuilder::finish` needs access to
`BasicMetadataBuilder::finish` in order to work correctly, and that's
unavailable because the parent state has upcast it to `&mut dyn
MetadataBuilder`.
I initially reached for `std::any::AsAny`, but it turns out that types with
non-static lifetimes cannot implement `std::any::Any` because the lifetime
information would be lost.
In order to get the latest merge with upstream to compile, I had to define a
new `MetadataBuilder::finish` trait method to expose the underlying
`BasicMetadataBuilder::finish`, but that also forced a no-op
`ReadOnlyMetadataBuilder::finish`.
Honestly, the generic mess is messy enough that I lean strongly toward
keeping the `dyn` indirection unless we're _really_ certain we need generics
for performance or functionality reasons.
--
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
