nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3983210712
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -99,13 +99,13 @@ static String readString(ByteBuffer buffer, int offset, int
length) {
}
}
- static <T extends Comparable<T>> int find(int size, T key, Function<Integer,
T> resolve) {
+ static int find(int size, String key, Function<Integer, String> resolve) {
int low = 0;
int high = size - 1;
while (low <= high) {
int mid = (low + high) >>> 1;
- T value = resolve.apply(mid);
- int cmp = key.compareTo(value);
+ String value = resolve.apply(mid);
+ int cmp = VariantMetadata.FIELD_NAME_ORDER.compare(key, value);
Review Comment:
The miss is only reachable for a file that predates this change and has
Variant field names with supplementary-plane code points (> U+FFFF), which
effectively never occur. The failure is bounded: `find` returns only on an
exact match, so the worst case is a false "not found" (`id()` returns -1),
never a wrong value.
To keep it deliberate rather than silent, I added a test
`utf16OrderedDictionaryFlaggedSortedMissesLookup` which pins that behavior, and
the PR description documents the break with remediation (rewrite Variant data
written before this change that has supplementary-char field names). A
linear-scan-on-miss fallback stays a one-line follow-up if anyone ever hits it,
and that test flips to asserting recovery, but I would rather not add a second
lookup path for a case that requires field names that do not occur in practice.
As mentioned in a previous comment thread, it wasn't easy to implement
efficiently.
This also matches the direction from the community sync: fix the write path,
do not carry a read-side fallback.
CC: @RussellSpitzer who looked at this during the sync as well.
--
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]
