laskoviymishka commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3862487534
##########
api/src/main/java/org/apache/iceberg/variants/VariantUtil.java:
##########
@@ -99,13 +100,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 = Comparators.charSequences().compare(key, value);
Review Comment:
I think there's a subtle backward-compat issue here. `find()` now
binary-searches with `Comparators.charSequences()` (UTF-8 order), but any
metadata already on disk that has `sorted_strings=1` was written by old code
using `String.compareTo` (UTF-16 order). For field names with supplementary
code points (> U+FFFF) the two disagree — old code writes `[U+10000, U+FFFF]`
flagged sorted, and the new search compares U+FFFF < U+10000, never finds it,
and `id()` returns -1 for a name that's actually present.
Blast radius is tiny — supplementary chars in Variant field names are
basically unheard of — but it's a silent wrong-answer-on-read on upgrade, and
`SerializedObject.get()` has no sorted-flag guard at all, so it always takes
this path. I'd want us to make an explicit call: either accept the risk and
document it (release/migration note: rewrite Variant data with
supplementary-char field names written before this fix), or add a linear-scan
fallback when the sorted search misses. Either is fine, but I'd rather it be
deliberate than silent. wdyt?
##########
core/src/main/java/org/apache/iceberg/variants/Variants.java:
##########
@@ -57,7 +58,7 @@ public static VariantMetadata metadata(Collection<String>
fieldNames) {
for (String name : fieldNames) {
nameBuffers[pos] =
ByteBuffer.wrap(name.getBytes(StandardCharsets.UTF_8));
dataSize += nameBuffers[pos].remaining();
- if (last != null && last.compareTo(name) >= 0) {
+ if (last != null && Comparators.charSequences().compare(last, name) >=
0) {
Review Comment:
This comparator is now effectively the spec's `sorted_strings` ordering
primitive, but `Comparators.charSequences()` gets called inline at every
field-name comparison in the subsystem (here, `VariantUtil.find`,
`ShreddedObject` twice, `RecordConverter`, `ParquetMetrics`) with nothing
saying why this specific ordering is required. My worry is a future refactor
"simplifies" one of them back to `String::compareTo` and quietly re-breaks the
flag.
Could we pull it into a single named constant — something like
`FIELD_NAME_ORDER` with a javadoc pointing at VariantEncoding.md's UTF-8
byte-order requirement — and use it everywhere? If we also let `find` take a
`Comparator<String>` param, the constant flows straight through and the binary
search gets easier to unit-test in isolation. wdyt?
##########
core/src/test/java/org/apache/iceberg/variants/TestShreddedObject.java:
##########
@@ -65,6 +65,25 @@ public void testShreddedFields() {
assertThat(object.get("c").asPrimitive().get()).isEqualTo(new
BigDecimal("12.21"));
}
+ @Test
+ public void testShreddedFieldOrderFollowsUtf8ByteOrder() {
+ // U+FFFF is 3 UTF-8 bytes (EF BF BF), U+10000 is 4 (F0 90 80 80); they
order oppositely in
+ // UTF-16
+ String threeByteName = new String(Character.toChars(0xFFFF));
+ String fourByteName = new String(Character.toChars(0x10000));
+ VariantMetadata metadata = Variants.metadata(threeByteName, fourByteName);
+ Map<String, VariantValue> fields =
+ ImmutableMap.of(threeByteName, Variants.of(1), fourByteName,
Variants.of(2));
+ ShreddedObject object = createShreddedObject(metadata, fields);
+
+ VariantValue value = roundTripMinimalBuffer(object, metadata);
+
+ assertThat(value).isInstanceOf(SerializedObject.class);
+ SerializedObject actual = (SerializedObject) value;
+ assertThat(actual.get(threeByteName).asPrimitive().get()).isEqualTo(1);
+ assertThat(actual.get(fourByteName).asPrimitive().get()).isEqualTo(2);
Review Comment:
This test doesn't actually exercise the fix. It only checks that `get()`
returns the right values, which passes on the unpatched code too — old
`Variants.metadata()` sees `U+FFFF > U+10000` in UTF-16, flags the dictionary
`sorted=false`, and lookups fall back to linear scan, so nothing here depends
on the new ordering.
To pin the regression I'd assert the actual order —
`assertThat(actual.fieldNames()).containsExactly(threeByteName, fourByteName)`
and that the dictionary ID for `threeByteName` is 0. That fails on old code,
which is the point.
##########
core/src/test/java/org/apache/iceberg/variants/TestVariantMetadataFieldOrdering.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.variants;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestVariantMetadataFieldOrdering {
+
+ // U+FFFF encodes to 3 UTF-8 bytes (EF BF BF), U+10000 to 4 (F0 90 80 80)
+ private static final String NAME_3_BYTE = new
String(Character.toChars(0xFFFF));
+ private static final String NAME_4_BYTE = new
String(Character.toChars(0x10000));
+
+ @Test
+ public void utf8OrderedDictionaryIsSortedAndOrdered() {
+ SerializedMetadata metadata =
+ (SerializedMetadata) Variants.metadata(ImmutableList.of(NAME_3_BYTE,
NAME_4_BYTE));
+
+ assertThat(metadata.isSorted()).isTrue();
+ assertThat(metadata.get(0)).isEqualTo(NAME_3_BYTE);
+ assertThat(metadata.get(1)).isEqualTo(NAME_4_BYTE);
+ assertThat(metadata.id(NAME_3_BYTE)).isEqualTo(0);
+ assertThat(metadata.id(NAME_4_BYTE)).isEqualTo(1);
+ }
+
+ @Test
+ public void utf16OrderedDictionaryIsNotFlaggedSorted() {
+ // UTF-16 order differs from UTF-8: high surrogate D800 sorts before FFFF
+ List<String> utf16Ordered =
+ ImmutableList.of(NAME_3_BYTE,
NAME_4_BYTE).stream().sorted().collect(Collectors.toList());
+ assertThat(utf16Ordered).containsExactly(NAME_4_BYTE, NAME_3_BYTE);
+
+ SerializedMetadata metadata = (SerializedMetadata)
Variants.metadata(utf16Ordered);
+
+ assertThat(metadata.isSorted()).isFalse();
Review Comment:
Nice to have the "UTF-16 order isn't flagged sorted" case covered here. The
mirror case is the one that actually bites on upgrade though: a dictionary that
*is* flagged `sorted_strings=1` but written in UTF-16 order (what old code
produced for supplementary-char names). Right now nothing pins how `id()`
behaves on that input.
If we go the document-and-accept route on the compat issue, I'd hand-craft
that buffer here and assert the current behavior so it's at least intentional;
if we add a linear-scan fallback, this is where I'd prove it recovers the
field. Ties back to the `VariantUtil.find` thread.
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/RecordConverter.java:
##########
@@ -581,7 +582,9 @@ protected Variant convertVariantValue(Object value) {
}
List<String> sortedFieldNames =
-
collectFieldNames(value).stream().sorted().collect(Collectors.toList());
+ collectFieldNames(value).stream()
+ .sorted(Comparators.charSequences())
Review Comment:
This is the only production change in kafka-connect, but nothing in the
suite exercises it — existing tests use ASCII names where natural order and
`charSequences()` agree, so reverting this line wouldn't fail anything. I'd add
a `convertVariantValue` case with a map whose keys include a supplementary-char
name (U+10000 alongside a U+FFFF-level one) and assert the metadata is flagged
sorted with the right ids. Same gap applies to the `ParquetMetrics` one-liner
if that suite is ASCII-only too.
--
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]
