taepper commented on code in PR #51108:
URL: https://github.com/apache/arrow/pull/51108#discussion_r4335247319
##########
cpp/src/arrow/json/parser.cc:
##########
@@ -526,50 +526,51 @@ class RawBuilderSet {
/// Appending null is slightly tricky since null count is stored inline
/// for builders of Kind::kNull. Append nulls using this helper
- Status AppendNull(BuilderPtr parent, int field_index, BuilderPtr builder) {
- if (ARROW_PREDICT_FALSE(!builder.nullable)) {
+ Status AppendNull(BuilderPtr parent, int field_index, BuilderPtr* builder) {
Review Comment:
We already pass `BuilderPtr parent`, and `int field_index` identifies the
child `builder` within `parent`. It is an anti-pattern to also pass the builder
along.
Also AFAICT the bug was not actually that the update was lost, we do update
the parent and acknowledge that the builder's value will be dropped after the
scope:
> `// update the parent, since changing builder doesn't affect parent`
Instead the root cause was that we store two `BuilderPtr` _outside_ this
function. The `builder_` and `builder_stack_.back().<child at current field
index>`, and _both_ need to be updated.
I think to avoid this bug in the future we should change the signature to
reflect what needs to change:
The `AppendNull()` method should remove the third parameter and instead
return `Result<BuilderPtr>` to signify that the value needs to be used to
update the outside `BuilderPtr`.
The problem with the current fix is that a new caller that has a `parent`
can just call the function with `AppendNull(parent, field_index,
ChildAtFieldIndex(parent, field_index))` and still get the same bug.
Also 3 of the 4 call sites of `AppendNull` only create a temporary `builder`
to pass as the third argument which is dropped immediately afterwards. Making
the third parameter and in-out-parameter will make those call sites look like
they are incorrect
--
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]
