You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nssalian (via GitHub)" <gi...@apache.org> on 2026/08/19 20:42:05 UTC
[PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
nssalian opened a new pull request, #17726:
URL: https://github.com/apache/iceberg/pull/17726
## Rationale for this change
Variant metadata stores field names in a dictionary. When its `sorted_strings` flag is set, [VariantEncoding.md](https://github.com/apache/parquet-format/blob/master/VariantEncoding.md) requires those names to be in UTF-8 byte order.
Iceberg sorts and looks them up with Java's `String.compareTo`, which is UTF-16 order. For ASCII and most characters the two orders are identical, but they differ for characters above U+FFFF. So Iceberg can flag a dictionary as `sorted` when it is not actually UTF-8-ordered. Iceberg reads its own files fine because it sorts and searches the same way, but a spec-conforming reader (iceberg-go, iceberg-rust) searches by UTF-8 and would fail to find those fields.
## Changes
Sort and look up Variant field names with the existing UTF-8 comparator `Comparators.charSequences()` at every site:
- `Variants.metadata` - sets the dictionary `sorted_strings` flag
- `VariantUtil.find` - reader lookup over field names
- `ShreddedObject` - shredded object field order
- `RecordConverter` (kafka-connect) and `ParquetMetrics` - sort field names before building the dictionary
No on-disk format or spec change - this conforms Iceberg to the existing spec. Files with only ASCII or common field names are unaffected; only names above U+FFFF change ordering.
## Testing
`TestVariantMetadataFieldOrdering` and a new `TestShreddedObject` case check that a dictionary is flagged `sorted` only when it is actually UTF-8-ordered, and that fields with characters above U+FFFF are found on read. The tests fail on the old `String.compareTo` code and pass on the fix (verified by reverting the change and watching them go red).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r4004705399
##########
api/src/main/java/org/apache/iceberg/variants/VariantMetadata.java:
##########
@@ -19,10 +19,15 @@
package org.apache.iceberg.variants;
import java.nio.ByteBuffer;
+import java.util.Comparator;
import java.util.NoSuchElementException;
+import org.apache.iceberg.types.Comparators;
/** A variant metadata dictionary. */
public interface VariantMetadata {
+ /** Unsigned UTF-8 byte order for field names, per VariantEncoding.md's sorted_strings rule. */
+ Comparator<CharSequence> FIELD_NAME_ORDER = Comparators.charSequences();
Review Comment:
The compat call reads as settled now that the PR body's Compatibility section spells it out — accept the break, rewrite affected data — so I'm not asking for a fallback.
The one thing I'd add, and it's not a blocker: that reasoning lives in the PR body and the miss test, but not in the code that relies on it. I'd either extend this javadoc or drop a line on the `isSorted` branch in `SerializedMetadata.id()` noting we take `sorted_strings=1` at face value per spec, so a pre-PR dict flagged sorted but laid out in UTF-16 (supplementary-plane names) will miss and must be rewritten — keeps it in front of the next reader who lands on the search. wdyt?
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestVariantMetrics.java:
##########
@@ -607,6 +607,34 @@ public void testShreddedObject() throws IOException {
.isEqualTo(Map.of(1, Types.LongType.get(), 2, Types.VariantType.get()));
}
+ @Test
+ public void testShreddedObjectBoundFieldsOrderedByUtf8() throws IOException {
+ // U+FFFF is 3 UTF-8 bytes (EF BF BF), U+10000 is 4 (F0 90 80 80); UTF-16 orders them oppositely
+ String threeByteName = new String(Character.toChars(0xFFFF));
+ String fourByteName = new String(Character.toChars(0x10000));
+ VariantMetadata objectMetadata = Variants.metadata(threeByteName, fourByteName);
+
+ ShreddedObject object0 = Variants.object(objectMetadata);
+ object0.put(threeByteName, Variants.of(1));
+ object0.put(fourByteName, Variants.of(2));
+ ShreddedObject object1 = Variants.object(objectMetadata);
+ object1.put(threeByteName, Variants.of(3));
+ object1.put(fourByteName, Variants.of(4));
+
+ Metrics metrics =
+ writeParquet(
+ (id, name) -> ParquetVariantUtil.toParquetSchema(object0),
+ Variant.of(objectMetadata, object0),
+ Variant.of(objectMetadata, object1));
+
+ String threePath = "$['" + threeByteName + "']";
+ String fourPath = "$['" + fourByteName + "']";
+ VariantObject lowerBounds = Variant.from(metrics.lowerBounds().get(2)).value().asObject();
+ assertThat(lowerBounds.fieldNames()).containsExactly(threePath, fourPath);
Review Comment:
Small non-blocker, and it's the same shape as the test worry I raised in round 1: this asserts the field-name order but doesn't actually pin the ParquetMetrics fix. That `[threePath, fourPath]` ordering comes from `SerializationState`'s tree map, so if `Sets.newTreeSet(FIELD_NAME_ORDER)` in `ParquetMetrics.variant()` were reverted to a natural-order set, the dict would come out `isSorted=false` but `fieldNames()` would still return this same order and the test passes.
I'd add an assert that the backing metadata is `isSorted()` (cast the lower-bound metadata to `SerializedMetadata`) so it guards the line it's here for. wdyt?
##########
core/src/test/java/org/apache/iceberg/variants/TestVariantMetadataFieldOrdering.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.nio.ByteBuffer;
+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() {
Review Comment:
Small non-blocker: these three drop the `test` prefix the rest of the package uses (the new `testShreddedFieldOrderFollowsUtf8ByteOrder` right in this PR, all of `TestShreddedObject`). Some tooling pattern-matches on it, so I'd rename to `testUtf8OrderedDictionaryIsSortedAndOrdered` and friends to stay consistent.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3938113100
##########
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:
Added assertThat(actual.fieldNames()).containsExactly(threeByteName, fourByteName), which is red on the old order. The dictionary-id assertion you mention is covered in TestVariantMetadataFieldOrdering.utf8OrderedDictionaryIsSortedAndOrdered (id(threeByteName) == 0).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3938114617
##########
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:
Added supplementary-char coverage at both: TestRecordConverter.testConvertVariantValueOrdersFieldNamesByUtf8 (a convertVariantValue map keyed by U+FFFF and U+10000, asserting the UTF-8 metadata order and the field values) and TestVariantMetrics.testShreddedObjectBoundFieldsOrderedByUtf8.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #17726:
URL: https://github.com/apache/iceberg/pull/17726
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3982968533
##########
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:
Added `utf16OrderedDictionaryFlaggedSortedMissesLookup` it builds a UTF-16-ordered dictionary, forces `sorted_strings=1`, and asserts `id()` returns -1 for the out-of-order name and 0 for the in-order one. This pins the accepted behavior in the suite. If we ever add the fallback, this assertion is the one that flips to prove recovery.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3938111283
##########
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:
Done. Added VariantMetadata.FIELD_NAME_ORDER with a javadoc pointing at VariantEncoding.md's UTF-8 byte-order rule. Cleaner and I updated the usage.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3856961036
##########
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 am checking if the linear scan is performant and I am not seeing promise. Best I could get is an O(lg n). I might hold off on this since it only hits for specific characters. Let me get back to this PR if someone does hit the edge case rather than possibly introducing a new path here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3839002071
##########
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:
I did see the `TestVariantMetadataFieldOrdering` fail on revert in my local tests but this one is missing the comparator/order assertion. Let me add your suggestion.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3938116837
##########
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:
Since we're going document-and-accept (no fallback), I did not hand-craft the sorted_strings=1 + UTF-16-order buffer. If you'd like the current behavior pinned as intentional, I can add that case asserting id() returns -1 on such a buffer - happy to include it. Ties back to the VariantUtil.find thread.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3838481037
##########
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 passes without the fix too — the old code sorted and searched consistently in UTF-16, so the round trip succeeded either way. Could you add an assertion on the stored order so it actually pins the layout?
```
assertThat(actual.fieldNames()).containsExactly(threeByteName, fourByteName);
```
On main the stored order is `[fourByteName, threeByteName]`, so this goes red without the change.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3982961265
##########
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) {
Review Comment:
Fixed all of them
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r4170426345
##########
core/src/test/java/org/apache/iceberg/variants/TestVariantMetadataFieldOrdering.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.nio.ByteBuffer;
+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() {
Review Comment:
Kept no-test-prefix. I don't think we are asking to add to newer tests. the prefixed ones are in TestShreddedObject, which already uses that style.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3938109443
##########
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:
Making it deliberate: accept and document, no fallback. The divergence is confined to field names with supplementary code points (> U+FFFF), which I want to say are effectively nonexistent in Variant field names, and find returns only on exact match so the worst case is a false "not found," never a wrong value. I prototyped a linear-scan fallback but it adds a second lookup path for a case no one is expected to hit, so I'd rather not introduce it now.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3856961036
##########
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 am checking if the linear scan is performant and I am not seeing promise. I might hold off on this since it only hits for specific characters. Let me get back to this PR if someone does hit the edge case rather than possibly introducing a new path here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#issuecomment-5624681745
@laskoviymishka thanks for the note. Wanted to clarify around the fallback a bit more here:
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3838488734
##########
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:
Question: older files were written in UTF-16 order with the sorted flag set. Since the reader now searches in UTF-8 order, could lookups miss on those files? If so, how should we handle it?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3839006602
##########
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:
lookups can miss on files written before this change, but only for field names with code points above U+FFFF (ASCII/BMP are unaffected). find only returns on an exact match, so a wrong-order search gives a false "not found," never a wrong field. I'm going to add a linear-scan fallback on a binary-search miss in SerializedMetadata.id and SerializedObject.get so we are correct for any legacy ordering.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3982964993
##########
api/src/test/java/org/apache/iceberg/variants/VariantTestUtil.java:
##########
@@ -163,7 +163,10 @@ public static ByteBuffer createMetadata(Collection<String> fieldNames, boolean s
}
int numElements = fieldNames.size();
- Stream<String> names = sortNames ? fieldNames.stream().sorted() : fieldNames.stream();
+ Stream<String> names =
+ sortNames
+ ? fieldNames.stream().sorted(VariantMetadata.FIELD_NAME_ORDER)
Review Comment:
`createObject` now sorts with `FIELD_NAME_ORDER`, matching `createMetadata`. An object built with supplementary-plane names now serializes in UTF-8 order and `SerializedObject.get()` binary-searches it correctly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#issuecomment-5962482827
Added the fallback attempts similar to the changes in Spark more recently.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r4170591739
##########
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:
Ended up adding the fallback.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Order Variant metadata dictionary and object fields by UTF-8 byte order [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #17726:
URL: https://github.com/apache/iceberg/pull/17726#discussion_r3977586419
##########
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:
This is the backward-compat read I raised last round, and it's still the one thing I'd want a deliberate call on before we merge.
New files are fine now that both the write side and this search share `FIELD_NAME_ORDER`. The gap is old files that set `sorted_strings=1` but physically ordered the dictionary in UTF-16 — only reachable with supplementary-plane field names, so a narrow blast radius, but a real one. This search now runs UTF-8 order over that UTF-16 layout, the binary search diverges, and `SerializedMetadata.id()` returns -1 for a name that's actually present. Silent wrong-answer-on-read, no exception.
A one-line linear-scan fallback in `SerializedMetadata.id()` on a binary-search miss would cover it cheaply (only on the miss path), or we document the break in a release note — either's fine, I just don't want it landing silently. wdyt?
##########
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) {
Review Comment:
Now that `find` is concrete on `String`, `Function<Integer, String>` boxes an `Integer` on every `resolve.apply(mid)` — and this is the binary-search hot path (once per step in `id()`, twice per `SerializedObject.get()`). All three call sites already fit `IntFunction<String>`, so since we're touching the signature anyway I'd switch it while we're here.
##########
api/src/test/java/org/apache/iceberg/variants/VariantTestUtil.java:
##########
@@ -163,7 +163,10 @@ public static ByteBuffer createMetadata(Collection<String> fieldNames, boolean s
}
int numElements = fieldNames.size();
- Stream<String> names = sortNames ? fieldNames.stream().sorted() : fieldNames.stream();
+ Stream<String> names =
+ sortNames
+ ? fieldNames.stream().sorted(VariantMetadata.FIELD_NAME_ORDER)
Review Comment:
`createMetadata` sorts by `FIELD_NAME_ORDER` now, but `createObject` down at line 246 still uses plain `.sorted()` (UTF-16). With `NAME_3_BYTE`/`NAME_4_BYTE` now first-class test data, a test that builds an object with supplementary names and then calls `SerializedObject.get()` would binary-search over mis-ordered fields and fail. I'd switch 246 to `.sorted(VariantMetadata.FIELD_NAME_ORDER)` for consistency while we're in here.
##########
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:
This nicely documents the unsorted/linear path. The case it doesn't cover is the hazard from the `find` comment: a dict flagged `sorted_strings=1` but laid out in UTF-16 order. I'd add a test that hand-builds exactly that buffer and asserts today's behavior (`id()` returns -1), so whichever way we go on the fallback it's pinned by a test rather than invisible in the suite.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org