GH-3735: Use unsigned UTF-8 byte order for Variant object field keys - #3746
Open
peterxcli wants to merge 2 commits into
Open
GH-3735: Use unsigned UTF-8 byte order for Variant object field keys#3746peterxcli wants to merge 2 commits into
peterxcli wants to merge 2 commits into
Conversation
… keys The Variant spec requires object field ids to be sorted by the unsigned byte order of the field names' UTF-8 encoding, so readers can binary search them. VariantBuilder sorted fields - and Variant.getFieldByKey binary-searched them - with String.compareTo, which orders UTF-16 code units instead. The two orders diverge for keys containing supplementary-plane characters (U+10000 and above). - Add VariantUtil.encodeKey/compareKeys and use them when sorting object fields and binary-searching by key (adapted from apache#3736) - Retry lookups in UTF-16 order for keys containing code units at or above U+D800, so objects written before this fix remain readable Co-authored-by: rayokota <rayokota@gmail.com>
peterxcli
force-pushed
the
claude/equivalence-fix-9340dd
branch
from
August 25, 2026 01:45
0fdc9ac to
28c2621
Compare
divjotarora
approved these changes
Aug 26, 2026
divjotarora
left a comment
Contributor
There was a problem hiding this comment.
One comment about potential perf issues, non-blocking as obviously correctness is more important. Also, great test coverage!
| int midId = VariantUtil.readUnsignedLittleEndian(value, idStart + info.idSize * mid, info.idSize); | ||
| String midKey = getMetadataKeyCached(midId); | ||
| int cmp = attempt == 0 | ||
| ? VariantUtil.compareKeys(VariantUtil.encodeKey(midKey), keyBytes) |
Contributor
There was a problem hiding this comment.
IIUC VariantUtil.encodeKey(midKey) means we now do an allocation on every iteration, correct? Can we measure the impact of this? Would it be better to cache the encoded keys, similar to getMetadataKeyCached?
Member
There was a problem hiding this comment.
Or do something like this?
boolean needsUtf8 = containsCodeUnitAtLeast(key, Character.MIN_SURROGATE);
byte[] keyBytes = needsUtf8 ? VariantUtil.encodeKey(key) : null;
// binary-search loop:
int cmp = needsUtf8
? VariantUtil.compareKeys(VariantUtil.encodeKey(midKey), keyBytes)
: midKey.compareTo(key);
Both reviewers flagged the per-comparison `encodeKey` allocation in the binary search. UTF-8 byte order is exactly code point order, so compare the UTF-16 code units directly with a surrogate adjustment instead: `VariantUtil.compareKeys(String, String)` allocates nothing, and both the builder's sort and the reader's search use it. Per wgtmac's suggestion, a lookup key with no code unit at or above U+D800 compares identically under either order, so it takes a single `String.compareTo` search that is unchanged from before this PR. Keys that do contain one take an out-of-line path, so the common search is not enlarged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
The Variant specification requires an object's field ids to be sorted by the unsigned byte order of the field names' UTF-8 encoding, so that a reader can binary search them.
VariantBuildersorted those fields withString.compareTo, which orders UTF-16 code units, andVariant.getFieldByKeysearched them with the same comparison.The two orders agree on every name in the Basic Multilingual Plane, so they only diverge for a name holding a character above U+FFFF.
String.compareToplaces a leading high surrogate (0xD800 to 0xDBFF) below code points in U+E000 to U+FFFF, while UTF-8 byte order places it above. An object that parquet-java writes with such a name therefore carries its field ids in an order the specification forbids, and a reader that follows the specification can miss fields when it binary searches. The gap runs the other way too: parquet-java could fail to find a name above U+FFFF in an object written by another implementation.This work started from #3736 by @rayokota. It also carries over the fallback that lets a reader still find names in objects written in the old order, which came from the equivalent Spark fix in apache/spark#58239 and the discussion there.
What changes are included in this PR?
VariantUtil.compareKeys(String, String)orders two field names the way their UTF-8 encodings compare as unsigned bytes, without encoding either one. UTF-8 byte order is exactly code point order, so the method walks the UTF-16 code units and adjusts for surrogates: a surrogate always encodes a code point above U+FFFF, so U+D800 to U+DFFF rank above U+E000 to U+FFFF, and the latter shift down to fill the gap they leave.VariantBuilder.FieldEntry.compareTosorts an object's fields through this method, so the sort allocates nothing.Variant.getFieldByKeyruns its usualString.compareTobinary search whenever the lookup key holds no code unit at or above U+D800. Such a key compares the same way under both orders, so a single search finds it in an object written either way.A lookup key that does hold such a code unit goes to a separate method, which searches in UTF-8 byte order and then searches again in the UTF-16 order that older versions wrote, so their objects stay readable. Giving it its own method keeps the common search small. Folding both attempts into
getFieldByKeycost ordinary lookups about 15%, because the larger method inlines worse.Are these changes tested?
Four tests in
TestVariantObjectBuildercover the fix.testObjectKeysSortedByUtf8ByteOrderbuilds an object whose names are U+FFFF (EF BF BF) and U+10000 (F0 90 80 80), appends them in reverse, and asserts the encoded order is U+FFFF then U+10000. The old comparison put them the other way around.testLargeObjectBinarySearchWithSupplementaryKeybuilds an object of 42 fields, aboveBINARY_SEARCH_THRESHOLD, mixing ASCII names with U+FFFF and U+10000, and assertsgetFieldByKeyfinds both through the binary search.testLegacyUtf16OrderedObjectLookuprewrites a canonical object's id and offset lists into the old UTF-16 order, then assertsgetFieldByKeystill finds the ASCII, U+FFFF, and U+10000 names, and still returns null for a name the object does not hold.testCompareKeysMatchesUtf8ByteOrdercheckscompareKeysagainstArrays.compareUnsignedon the two names' UTF-8 bytes, over names covering every UTF-8 length, both sides of the surrogate range, prefixes, and 200 random names. It leaves out unpaired surrogates, which have no UTF-8 encoding because Java's encoder substitutes?for them, as the method documents.All 184
parquet-varianttests pass, along with theparquet-avrovariant read and write suites.I also measured lookups on an object of 256 fields, alternating JVM runs on Zulu 21 and Apple silicon and taking the best of seven rounds per run, in ns/op.
The ASCII rows run the same binary search as before, and the small difference is the single scan of the lookup key for a code unit at or above U+D800. The last row now searches in UTF-8 order, and searches a second time when the object uses the old order.
Are there any user-facing changes?
An object written from now on orders a field name above U+FFFF as the specification requires.
getFieldByKeystill finds such names in objects already written in the UTF-16 order.Closes #3735