perf(variant): resolve borrowed field names without searching the metadata dictionary - #10882
perf(variant): resolve borrowed field names without searching the metadata dictionary#10882adriangb wants to merge 1 commit into
Conversation
…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.
|
FYI @sdf-jkl |
sdf-jkl
left a comment
There was a problem hiding this comment.
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.
| if let Some(field_id) = self.metadata.borrowed_field_id(field_name) { | ||
| return Ok(field_id); | ||
| } |
There was a problem hiding this comment.
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());
}
Which issue does this PR close?
Rationale for this change
VariantMetadata::get_entryresolves a field name to a field id by searching thedictionary, 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_variantwrites every field theshredding schema does not cover into the leftover
valuecolumn, and projectionpaths 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 verydictionary. Searching for it by name is a round trip: the id was already known,
and the search spends string comparisons recovering it.
ReadOnlyMetadataBuilderhas aknown_field_namescache intended to absorb thiscost, but it cannot help here.
VariantValueArrayBuilder::builder_extconstructsa fresh
ReadOnlyMetadataBuilderper value, so in a per-row builder the cache ispopulated 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_rowsbenchmark addedhere,
ReadOnlyMetadataBuilder::try_upsert_field_nameaccounted for about 57% ofshred_variant. After this change it accounts for about 20%, andget_entrynolonger appears in the hot path.
What changes are included in this PR?
VariantMetadata::borrowed_field_id(crate-private). A field name that is aslice 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.
Note that the offset array is monotonic regardless of whether the dictionary is
sorted, so this binary search is available for unsorted dictionaries too, where
get_entryitself can only search linearly.VariantMetadata::get_entrytries the above first, so all callers benefit.Cost for callers this cannot help:
get_entryis public, and a name that doesnot borrow from the dictionary 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. Both are bounded, but I would rather state them than have
them found in review.
ReadOnlyMetadataBuilder::try_upsert_field_nametries it before consultingknown_field_names, so the paths described above do no hashing at all. Thecache still serves field names that do not borrow from the dictionary.
shred_variantreuses one scratch buffer to track which shredded fields a rowsupplied, instead of allocating a
HashSetper row.Two new benchmarks in
parquet-variant-compute/benches/variant_kernels.rscovering objects that the shredding schema matches partially and not at all.
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_entrypreviously returned the first matching id and now returns the id theborrowed 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.
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 thesame 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, and empty field names. The existing
parquet-variant,parquet-variant-compute,parquet-variant-json, proptest fuzz, andvariant_interopsuites 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 public API changes.
get_entrykeeps its signature and its contract that areturned field id names the requested string; see the behavior note above for the
one case where the specific id it picks can differ.