Skip to content

perf(variant): resolve borrowed field names without searching the metadata dictionary - #10882

Open
adriangb wants to merge 2 commits into
apache:mainfrom
adriangb:perf/variant-shred-field-id-lookup
Open

perf(variant): resolve borrowed field names without searching the metadata dictionary#10882
adriangb wants to merge 2 commits into
apache:mainfrom
adriangb:perf/variant-shred-field-id-lookup

Conversation

@adriangb

@adriangb adriangb commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

VariantMetadata::get_entry resolves a field name to a field id by searching the
dictionary, decoding and comparing dictionary strings as it goes (linear for an
unsorted dictionary, logarithmic for a large sorted one).

Several paths copy fields out of a variant object and back into a builder that
shares that object's metadata dictionary. shred_variant writes every field the
shredding schema does not cover into the leftover value column, and projection
paths do the same. In all of them the field name handed to the builder came from
VariantObject::iter, which produced it by looking up a field id in that very
dictionary. Searching for it by name is a round trip: the id was already known,
and the search spends string comparisons recovering it.

ReadOnlyMetadataBuilder has a known_field_names cache intended to absorb this
cost, but it cannot help here. VariantValueArrayBuilder::builder_ext constructs
a fresh ReadOnlyMetadataBuilder per value, so in a per-row builder the cache is
populated and dropped again on every row, never serving a lookup, and each row
pays to hash names it will never see again.

In a CPU profile of the shred_variant_unmatched_object_8k_rows benchmark added
here, ReadOnlyMetadataBuilder::try_upsert_field_name accounted for about 57% of
shred_variant. After this change it accounts for about 20%, and get_entry no
longer appears in the hot path.

What changes are included in this PR?

  • VariantMetadata::borrowed_field_id (crate-private). A field name that is a
    slice of the dictionary's own value region already encodes its field id: it
    belongs to the entry whose offset equals the name's distance from the start of
    that region. This finds the entry with a binary search over the offset array,
    comparing integers instead of decoding dictionary strings, and confirms the hit
    by comparing lengths rather than bytes.

    A candidate is accepted only when it starts at the name's address and has the
    name's length, which makes the entry's bytes and the name's bytes the same
    bytes. Anything else, including a name that borrows from elsewhere, a slice of
    an entry, or metadata with arbitrary offsets, falls back to the existing
    search. Addresses are only ever compared as integers, never dereferenced.

    This is attempted only for a sorted dictionary; see "Why sorted only"
    below.

  • VariantMetadata::get_entry tries the above first, so all callers benefit.

    Cost for callers this cannot help: get_entry is public, and a name looked up
    against a sorted dictionary that does not borrow from it now runs the address
    range check before the existing search. That check short circuits on a failed
    comparison, so such a caller pays a few integer operations and nothing else.
    The one case that pays more is a name pointing into the value region without
    starting an entry, for example a substring of one: that costs a binary search
    over the offset array before falling back. Against an unsorted dictionary the
    added cost is a single boolean test. All three are bounded, but I would rather
    state them than have them found in review.

  • ReadOnlyMetadataBuilder::try_upsert_field_name tries it before consulting
    known_field_names, so the paths described above do no hashing at all. The
    cache still serves field names that do not borrow from the dictionary, and all
    field names when the dictionary is unsorted.

  • shred_variant reuses one scratch buffer to track which shredded fields a row
    supplied, instead of allocating a HashSet per row.

  • Two new benchmarks in parquet-variant-compute/benches/variant_kernels.rs
    covering objects that the shredding schema matches partially and not at all.

Why sorted only

Thanks to @sdf-jkl for catching this; an earlier revision of this PR did not
restrict the fast path, and was wrong.

The spec requires dictionary entries to be unique only when
sorted_strings is set: "If the value is set to 0, readers may not make any
assumptions about string order or uniqueness." So an unsorted dictionary may
legally hold the same string at more than one field id, and this crate already
relies on that — with_full_validation checks uniqueness only in the sorted
branch, and test_object_rejects_duplicate_field_names covers exactly such a
dictionary.

For such a dictionary, resolving a borrowed name by the id it came from
disagrees with resolving the same string by name. That is not a cosmetic
difference in which id gets picked, because ObjectBuilder detects duplicate
fields by comparing field ids: two ids naming the same string defeat
validate_unique_fields, and with validation off they build an object whose
field names are not unique, which Variant::try_new then rejects with "field
names not sorted".

Restricting borrowed_field_id to sorted dictionaries removes that entirely,
because validation rejects a sorted dictionary with duplicate entries, so the id
a name was borrowed from is necessarily the id a search by name returns. An
unsorted dictionary goes back to the known_field_names cache and the existing
name search, exactly as before this PR.

The cost is that the fast path no longer applies to unsorted dictionaries, where
get_entry can only search linearly. The benchmarks here build a sorted
dictionary (300 entries, is_sorted() == true), so the profile numbers above are
unaffected by the restriction; recovering the unsorted case would need a
duplicate-free flag computed during validation, which I would rather measure
separately than fold in here.

Are these changes tested?

Yes. New unit tests cover sorted and unsorted dictionaries, agreement between the
borrowed lookup, get_entry, and lookups by an owned (non-borrowed) copy of the
same name, names borrowed from a different dictionary that must not be resolved
against this one, a slice of an entry that shares its start offset without being
equal to it, an unsorted dictionary holding the same string twice, and empty
field names in both a sorted and an unsorted dictionary.

Two regression tests build objects through a ReadOnlyMetadataBuilder over an
unsorted ["a", "a"] dictionary: one asserts that validate_unique_fields
rejects the second insert, and one asserts that a borrowed name and an owned copy
of it collapse to a single field rather than producing a value that fails
validation. Both fail if the sortedness restriction is removed.

The existing parquet-variant, parquet-variant-compute, parquet-variant-json,
proptest fuzz, and variant_interop suites pass unchanged.

The two new benchmarks cover 8192 rows over a 300-entry dictionary with 15-field
objects. I am deliberately not posting timings yet. The machine available to me is
heavily contended, and a paired interleaved probe there produced a 65% spread
within a single invocation on identical work, so any speedup figure from it would
be indistinguishable from noise. I will follow up with numbers from a quiet
machine, measured with interleaved arms and with unaffected control benchmarks
used to certify that the run is valid.

Are there any user-facing changes?

No. There are no public API changes, and get_entry returns exactly what it
returned before this PR for every input, including a dictionary that holds the
same string at more than one field id.

…tionary

Copying fields out of a variant object and back into a builder that shares
the object's metadata dictionary (unshredding, shredding, and projection all
do this) resolves each field name against that dictionary with
`VariantMetadata::get_entry`, which decodes and compares dictionary strings
until it finds a match.

Those field names are slices of the dictionary's own value region, so they
already encode their field id: a name belongs to the entry whose offset
equals the name's distance from the start of the value region. Add
`VariantMetadata::borrowed_field_id`, which recovers the field id with a
binary search over the offset array and confirms the hit by comparing
lengths, so it performs no string decoding or comparison at all. The check
is exact, so an unrelated or non-borrowed name simply falls back to the
existing search.

`ReadOnlyMetadataBuilder` tries this before consulting `known_field_names`.
That cache is built per value, so in a per-row builder it was populated and
discarded without ever serving a lookup, and every row paid to hash names it
would never see again.

Also reuse one scratch buffer for the set of shredded fields seen in a row,
instead of allocating a `HashSet` per row.
@github-actions github-actions Bot added the parquet-variant parquet-variant* crates label Aug 27, 2026
@alamb

alamb commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

FYI @sdf-jkl

@sdf-jkl sdf-jkl 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.

Thanks @adriangb, a note on the PR description and a bug

The note -

One behavior note

The spec requires dictionary keys to be unique, and validation enforces that for
sorted dictionaries. It does not enforce it for unsorted ones, so a dictionary
that validates can still contain the same key twice. For such a dictionary,
get_entry previously returned the first matching id and now returns the id the
borrowed name actually came from. Both ids name that same string, so a returned
field id still always names the string the caller asked for, but the specific id
can differ from before in that spec-violating case. I am happy to reject
duplicates during validation of unsorted dictionaries instead, or to fold that
into a follow-up, if maintainers prefer.

This is incorrect. https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#metadata-encoding-grammar notes below:

If sorted_strings is set to 1, strings in the dictionary must be unique and sorted in lexicographic order. If the value is set to 0, readers may not make any assumptions about string order or uniqueness.

Comment on lines +111 to +113
if let Some(field_id) = self.metadata.borrowed_field_id(field_name) {
return Ok(field_id);
}

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.

This can lead to multiple object fields with the same name which is illegal for Variant::Object

MRE:

  #[test]
  fn duplicate_names_with_distinct_ids_bypass_validation() {
      // Valid unsorted metadata dictionary: ["a", "a"]
      let bytes = [0x01, 0x02, 0x00, 0x01, 0x02, b'a', b'a'];
      let metadata = VariantMetadata::try_new(&bytes).unwrap();

      let mut values = VariantValueArrayBuilder::new(1);
      let mut builder = values.builder_ext(&metadata);
      let mut object = builder
          .try_new_object()
          .unwrap()
          .with_validate_unique_fields(true);

      object.try_insert(metadata.get(0).unwrap(), 1_i8).unwrap();

      // Expected: Err, because both IDs resolve to the object key "a".
      // Actual on #10882: Ok, because the builder compares IDs 0 and 1.
      assert!(object
          .try_insert(metadata.get(1).unwrap(), 2_i8)
          .is_err());
  }

…ionary

The spec requires dictionary entries to be unique only when `sorted_strings`
is set, so an unsorted dictionary may legally hold the same string at more
than one field id. Resolving a borrowed field name by the id it came from
therefore disagreed with resolving it by name, and `ObjectBuilder` detects
duplicate fields by comparing field ids: an object could end up with two ids
naming the same string. That defeated `validate_unique_fields`, and without
it built an object that `Variant::try_new` rejects as having unsorted field
names.

Restrict `borrowed_field_id` to sorted dictionaries, where validation has
already rejected duplicate entries and so the two resolutions must agree.
`get_entry` keeps its previous semantics for every input, and an unsorted
dictionary goes back to the `known_field_names` cache and the name search.

Reported by @sdf-jkl in review of apache#10882.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@sdf-jkl sdf-jkl 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.

Thanks @adriangb LGTM. @klion26 PTAL.

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

Labels

parquet-variant parquet-variant* crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Variant: copying an object's fields back into a builder re-searches the metadata dictionary by name

3 participants