Skip to content

variant: optimize variant object path access (79% lower latency) - #10358

Open
friendlymatthew wants to merge 2 commits into
apache:mainfrom
pydantic:friendlymatthew/optimize-variant-object-path-access
Open

variant: optimize variant object path access (79% lower latency)#10358
friendlymatthew wants to merge 2 commits into
apache:mainfrom
pydantic:friendlymatthew/optimize-variant-object-path-access

Conversation

@friendlymatthew

Copy link
Copy Markdown
Contributor

Rationale for this change

Based on benchmark #10357

Screenshot 2026-07-16 at 5 15 20 PM

This PR speeds up field extraction from unshredded variant objects by avoiding repeated field name decoding for every row

Variant object keys are stored as numeric field ids that map to a metadata dictionary. variant_get currently stores a named path by repeatedly mapping those ids back to strings and comparing the strings for each row, even when a batch shares one metadata dictionary. This repeated per-row work dominates when dealing with wide dictionaries

This optimization adds metadata scoped field id lookups, caches parsed metadata, and resolves named path elements to ids once per metadata dictionary before scanning rows. It re-resolves paths when the metadata changes and retains name lookup for unsorted dictionaries

@github-actions github-actions Bot added the parquet-variant parquet-variant* crates label Jul 16, 2026
@friendlymatthew

Copy link
Copy Markdown
Contributor Author

cc @alamb

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good overall -- thank you @friendlymatthew -- I had a few suggestions.

.expect("created struct should be a valid shredded variant")
}

fn create_unshredded_object_variant_array(size: usize) -> VariantArray {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful if you split the change to the benchmark off into a separate PR I can merge it and then use the benchmarking scripts

builder.append_null()?;
false // add this to make match arms have the same return type

if has_path

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some comments to explain the rationale for this condition / fast path?

&& target.typed_value_column().is_none()
&& let Some(value_column) = target.value_column()
{
let metadata_column = target.metadata_column();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this function is already pretty long, could you please try and refactor some of the logic into smaller documented functions that each do some particular operation?

@alamb

alamb commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

FYI @adriangb -- is this one related to your recent work?

@sdf-jkl

sdf-jkl commented Sep 3, 2026

Copy link
Copy Markdown
Member

@alamb

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

Labels

parquet-variant parquet-variant* crates performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants