alamb commented on PR #7843: URL: https://github.com/apache/arrow-rs/pull/7843#issuecomment-3032539562
> I'm neutral about this change. Not convinced it's actually useful in practice, but it also seems harmless enough so there's no reason to block it. > > **Against**: Outside unit tests, recursive builders will almost always live in a nested scope vs. their parent -- usually a called function or method, or at least a `match` clause -- which would trivially defeat the drop sentinel. I don't understand what you mean by defeat the drop sentinel. Since the `ObjectBuilder` has a `mut` reference to its parent, I don't think the rust compiler will allow anything to the same underlying parent builder Maybe I am missing something here and an example would help 🤔 > **For**: The sentinel is helpful when it _is_ able to catch an issue, and harmless the rest of the time. In the uncommon case that somebody actually _wants_ to avoid calling `finish`, they can just `drop` the builder (which also documents the intent). My example above shows that not dropping the builder can lead to a corrupted output so I don't think we support the case of avoiding calling finish without some more work (to be clear I am not sure the "rollback a partially built object" s a valuable API yet) > **For**: Like it or not, unit tests will end up being a very significant consumer of the variant API, so "quality of life" features that make unit tests easier to write and maintain are not necessarily a bad thing. > **Against**: Suppose a unit test does have a bug in forgetting to `finish` a builder. I'm very unclear on what was being tested, if the resulting variant wasn't checked in a way that would notice big swaths of missing data? Even if the drop sentinel catches it, the test is still arguably broken/useless due to incomplete verification of what it claims to test. I agree the tests could be improved and we should add a test showing what happens if you don't call `finish` (I have an example above) > **Against**: A quick grep over the rest of arrow-rs suggests that none of the other crates' builders employ drop sentinels like this. The only matches for `impl.* Drop ` are for buffers and FFI wrappers (to manually delete internal pointers). Another difference to the other builders is they don't have this model of nested nested builders -- so there is no "parent" state to finalize. Variant is somewhat different than the other arrow types because its nesting level camn be arbitary, which is what these nested builders allow) -- 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]
