[Variant] Support nested lists and object lists #7740

friendlymatthew · 2025-06-22T23:01:42Z

Which issue does this PR close?

Closes [Variant] Support Nested Data in VariantBuilder #7696

Rationale for this change

Heavily refactors VariantBuilder, ObjectBuilder, and ListBuilder to allow nested list and object building.

Now, ObjectBuilder and ListBuilder have their own VariantBuffer, an intermediate buffer that gets written to upon every call to insert or append_value. Only when finish is called will the builder flush the intermediate buffer to its parent buffer.

VariantBuilder was split to hold VariantBuffer and FieldMetadataDictionary to better simplify and separate the nesting logic.

alamb · 2025-06-23T15:02:16Z

@PinkCrow007 @superserious-dev and @mkarbo it would also be great to hear your thoughts as well if you have time to review this PR

friendlymatthew · 2025-06-23T15:04:59Z

Hi @scovich, I'd be curious to get your thoughts on this PR as well!

alamb · 2025-06-23T21:54:45Z

Yes indeed, @scovich your review would be very helpful.

I plan to review this PR as well tomorrow

parquet-variant/src/builder.rs

alamb · 2025-06-23T22:16:55Z

parquet-variant/src/builder.rs

+/// };
+///
+/// assert_eq!(
+///     obj1.field_by_name("id").unwrap(),


I found this API really awkward too -- here is a suggestion on making it better

[Variant] Improved API for accessing Variant Objects and lists #7756

@friendlymatthew

# Which issue does this PR close? - part of #6736 # Rationale for this change - While reviewing @friendlymatthew 's PR #7740 I found that the code to get the Variant object was awkward I think that an accessor is similar to the existing `as_null`, `as_i32,` etc APIs. # What changes are included in this PR? 1. Add Variant::as_object and Variant::as_list # Are there any user-facing changes? New API (and docs with tests)

crepererum · 2025-06-24T11:27:00Z

FYI: #7755 got merged, so you can now use the respective methods.

friendlymatthew · 2025-06-24T11:58:22Z

FYI: #7755 got merged, so you can now use the respective methods.

Thank you, this PR has been rebased

alamb

Thank you @friendlymatthew -- I went through this PR and I thought it is nicely tested and structured. I had some comments but I don't think anything is required

parquet-variant/src/builder.rs

alamb · 2025-06-24T16:50:28Z

parquet-variant/src/builder.rs

This is a very nice pattern

alamb · 2025-06-24T16:54:36Z

parquet-variant/src/builder.rs

A minor thought would be to move the construction of the metadata / dictionary into the field_metadata_dictionary itself

Something like

let metadata = self.field_metadata_dictionary.build()

I am thinking the more encapsulated building the metadata dictionary is, the easier it will be to optimize it later (for example to avoid allocating and copying so many strings)

Doesn't have to be for this PR, I just noticed it during review

I like that! Could we do this in a subsequent PR? I have another PR that will follow this that does nested object and object with list building

alamb · 2025-06-24T16:57:24Z

parquet-variant/src/builder.rs

parent_buffet.offset() seems to just return the length.

I think this code is doing the equivalent of self.parent_buffer.reserve(header_size) to reserver enough space (I am not sure what make_room_for_header is doing)

We can't replace make_room_for_header with Vec::reserve here, since make_room_for_header actually allocates header_size bytes worth of space, whereas reserve merely ensures capacity without changing the buffer's length.

We could do this for now,

self.parent_buffer .0 .resize(self.parent_buffer.0.len() + header_size, 0);

but I think the actual issue is there's room to simplify the whole buffer-copy slicing ceremony going on here. I have some thoughts that I'd like to push up in a separate PR

A new PR sounds good -- there are other APIs like extend and push for Vecs that might simplify the code (and likely perform better too)

alamb · 2025-06-24T16:58:33Z

parquet-variant/src/builder.rs

I don't understand how this can be writing to anything other than the end of parent_buffer and thus it seems like the code to handle other positions is unecesssary

I also see you just refactored this here rather than introducing it, so no need to change it in this PR

See: #7740 (comment)

alamb · 2025-06-24T17:01:44Z

parquet-variant/src/builder.rs

As a follow on it would be really nice to avoid having to copy all the values back to the parent buffer.

One problem is that you don't know beforehand how many elements are in the list (so we don't know the offset size and therefore we don't know how many bytes to leave empty for the list length). However, I think we could do something fancy like assume it will be a short list (and leave 1 byte for the length) and then if we end up with more we could go back and copy over the values

We could also add a nice api like make_large() or something on the ListBuilder

I am just speculating, there is no need to do this here in this PR or without benchmarks

Ah, I found I already wrote this up:

[Variant] Test and implement efficient building for "large" Arrays #7699

I was thinking about this as well -- and yes, now that we can build large lists, it'll be worthy of profiling and benchmarking 👍

parquet-variant/src/builder.rs

alamb · 2025-06-24T18:00:32Z

parquet-variant/src/builder.rs

alamb

Thanks again @friendlymatthew -- let's merge this one in and keep on iterating

scovich

Belated review (I had it pending but forgot to post it, oops)

scovich · 2025-06-23T11:46:02Z

parquet-variant/src/builder.rs

nit: perhaps worth factoring out an append_primitive_header helper?

Suggested change

self.0

.push(primitive_header(VariantPrimitiveType::TimestampMicros));

self.append_primitive_header(VariantPrimitiveType::TimestampMicros);

(similar for append_from_slice)

scovich · 2025-06-23T15:34:47Z

parquet-variant/src/builder.rs

+#[derive(Default)]
+struct FieldMetadataDictionary {
+    field_name_to_id: BTreeMap<String, u32>,
+    field_names: Vec<String>,


What purpose does this vec of strings serve? Seems like the map is all we need?
(it looks like all the immediately obvious uses of field_names can be replaced by similar operations over the map instead)

scovich · 2025-06-23T15:54:22Z

parquet-variant/src/builder.rs

+
+impl FieldMetadataDictionary {
+    /// Add field name to dictionary, return its ID
+    fn add_field_name(&mut self, field_name: &str) -> u32 {


Not really "add"? More of an "upsert" or "ensure" ?

scovich · 2025-06-23T15:58:40Z

parquet-variant/src/builder.rs

+        use std::collections::btree_map::Entry;
+        match self.field_name_to_id.entry(field_name.to_string()) {
+            Entry::Occupied(entry) => *entry.get(),
+            Entry::Vacant(entry) => {
+                let id = self.field_names.len() as u32;
+                entry.insert(id);
+                self.field_names.push(field_name.to_string());
+                id
+            }
+        }
+    }


If we get rid of field_names, then I believe this simplifies to just

Suggested change

use std::collections::btree_map::Entry;

match self.field_name_to_id.entry(field_name.to_string()) {

Entry::Occupied(entry) => *entry.get(),

Entry::Vacant(entry) => {

let id = self.field_names.len() as u32;

entry.insert(id);

self.field_names.push(field_name.to_string());

id

}

}

}

*self

.field_name_to_id

.entry(field_name.to_string())

.or_insert(self.field_names.len() as u32)

scovich · 2025-06-24T22:09:02Z

parquet-variant/src/builder.rs


-/// Builder for [`Variant`] values
+#[derive(Default)]
+struct ValueBuffer(Vec<u8>);


Adding impl Deref and impl DerefMut to the underlying vec might simplify a lot of code -- if it's ok to expose it that way?

Hm, I was thinking about this and I'm a bit cautious to do so. Since the inner value is just a Vec<u8>, I thought it would be nice to encapsulate this with a well defined API

alamb · 2025-06-25T15:54:20Z

Belated review (I had it pending but forgot to post it, oops)

@friendlymatthew -- are you interested in making the changes as a new PR?

Or maybe @scovich you would like to just make the suggestions yourself directly in a new PR?

friendlymatthew · 2025-06-25T16:47:57Z

Belated review (I had it pending but forgot to post it, oops)

@friendlymatthew -- are you interested in making the changes as a new PR?

Or maybe @scovich you would like to just make the suggestions yourself directly in a new PR?

Hi, I'm happy to make the following changes. I'm going to push the nested object + object list building first and then will push up follow up PRs if that is alright.

# Which issue does this PR close? - Closes #7696 - Related to #7740 # Rationale for this change This PR adds a feature to `Variant::ObjectBuilder` that enables constructing nested objects and objects with lists. # Are there any user-facing changes? Adds two public methods to the `ObjectBuilder` API, `new_list` and `new_object` --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added the parquet Changes to the parquet crate label Jun 22, 2025

friendlymatthew force-pushed the friendlymatthew/new-builder branch 5 times, most recently from 3b539d0 to 0168c8c Compare June 23, 2025 12:00

friendlymatthew mentioned this pull request Jun 23, 2025

[Variant] Use BTreeMap for VariantBuilder.dict and ObjectBuilder.fields to maintain invariants upon entry writes #7720

Merged

alamb mentioned this pull request Jun 23, 2025

[Variant] Add Variant::as_object and Variant::as_list #7755

Merged

alamb reviewed Jun 23, 2025

View reviewed changes

parquet-variant/src/builder.rs Outdated Show resolved Hide resolved

alamb reviewed Jun 23, 2025

View reviewed changes

friendlymatthew force-pushed the friendlymatthew/new-builder branch 2 times, most recently from 3625f37 to e25d359 Compare June 24, 2025 11:57

alamb approved these changes Jun 24, 2025

View reviewed changes

alamb mentioned this pull request Jun 24, 2025

feat: [Variant] Add Validation for Variant Deciaml #7738

Merged

Support nested list and object list building

16bae15

friendlymatthew force-pushed the friendlymatthew/new-builder branch from e25d359 to 16bae15 Compare June 24, 2025 18:23

friendlymatthew added 3 commits June 24, 2025 14:27

Complete rebase

ee76685

Rename FieldMetadataDictionary to MetadataBuilder

bcee887

Rename VariantBuffer to ValueBuffer

2971776

alamb approved these changes Jun 24, 2025

View reviewed changes

alamb merged commit 8d8541c into apache:main Jun 24, 2025
12 checks passed

scovich reviewed Jun 24, 2025

View reviewed changes

friendlymatthew mentioned this pull request Jun 25, 2025

[Variant] Support creating nested objects and object with lists #7778

Merged

alamb mentioned this pull request Jul 28, 2025

[Variant] Test and implement efficient building for "large" Arrays #7699

Closed

	self.0
	.push(primitive_header(VariantPrimitiveType::TimestampMicros));
	self.append_primitive_header(VariantPrimitiveType::TimestampMicros);

[Variant] Support nested lists and object lists #7740

[Variant] Support nested lists and object lists #7740

Uh oh!

Conversation

friendlymatthew commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

Uh oh!

alamb commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

friendlymatthew commented Jun 23, 2025

Uh oh!

alamb commented Jun 23, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crepererum commented Jun 24, 2025

Uh oh!

friendlymatthew commented Jun 24, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friendlymatthew Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 25, 2025

Uh oh!

friendlymatthew commented Jun 25, 2025

Uh oh!

Reviewers

Assignees

friendlymatthew commented Jun 22, 2025 •

edited

Loading

alamb commented Jun 23, 2025 •

edited

Loading

friendlymatthew Jun 24, 2025 •

edited

Loading