[Variant] VariantBuilder (wrongly?) accepts 0+ variant values

[scovich]

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.

Originally posted by @scovich in #7843 (comment)

[scovich]

It almost seems like the top-level VariantBuilder shouldn't even exist:

• For any primitive value, Variant::from the input should work
• For list and object values, perhaps we need a way to construct a list or object builder directly

The one thing VariantBuilder does provide is the handling of separate metadata + value buffers. But that just proves the current "append as many values as you like" API is incorrect -- each top-level variant value in a column of variant values needs its own metadata value -- not one aggregate metadata value for the whole column.

[scovich]

Also, it turns out you can finish a VariantBuilder without ever appending anything to it, which is also bad (produces an empty value buffer).

[alamb]

The one thing VariantBuilder does provide is the handling of separate metadata + value buffers. But that just proves the current "append as many values as you like" API is incorrect -- each top-level variant value in a column of variant values needs its own metadata value -- not one aggregate metadata value for the whole column.

I think this is an important usecase (it is possible / allowed for the variant value stored in a row to be non nested)

Also, it turns out you can finish a VariantBuilder without ever appending anything to it, which is also bad (produces an empty value buffer).

I think an important usecase for VariantBuilder is to convert a column of an existing type (e.g. json strings, ints, etc) into a column of varaint values. Something like:

let mut builder = VariantBuilder::new();
// store the offsets of the variants created
let mut offsets = vec[];
for row in rows {
  // record where the current variant starts (THIS API DOES NOT EXIST YET)
  offsets.push(builder.current_offset());
  // convert the row to a variant
  convert_row_to_variant(&mut builder, row))?
}

Maybe in addition to the metadata and values, VariantBuilder itself should store the offsets of each of the created variants 🤔

[scovich]

I think an important usecase for VariantBuilder is to convert a column of an existing type (e.g. json strings, ints, etc) into a column of varaint values.

Maybe in addition to the metadata and values, VariantBuilder itself should store the offsets of each of the created variants 🤔

... that could work, but only if it also creates one metadata dictionary per variant value and tracks those offsets as well:

The one thing VariantBuilder does provide is the handling of separate metadata + value buffers. But that just proves the current "append as many values as you like" API is incorrect -- each top-level variant value in a column of variant values needs its own metadata value -- not one aggregate metadata value for the whole column.

... at which point it's no longer meaningful to provide a finish method. Instead, an into_parts method would consume self and return all four building blocks, which a caller could then assemble into a StructArray of BinaryArray.

Careful tho -- BinaryArray uses i32 offsets while variant uses u32!

[alamb]

... that could work, but only if it also creates one metadata dictionary per variant value and tracks those offsets as well:

We probably want to make sure we can share the metadata across rows if desired (so in other words I don't think it should require starting a new metadata...)

I suggest we postpone any more API changes here until we start working on actual variant cast kernels 🤔

Maybe we could start with one we know we will likely want

/// Converts a StringArray, LargeStringArray or StringViewArray that contains
/// JSON formatted text into a Struct array containing a Parquet Variant
json_to_variant_array(arr: ArrayRef) -> Result<StructArray> {
...
}

I think that will also force us to figure out what kind of arrays will be supported

For example is it

StructArray {
  metadata: BinaryView,
  value: BinaryView
}

Or

StructArray {
  metadata: Binary,
  value: Binary
}

Or something else

[scovich]

Or something else

Shredding will add/allow an arbitrary-typed typed_value field as well...

[scovich]

We probably want to make sure we can share the metadata across rows if desired (so in other words I don't think it should require starting a new metadata...)

That's tricky, because we don't know, at the time we start a new variant, whether it will add new field names and/or use all existing ones. From a parquet dictionary encoding perspective, it's attractive to use one/few byte-identical metadata dictionaries for the whole column. But getting it wrong could bloat things really badly.

I suggest we postpone any more API changes here until we start working on actual variant cast kernels 🤔

Fair.

[alamb]

That's tricky, because we don't know, at the time we start a new variant, whether it will add new field names and/or use all existing ones. From a parquet dictionary encoding perspective, it's attractive to use one/few byte-identical metadata dictionaries for the whole column. But getting it wrong could bloat things really badly.

I agree the writer will have to be clever and there is room for substantial tradeoffs (like slower write / better encoding / more memory usage).

What I suggest is that we have all the low level APIs needed (aka allow the caller to decide if they want to create new metadata, or if they want to keep using the same one even though it might be bloated, etc)