adriangb opened a new pull request, #10879:
URL: https://github.com/apache/arrow-rs/pull/10879
# Which issue does this PR close?
- Closes #10877.
# Rationale for this change
`VariantMetadata::get` validated the dictionary entry as UTF-8 on every
call, including on
instances that `try_new` or `with_full_validation` had already validated end
to end. Full
validation proves the whole dictionary value region is valid UTF-8 and that
every adjacent pair of
offsets delimits an in-bounds, non-decreasing range landing on character
boundaries, so entries
were being revalidated once per field access instead of once per metadata
buffer.
`get` is on the field-access path for `VariantObject::field_name`,
`VariantObject::iter`,
`VariantMetadata::get_entry` and `impl Index`, so this affects any workload
that reads field names
back out of a validated buffer.
# What changes are included in this PR?
- `VariantMetadata::get` reads the entry directly when the instance is
already fully validated,
and keeps the existing checked path otherwise.
- A `variant_metadata_access` benchmark covering dictionary iteration,
`field_name`, object
iteration and lookup by name. The existing benchmarks cover building and
validating variants,
but nothing measured reading field names back out.
## On the `unsafe`
This introduces the first `unsafe` block in `parquet-variant`, so it
deserves scrutiny. The
argument that `validated` implies the bytes are valid UTF-8:
- `validated` is set only by `with_full_validation`, which proves the value
region is valid UTF-8
and slices every dictionary entry out of it, so each entry range is in
bounds, non-decreasing and
on character boundaries.
- `get_offset` reads that same offset array and bounds checks `i + 1`
against it, so the range
`get` computes is one of the ranges validation already checked.
- `bytes` is assigned once during construction and never reassigned.
- `EMPTY_VARIANT_METADATA` is `validated` but has no entries, so `get`
returns `Err` from the
offset lookup before reaching the conversion.
I considered caching the value region as a `&str` to avoid `unsafe`
entirely, but
`expect_size_of::<VariantMetadata>(32)` pins the struct at 32 bytes on
64-bit and a second fat
pointer would take it to 48.
# Are these changes tested?
Yes.
- `get` now carries a `debug_assert!` re-checking the invariant the
conversion relies on. The
proptest suite runs in debug, so `validated_metadata_is_accessible` (added
in #10352) becomes a
direct test of the unchecked path. It passes at 500,000 cases.
- New unit tests pin the invariant: full validation rejects offsets that
split a multi-byte
character (sorted and unsorted headers); validated and unvalidated
instances agree on every
entry across 1-, 2-, 3- and 4-byte characters and empty entries; and an
unvalidated instance
still reports invalid UTF-8 through the fallible accessor.
- I checked the first of those is not vacuous by reverting the validation it
protects, which makes
it fail.
- Existing tests pass, including with `--no-default-features`.
## Benchmark results
cargo bench -p parquet-variant --bench variant_metadata_access
I did not have an idle machine available, so rather than quote a single run
I interleaved the two
builds, `A B A B ...` for 60 rounds each, and report the minimum per
benchmark across all 60 runs
of each build. Wall-clock minima are robust to competing load in a way that
means and medians are
not, since contention can only add time.
| benchmark | before | after | speedup |
| --- | --- | --- | --- |
| `metadata_iter/8` | 86 ns | 39 ns | 2.23x |
| `metadata_iter/32` | 342 ns | 147 ns | 2.33x |
| `metadata_iter/128` | 1.41 µs | 574 ns | 2.46x |
| `object_field_name/8` | 104 ns | 59 ns | 1.76x |
| `object_field_name/32` | 439 ns | 232 ns | 1.89x |
| `object_field_name/128` | 1.88 µs | 919 ns | 2.04x |
| `object_iter/8` | 160 ns | 110 ns | 1.46x |
| `object_iter/32` | 664 ns | 429 ns | 1.55x |
| `object_iter/128` | 2.85 µs | 1.76 µs | 1.62x |
| `object_get_by_name/8` | 52 ns | 34 ns | 1.52x |
| `object_get_by_name/32` | 72 ns | 43 ns | 1.67x |
| `object_get_by_name/128` | 124 ns | 73 ns | 1.70x |
As a cross-check on the method, I also computed each result as a paired
ratio against the
neighbouring runs of the other build, which cancels drift, and ran the
identical estimator over
runs of the *same* build, where the answer must be 1.00x. That null control
came out at 1.06x with
every per-benchmark confidence interval containing 1.00x, and the paired
estimates agreed with the
minima above to within about 10% once corrected by it. Absolute numbers from
a quiet machine would
still be preferable and I am happy to re-run if that would help.
# Are there any user-facing changes?
No API changes. `get` returns the same values for the same inputs; validated
instances simply stop
repeating work already done at construction.
# AI usage
This PR was written with Claude Code and reviewed by a human.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
