cc @alamb

👋 @viirya -- I am checking this one out

FYI @friendlymatthew and @scovich and @micoo227 who just added the validation code today!

I also wrote up some additional documentation and examples for your consideration here:

Thank you. I merged it.

This change makes me nervous, after having been bitten in the past -- multiple times in multiple projects and multiple languages -- by unconditional finalizations like this that don't consider whether the the finalization is actually desirable. The most recent was a bad interaction between spark and hadoop, where the upload stream's unconditional auto-close behavior meant that early return due to an exception uploaded a partial object to cloud storage (= a torn write that caused havoc to readers who don't expect torn writes in cloud storage to be possible). The offending code was buried many levels deep, which made it a real treat to root-cause, and even more "fun" to workaround.

Having any object attempt to "finish" in spite of a panic=unwind, or even a ? return would be unexpected and unwelcome behavior IMO. And changing a Result into a panic inside drop glue is even worse (double fault risk).

If the worry is that somebody might forget to invoke the finish method... can we try to figure out other ways to address it, which don't resort to implicit/spooky behavior?

Making sense. I forgot that this is not a good practice to put the finalization login into Drop under panic=unwind.

I reverted the Drop impl and related comments and tests added by @alamb. Just keep some comments unrelated to Drop and the small fix to the original tests.

We can figure out the other ways to address that.

Hmm, actually, with an empty Drop, it can catch up the bug in the test like I mentioned in #7843 (comment). So it doesn't do any "finish" stuff or panic in Drop.

In other words, it can achieve the effort to address the original issue without the worries.

So I personally prefer the automatic finalize on drop behavior -- I can understand how that behavior can cause trouble when external resources are affected but in this case I think it makes this code much easier to use for most cases.

If we don't finalize the object on drop of the builder, for example, the resulting Variant object built by the builder seems to be malformed. Here is an example that would work with finalize on drop, but not with this PR as is

#[test]
    fn test_object_list_no_finish() {
        let mut builder = VariantBuilder::new();

        let mut list_builder = builder.new_list();

        {
            let mut object_builder = list_builder.new_object();
            object_builder.insert("id", 1);
            object_builder.finish().unwrap();
        }

        {
            let mut object_builder = list_builder.new_object();
            object_builder.insert("id", 2);
            // NOTE object_builder.finish() is not called here
        }

        list_builder.finish();

        let (metadata, value) = builder.finish();

        let variant = Variant::try_new(&metadata, &value).unwrap();
        let list = variant.as_list().unwrap();
}

If object_builder.finish() is not called, then this line fails:

        let list = variant.as_list().unwrap();

The error is like this (which is pretty unintuitive to me):

called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Received empty bytes")
thread 'builder::tests::test_object_list_no_finish' panicked at parquet-variant/src/builder.rs:1154:59:
called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Received empty bytes")
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/6b00bc3880198600130e1cf62b8f8a93494488cc/library/std/src/panicking.rs:697:5
   1: core::panicking::panic_fmt
             at /rustc/6b00bc3880198600130e1cf62b8f8a93494488cc/library/core/src/panicking.rs:75:14
   2: core::result::unwrap_failed
             at /rustc/6b00bc3880198600130e1cf62b8f8a93494488cc/library/core/src/result.rs:1732:5
   3: core::result::Result<T,E>::unwrap
             at /Users/andrewlamb/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:1137:23
   4: parquet_variant::builder::tests::test_object_list_no_finish
             at ./src/builder.rs:1154:23
   5: parquet_variant::builder::tests::test_object_list_no_finish::{{closure}}
             at ./src/builder.rs:1133:36
   6: core::ops::function::FnOnce::call_once
             at /Users/andrewlamb/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/6b00bc3880198600130e1cf62b8f8a93494488cc/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

So TLDR I still prefer the finalize on drop behavior

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)

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 🤔

From what I understood, the example you gave above would trivially defeat an impl Drop sentinel (= empty drop method), because the object_builder goes out of scope before the call to list_builder.finish():

        let mut builder = VariantBuilder::new();
        let mut list_builder = builder.new_list();

        {
            let mut object_builder = list_builder.new_object();
            object_builder.insert("id", 1);
            // NOTE object_builder.finish() is not called here
        }

        list_builder.finish();
        let (metadata, value) = builder.finish();        

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

Doesn't that mean our current implementation is not even safe in the presence of early return by e.g. ?, let alone UnwindSafe? Any error at any nesting depth must be treated as unrecoverable, and the entire variant builder discarded? e.g. instead of this:

        {
            let mut object_builder = list_builder.new_object();
            object_builder.insert("id", 2);
            // NOTE object_builder.finish() is not called here
        }

imagine this:

        fn try_append(...) -> Result<(), ArrowError> {
            let mut object_builder = list_builder.new_object();
            object_builder.insert("id", 2);

            // some fallible operation whose `?` could return early

            object_builder.finish()?; // an err here also renders the parent unusable
            Ok(())
        }

With the current behavior, the caller of try_append is forced to fail the entire variant encoding attempt, for fear that it might have been left in an inconsistent state. And if they forget to treat some Err as fatal... boom. At a minimum, it seems like we should "poison" the parent builder so it knows this happened and its (now fallible) finish can fail cleanly? It should also refuse to create new sub-builders or append new values, to avoid wasting work. But that's a lot of work to code up...

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)

Honestly, now that the nested builders store their bytes in a separate vec, I would have expected rollback safety for free. But I guess we're still storing something in the parent vec. I would imagine that's easily addressed by having the constructor grab a marker, which finish advances; then drop can simply truncate the vec to that mark. Seems simpler than safely poisoning it, and safer than relying on callers to maintain an invisible (and unusual) error handling invariant?

No need to remove extra entries from the metadata dictionary, they're harmless.

So I personally prefer the automatic finalize on drop behavior -- I can understand how that behavior can cause trouble when external resources are affected but in this case I think it makes this code much easier to use for most cases.

Relating to the ?-safety and unwind safety comment above: unconditionally finalizing on drop (hopefully) produces a physically valid variant -- but the result is still a logically invalid "torn write". That kind of spookiness is really hard to reason around when coding, and even harder to triage when something goes wrong. As I mentioned, I've hit this issue many times over the years, in various forms. It's always surprising and never in a good way.

Honestly, now that the nested builders store their bytes in a separate vec, I would have expected rollback safety for free. But I guess we're still storing something in the parent vec. I would imagine that's easily addressed by having the constructor grab a marker, which finish advances; then drop can simply truncate the vec to that mark. Seems simpler than safely poisoning it, and safer than relying on callers to maintain an invisible (and unusual) error handling invariant?

Looking at the code, I think the bad behavior comes from the pending concept that the list and object builders employ. In a sense, they are unconditionally finalizing the builders (any subsequent method call checks for a pending builder)... but the finalization is only partial if the builder didn't actually finalize. Meanwhile, the variant builder itself lacks any such "pending" mechanism, which likely allows a different set of problems. In fact, it looks like the variant builder allows to "append" any number of variant values -- almost a column-oriented builder concept, but lacking any mechanism for tracking objects and the boundaries between them.

Relating to the ?-safety and unwind safety comment above: unconditionally finalizing on drop (hopefully) produces a physically valid variant -- but the result is still a logically invalid "torn write". That kind of spookiness is really hard to reason around when coding, and even harder to triage when something goes wrong. As I mentioned, I've hit this issue many times over the years, in various forms. It's always surprising and never in a good way.

The idea of a logically lost object being hard to debug makes sense to me. Thank you -- I missed that

Propsal

1. Add a function like ObjectBuilder::rollback to explicitly "cancel" the currently in progress object
2. Implement ObjectBuilder::drop such that it calls ObjectBuilder::rollback if ObjectBuilder::build() has not been called

That way:

1. No way to get accidentally completed ("torn") objects
2. No panics on drop
3. No partially / corrupt objects if finish isn't called

To be clear, it doesn't mean that @viirya needs to fix this in this PR (though if he does that would be great)

Also, I filed a separate ticket for corrupted objects if build() is not called;
-#7863

To be clear, it doesn't mean that @viirya needs to fix this in this PR (though if he does that would be great)

Sounds great to me. Is @scovich okay for this proposal? If so, I can work it on this PR too.

To be clear, it doesn't mean that @viirya needs to fix this in this PR (though if he does that would be great)

Sounds great to me. Is @scovich okay for this proposal? If so, I can work it on this PR too.

Oi... I didn't see this comment, and I already have a PR almost ready to go.

NOTE: The sentinel impl Drop in this PR will still be useful -- it will detect the missing finish calls in the new unit tests I'm adding (requiring them to drop the unfinished builders).

Then maybe we can merge this now?

Thanks @alamb @scovich