GH-3735 Fix Variant field name comparisons to use UTF-8 byte order - #3736
Open
rayokota wants to merge 2 commits into
Open
GH-3735 Fix Variant field name comparisons to use UTF-8 byte order#3736rayokota wants to merge 2 commits into
rayokota wants to merge 2 commits into
Conversation
peterxcli
reviewed
Aug 23, 2026
peterxcli
left a comment
peterxcli
left a comment
Member
There was a problem hiding this comment.
Thanks for this fix, just discovered the same problem few minutes ago 😆. left two comment, one is considering removing the threshold for binary search, the other is for backward compatability.
Parquet spec has regulated the logical field list order:
https://parquet.apache.org/docs/file-format/types/variantencoding/#:~:text=For%20objects%2C%20field,constructing%20Variant%20values.
For objects, field IDs and offsets must be listed in the order of the corresponding field names, sorted lexicographically (using unsigned byte ordering for UTF-8). Note that the field values themselves are not required to follow this order. As a result, offsets will not necessarily be listed in ascending order. The field values are not required to be in the same order as the field IDs, to enable flexibility when constructing Variant values.
btw, seems like spark also does the same incorrect behaviour.
VariantBuilder.FieldEntry.compareTodetermines the encoded object-field order:
https://github.com/apache/spark/blob/9da9f8d673914d1648514f59d85e3adafb300d1a/common/variant/src/main/java/org/apache/spark/types/variant/VariantBuilder.java#L1009-L1010Variant.getFieldByKeyusesString.compareTowhile binary-searching objects containing at least 32 fields:
https://github.com/apache/spark/blob/9da9f8d673914d1648514f59d85e3adafb300d1a/common/variant/src/main/java/org/apache/spark/types/variant/Variant.java#L145-L167
Comment on lines
265
to
274
| if (info.numElements < BINARY_SEARCH_THRESHOLD) { | ||
| for (int i = 0; i < info.numElements; ++i) { | ||
| int id = VariantUtil.readUnsignedLittleEndian(value, idStart + info.idSize * i, info.idSize); | ||
| String fieldKey = getMetadataKeyCached(id); | ||
| if (fieldKey.equals(key)) { | ||
| int offset = VariantUtil.readUnsignedLittleEndian( | ||
| value, offsetStart + info.offsetSize * i, info.offsetSize); | ||
| return childVariant(VariantUtil.slice(value, dataStart + offset)); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
I'm thinking if we could just do binary search anyway. as arrow-rs does it.
https://github.com/apache/arrow-rs/blob/7ec3f5ab0c41f10ece65d3745b989ddde510539b/parquet-variant/src/variant/metadata.rs#L378-L397
| int midId = VariantUtil.readUnsignedLittleEndian(value, idStart + info.idSize * mid, info.idSize); | ||
| String midKey = getMetadataKeyCached(midId); | ||
| int cmp = midKey.compareTo(key); | ||
| int cmp = VariantUtil.compareKeys(VariantUtil.encodeKey(midKey), keyBytes); |
Member
There was a problem hiding this comment.
Should we preserve read compatibility with Variant values written by earlier parquet-java versions? Its writers sorted object entries using Java String.compareTo.
Contributor
Author
|
@peterxcli , I see you made the equivalent fixes for Spark. Feel free to adapt this PR to match or to close this PR and open a new one. Thanks! |
sunchao
pushed a commit
to apache/spark
that referenced
this pull request
Aug 24, 2026
…ct fields ### What changes were proposed in this pull request? This PR fixes [SPARK-58949](https://issues.apache.org/jira/browse/SPARK-58949) by: - sorting newly written Variant object fields by unsigned lexicographic UTF-8 bytes, as required by the [Variant encoding specification](https://github.com/apache/parquet-format/blob/24102ed5c56e51b610a4897e5f79e76e43732d1d/VariantEncoding.md#L449-L463); - using binary search for object lookup at every object size and comparing UTF-8-encoded keys using unsigned byte ordering; - retrying lookup with Java UTF-16 order when needed so values written by older Spark versions remain readable; and - accepting both canonical UTF-8 order and legacy UTF-16 order during schema validation, while preserving the established schema field order. This follows the compatibility direction discussed in [apache/parquet-java#3736](apache/parquet-java#3736). ### Why are the changes needed? The Variant specification orders object keys by unsigned UTF-8 bytes, but Spark used String.compareTo, which orders UTF-16 code units. These orders differ for some valid keys. For example, UTF-16 places U+10000 before U+FFFF, while unsigned UTF-8 places U+FFFF first. As a result, Spark wrote non-canonical Variant objects and could miss fields when binary-searching canonical values produced by another implementation. ### Does this PR introduce _any_ user-facing change? Yes. Newly written Variant objects use the specification's unsigned UTF-8 field order. Spark continues to read affected values written in the legacy UTF-16 order, and schema output keeps its existing field order. ### How was this patch tested? Added regressions for canonical and legacy object lookup, nested objects, schema_of_variant, and Parquet shredding-schema inference: build/sbt \ 'catalyst/testOnly *VariantExpressionSuite -- -z "SPARK-58949"' \ 'sql/testOnly *VariantInferShreddingSuite -- -z "SPARK-58949"' Both suites passed (1 test each). The affected modules also passed Java checkstyle and main/test scalastyle. I also ran a temporary lookup microbenchmark on an Apple M4 with Zulu OpenJDK 21.0.6, comparing upstream/master at 9da9f8d with this patch at b3a050b. Each result is the median of three alternating JVM fork medians. Each fork used a fixed 2 GiB heap, 5 seconds of warmup per case, and 9 measured rounds of 5,000,000 lookups. Object construction was excluded. Lower is better. | Object / lookup | master (ns/op) | patch (ns/op) | Change | | --- | ---: | ---: | ---: | | 16 fields, ASCII present | 193.7 | 94.6 | -51.2% | | 16 fields, ASCII absent | 180.3 | 87.2 | -51.6% | | 256 fields, canonical present | 102.2 | 140.6 | +37.6% | | 256 fields, canonical absent | 111.5 | 147.0 | +31.8% (approx.) | | 256 fields, legacy fallback present | 196.5 | 366.3 | +86.4% | | 256 fields, legacy fallback absent | 201.7 | 367.0 | +82.0% | The correctness assertion for a canonical U+10000 lookup changes from false on master to true with this patch. Following parquet-java's implementation, each canonical binary-search probe decodes the metadata key and re-encodes it as UTF-8 instead of maintaining a separate raw-metadata comparator. This keeps the implementation simple but adds allocations: the measured 256-field canonical cases regress by 32-38%, and the legacy fallback cases regress by 82-86%. The 16-field cases improve by about 51% because they now use binary search instead of the previous linear scan. One canonical-absent fork was noisy, so that aggregate is marked approximate. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes #58239 from peterxcli/SPARK-58949-variant-utf8-order. Authored-by: peterxcli <peterxcli@gmail.com> Signed-off-by: Chao Sun <chao@openai.com>
sunchao
pushed a commit
to apache/spark
that referenced
this pull request
Aug 24, 2026
…ct fields ### What changes were proposed in this pull request? This PR fixes [SPARK-58949](https://issues.apache.org/jira/browse/SPARK-58949) by: - sorting newly written Variant object fields by unsigned lexicographic UTF-8 bytes, as required by the [Variant encoding specification](https://github.com/apache/parquet-format/blob/24102ed5c56e51b610a4897e5f79e76e43732d1d/VariantEncoding.md#L449-L463); - using binary search for object lookup at every object size and comparing UTF-8-encoded keys using unsigned byte ordering; - retrying lookup with Java UTF-16 order when needed so values written by older Spark versions remain readable; and - accepting both canonical UTF-8 order and legacy UTF-16 order during schema validation, while preserving the established schema field order. This follows the compatibility direction discussed in [apache/parquet-java#3736](apache/parquet-java#3736). ### Why are the changes needed? The Variant specification orders object keys by unsigned UTF-8 bytes, but Spark used String.compareTo, which orders UTF-16 code units. These orders differ for some valid keys. For example, UTF-16 places U+10000 before U+FFFF, while unsigned UTF-8 places U+FFFF first. As a result, Spark wrote non-canonical Variant objects and could miss fields when binary-searching canonical values produced by another implementation. ### Does this PR introduce _any_ user-facing change? Yes. Newly written Variant objects use the specification's unsigned UTF-8 field order. Spark continues to read affected values written in the legacy UTF-16 order, and schema output keeps its existing field order. ### How was this patch tested? Added regressions for canonical and legacy object lookup, nested objects, schema_of_variant, and Parquet shredding-schema inference: build/sbt \ 'catalyst/testOnly *VariantExpressionSuite -- -z "SPARK-58949"' \ 'sql/testOnly *VariantInferShreddingSuite -- -z "SPARK-58949"' Both suites passed (1 test each). The affected modules also passed Java checkstyle and main/test scalastyle. I also ran a temporary lookup microbenchmark on an Apple M4 with Zulu OpenJDK 21.0.6, comparing upstream/master at 9da9f8d with this patch at b3a050b. Each result is the median of three alternating JVM fork medians. Each fork used a fixed 2 GiB heap, 5 seconds of warmup per case, and 9 measured rounds of 5,000,000 lookups. Object construction was excluded. Lower is better. | Object / lookup | master (ns/op) | patch (ns/op) | Change | | --- | ---: | ---: | ---: | | 16 fields, ASCII present | 193.7 | 94.6 | -51.2% | | 16 fields, ASCII absent | 180.3 | 87.2 | -51.6% | | 256 fields, canonical present | 102.2 | 140.6 | +37.6% | | 256 fields, canonical absent | 111.5 | 147.0 | +31.8% (approx.) | | 256 fields, legacy fallback present | 196.5 | 366.3 | +86.4% | | 256 fields, legacy fallback absent | 201.7 | 367.0 | +82.0% | The correctness assertion for a canonical U+10000 lookup changes from false on master to true with this patch. Following parquet-java's implementation, each canonical binary-search probe decodes the metadata key and re-encodes it as UTF-8 instead of maintaining a separate raw-metadata comparator. This keeps the implementation simple but adds allocations: the measured 256-field canonical cases regress by 32-38%, and the legacy fallback cases regress by 82-86%. The 16-field cases improve by about 51% because they now use binary search instead of the previous linear scan. One canonical-absent fork was noisy, so that aggregate is marked approximate. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes #58239 from peterxcli/SPARK-58949-variant-utf8-order. Authored-by: peterxcli <peterxcli@gmail.com> Signed-off-by: Chao Sun <chao@openai.com> (cherry picked from commit 2a61a79) Signed-off-by: Chao Sun <chao@openai.com>
peterxcli
added a commit
to peterxcli/parquet-java
that referenced
this pull request
Aug 25, 2026
… 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>
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
2 participants
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 spec requires the field ids in an object's header to be sorted by the
UTF-8 byte order of the field names, so a reader can binary-search them.
VariantBuildersorted the fields — andVariant.getFieldByKeybinary-searched them —using
String.compareTo, which orders by UTF-16 code units, not UTF-8 bytes.The two orderings are identical for all field names in the Basic Multilingual Plane, but
they diverge for names containing supplementary-plane characters (U+10000 and above):
String.compareToorders a leading high surrogate (0xD800–0xDBFF) before code points inU+E000..U+FFFF, whereas UTF-8 byte order (and the spec) orders them after. Consequences:
violates the spec.
that object can fail to find fields.
object produced by a spec-compliant writer.
The bug only surfaces when an object both contains a supplementary-plane key and is large
enough to take the binary-search path, so it has gone unnoticed.
What changes are included in this PR?
VariantUtil.encodeKey(String)andVariantUtil.compareKeys(byte[], byte[]), which orderfield names by unsigned lexicographic UTF-8 byte order
VariantBuilder.FieldEntry.compareToandVariant.getFieldByKeynow use that comparisontestObjectKeysSortedByUtf8ByteOrderandtestLargeObjectBinarySearchWithSupplementaryKeyAre these changes tested?
Two new tests in
TestVariantObjectBuilder:testObjectKeysSortedByUtf8ByteOrder— builds an object with keys U+FFFF (EF BF BF) andU+10000 (
F0 90 80 80) appended in reverse and asserts the encoded field order is U+FFFFthen U+10000 (UTF-8 order), which the previous
compareToreversed.testLargeObjectBinarySearchWithSupplementaryKey— a 42-field object (aboveBINARY_SEARCH_THRESHOLD) mixing ASCII keys with U+FFFF and U+10000, assertinggetFieldByKeyresolves both through the binary-search path.Are there any user-facing changes?
Closes #GH-3735