rayokota opened a new pull request, #3736: URL: https://github.com/apache/parquet-java/pull/3736
<!-- Thanks for opening a pull request! If you're new to Parquet-Java, information on how to contribute can be found here: https://parquet.apache.org/docs/contribution-guidelines/contributing Please open a GitHub issue for this pull request: https://github.com/apache/parquet-java/issues/new/choose and format pull request title as below: GH-${GITHUB_ISSUE_ID}: ${SUMMARY} or simply use the title below if it is a minor issue: MINOR: ${SUMMARY} --> ### 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. `VariantBuilder` sorted the fields — and `Variant.getFieldByKey` binary-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.compareTo` orders a leading high surrogate (0xD800–0xDBFF) before code points in U+E000..U+FFFF, whereas UTF-8 byte order (and the spec) orders them after. Consequences: - An object parquet-java builds with such keys has field ids sorted in an order that violates the spec. - A spec-compliant reader (e.g. the Apache Arrow C++/Rust/Go Variant readers) binary-searching that object can fail to find fields. - Conversely, parquet-java's own binary search fails to find a supplementary-plane key in an 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? - Add `VariantUtil.compareKeys(String, String)`, comparing two field names by the unsigned lexicographic order of their UTF-8 encoded bytes (`Arrays.compareUnsigned` over `getBytes(UTF_8)`). - Use it at the two order-dependent sites, which must agree: - `VariantBuilder.FieldEntry.compareTo` — the field sort in `endObject()`. - `Variant.getFieldByKey` — the binary-search comparison. - The small-object linear-scan branch already uses `String.equals` (order-independent) and is unchanged; the metadata dictionary is written unsorted, so it is unaffected. ### Are these changes tested? Two new tests in `TestVariantObjectBuilder`: - `testObjectKeysSortedByUtf8ByteOrder` — builds an object with keys U+FFFF (`EF BF BF`) and U+10000 (`F0 90 80 80`) appended in reverse and asserts the encoded field order is U+FFFF then U+10000 (UTF-8 order), which the previous `compareTo` reversed. - `testLargeObjectBinarySearchWithSupplementaryKey` — a 42-field object (above `BINARY_SEARCH_THRESHOLD`) mixing ASCII keys with U+FFFF and U+10000, asserting `getFieldByKey` resolves both through the binary-search path. ### Are there any user-facing changes? <!-- Please uncomment the line below and replace ${GITHUB_ISSUE_ID} with the actual Github issue id. --> <!-- Closes #${GITHUB_ISSUE_ID} --> -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
