Skip to content

[Variant] Improve ergonomics when using VariantBuilder #9113

Description

@BlakeOrth

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

When using the VartiantBuilder I found it felt a bit clunky that the finish() method returned a raw tuple of (Vec<u8>, Vec<u8>). Given the 0-copy nature of the Variant type, I understand why this is the case, but I think the ergonomics of this could be improved, which would help the code be more self-documenting and in turn hopefully more clear for users.

Describe the solution you'd like
I don't feel like this needs a major overhaul, and I'm pretty sure some simple type aliasing might improve this.

(pseudo-code)

// There are probably better names here, but user usage with Variant::Metadata and Variant::ValueData seems
// relatively nice
pub type Metadata = Vec<u8>;
pub type ValueData = Vec<u8>;
pub type Data = (Metadata, ValueData);

Then, we could have something like:

// VariantBuilder
pub fn finish(&self) -> Data {
...
}

// Variant

 // blissfully ignoring lifetime specifiers for brevity
pub fn try_new(data: &Data) -> Result<Self, ArrowError> {
    Variant::try_new_from_parts(&data.0, &data.1)
}

// The existing `new()` methods could be kept, but renamed to maintain functionality
// and use the familiar "from_parts" convention
pub fn try_new_from_parts(metadata: &'m [u8], value: &'v [u8]) -> Result<Self, ArrowError> {
...
}

Usage on the user's end would then look more like:

let mut builder = VariantBuilder::new();
builder.append_value(12);
let data = builder.finish();

let variant = Variant::new(&data);

// or from parts
let (meta, value) = (data.0, data.1) // this admittedly doesn't read quite as nicely
let variant2 = Variant::new_from_parts(&meta, &value);

Describe alternatives you've considered
An alternative is, of course, doing nothing. There's clearly nothing explicitly incorrect about the existing implementation perhaps aside from it being easy to forget which element of the (Vec<u8>, Vec<u8>) is value data and which is metadata.

Additional Context
If this seems like a direction people would like to move I would be happy to contribute via PR or PR review etc.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementAny new improvement worthy of a entry in the changelog

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions