klion26 commented on PR #8031:
URL: https://github.com/apache/arrow-rs/pull/8031#issuecomment-3146577046
@scovich
> If you hit a buffer overflow panic, I think that means you tried to use
the "fast" version to write the Some(data_size) entry, after the loop exits. Or
did you hit a problem in the loop itself?
It would panic in the following test when creating a variant with
`VariantBuilder::new`.
```
let (m1, v1) = make_nested_object();
let variant = Variant::new(&m1, &v1);
// because we can guarantee metadata is validated through the builder
let mut builder =
VariantBuilder::new().with_metadata(VariantMetadata::new(&m1));
builder.append_value(variant.clone()); <-- panic here because we
have a buffer with size 6 and will try to copy 4 bytes to the buffer starting
from 3
let (metadata, value) = builder.finish();
let result_variant = Variant::new(&metadata, &value);
assert_eq!(variant, result_variant);
```
> The same argument likely applies to the object builder. Should we consider
reverting that back to match the list builder, for simplicity and
maintainability? I'd have to go back and check the other PR, but was the perf
improvement actually significant there?
Sorry, I've rechecked the current `ObjectBuilder::finish` logic(I
misremembered the implementation in main branch before). I think either the
current approach or aligning it with ListBuilder::finish is fine for me. The
current approach has some possibility of generating bugs as @scovich said in
the previous comment, but for now we have ut which ensures correctness.
Aligning the logic with ListBuilder::finish will fundamentally ensure
correctness(the benchmark difference is minimal). @alamb @scovich, I can modify
the code if necessary.
--
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
