alamb commented on code in PR #8324:
URL: https://github.com/apache/arrow-rs/pull/8324#discussion_r2346439492
##########
parquet-variant/src/builder.rs:
##########
@@ -736,34 +748,68 @@ impl BuilderSpecificState for ObjectParentState<'_> {
/// the parent, and we cannot "split" a mutable reference across two objects
(parent state and the
/// child builder that uses it). So everything has to be here.
#[derive(Debug)]
-pub struct ParentState<'a> {
+pub struct ParentState<'a, S: BuilderSpecificState> {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
metadata_builder: &'a mut dyn MetadataBuilder,
saved_metadata_builder_dict_size: usize,
- builder_state: Box<dyn BuilderSpecificState + 'a>,
+ builder_state: S,
Review Comment:
❤️
##########
parquet-variant/src/builder.rs:
##########
@@ -736,34 +748,68 @@ impl BuilderSpecificState for ObjectParentState<'_> {
/// the parent, and we cannot "split" a mutable reference across two objects
(parent state and the
/// child builder that uses it). So everything has to be here.
#[derive(Debug)]
-pub struct ParentState<'a> {
+pub struct ParentState<'a, S: BuilderSpecificState> {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
metadata_builder: &'a mut dyn MetadataBuilder,
saved_metadata_builder_dict_size: usize,
- builder_state: Box<dyn BuilderSpecificState + 'a>,
+ builder_state: S,
finished: bool,
}
-impl<'a> ParentState<'a> {
+impl<'a, S: BuilderSpecificState> ParentState<'a, S> {
/// Creates a new ParentState instance. The value and metadata builder
/// state is checkpointed and will roll back on drop, unless
[`Self::finish`] is called. The
/// builder-specific state is governed by its own `finish` and `rollback`
calls.
- pub fn new<T: BuilderSpecificState + 'a>(
+ pub fn new(
value_builder: &'a mut ValueBuilder,
metadata_builder: &'a mut dyn MetadataBuilder,
- builder_state: T,
+ builder_state: S,
) -> Self {
- ParentState {
+ Self {
saved_value_builder_offset: value_builder.offset(),
value_builder,
saved_metadata_builder_dict_size:
metadata_builder.num_field_names(),
metadata_builder,
- builder_state: Box::new(builder_state),
+ builder_state,
finished: false,
}
}
+ /// Marks the insertion as having succeeded and invokes
+ /// [`BuilderSpecificState::finish`]. Internal state will no longer roll
back on drop.
Review Comment:
I found the reference to the past ("no longer") a little confusing
```suggestion
/// [`BuilderSpecificState::finish`].
///
/// Note: Does not call `rollback()` on drop
```
--
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]
