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
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 gets messy enough (including a bunch of methods
that are only defined for specific combinations of types) that I lean strongly
toward keeping the `dyn` indirection unless we're _really_ certain we need it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
