[Variant] Caller provides ParentState to ValueBuilder methods #8189
Conversation
| Variant::Uuid(v) => self.append_uuid(v), | ||
| Variant::Object(obj) => self.append_object(metadata_builder, obj), | ||
| Variant::List(list) => self.append_list(metadata_builder, list), | ||
| Variant::Time(v) => self.append_time_micros(v), |
There was a problem hiding this comment.
FYI I moved Time up by Date for better logical grouping.
(again below)
| Variant::Float(v) => self.append_float(v), | ||
| Variant::Double(v) => self.append_double(v), | ||
| Variant::Binary(v) => self.append_binary(v), | ||
| Variant::Uuid(v) => self.append_uuid(v), |
There was a problem hiding this comment.
FYI I moved Uuid below ShortString to match append_variant above
|
@codephage2020 and @klion26 -- this is a fast-follow PR to the one you just approved. |
| Variant::Object(obj) => return Self::append_object(state, obj), | ||
| Variant::List(list) => return Self::append_list(state, list), | ||
| } | ||
| state.finish(); |
There was a problem hiding this comment.
Not sure if it's better to move this line to the caller, the finish now is associated with ParentState, maybe creating and finishing in the same place is better?
There was a problem hiding this comment.
The nested builders have to take ownership of their parent state (else many call sites would illegally return a reference to a temporary). So here, we have an owned value that sometimes needs finish and sometimes doesn't. Returning Option<ParentState<'_>> seems ugly and annoying?
There was a problem hiding this comment.
If there is interest in trying the alternative, how about we make a follow on PR
|
@alamb -- ready for final review+merge |
There was a problem hiding this comment.
Looks like an improvement to me -- thank you @scovich and @klion26 and @codephage2020
Let's keep the code flowing and we can continue improving it in follow on PRs
As you wish! |
Which issue does this PR close?
ValueBuilderAPI to work withParentStatefor reliable nested rollbacks #8188Rationale for this change
Today,
ValueBuilder::[try_]append_variantunconditionally creates and uses aParentState::Variant, but that is incorrect when the caller is aListBuilderorObjectBuilder. Rework the API so that the caller passes their parent state, thus ensuring proper rollback in all situations.This is also a building block that will eventually let us simplify
VariantArrayBuilderto use aValueBuilderdirectly, instead of aVariantBuilder.What changes are included in this PR?
Several methods become associated functions.
Are these changes tested?
Existing unit tests cover this refactor.
Are there any user-facing changes?
No