cshuo commented on code in PR #19414:
URL: https://github.com/apache/hudi/pull/19414#discussion_r3682227143
##########
hudi-io/src/test/java/org/apache/hudi/common/util/TestStringUtils.java:
##########
@@ -323,6 +323,66 @@ public void
testCompareUtf8BytesEmptyPrefixAndIdenticalStrings() {
assertEquals(0, StringUtils.compareUtf8Bytes("abc", "abc"));
Review Comment:
Fixed in 59f4e8db2478. Added an equal-content comparison using two distinct
String instances so the full scan and length tie-break are exercised.
##########
hudi-io/src/test/java/org/apache/hudi/common/util/TestStringUtils.java:
##########
@@ -323,6 +323,66 @@ public void
testCompareUtf8BytesEmptyPrefixAndIdenticalStrings() {
assertEquals(0, StringUtils.compareUtf8Bytes("abc", "abc"));
}
+ @Test
+ public void testCompareUtf8BytesMatchesEncodedByteOrder() {
+ String[] alphabet = {
+ // One-byte UTF-8 characters, including the upper boundary.
+ "?",
+ "a",
+ String.valueOf((char) 0x007F),
+ // Two-byte UTF-8 lower and upper boundaries.
+ String.valueOf((char) 0x0080),
+ String.valueOf((char) 0x07FF),
+ // Three-byte UTF-8 boundaries around the surrogate range, plus U+FFFD.
+ String.valueOf((char) 0x0800),
+ String.valueOf((char) 0xD7FF),
+ String.valueOf((char) 0xE000),
+ String.valueOf((char) 0xFFFD),
+ // Four-byte UTF-8 supplementary characters, including two sharing a
high surrogate.
+ "😀", // U+1F600
+ new String(Character.toChars(0x20000)),
+ new String(Character.toChars(0x20001)),
+ new String(Character.toChars(0x10FFFF))
+ };
+
+ // Generate every sequence of one to three code points from the alphabet.
This covers cases
+ // where strings differ before, within, or after a supplementary character.
+ List<String> values = new ArrayList<>();
+ values.add("");
+ for (String first : alphabet) {
+ values.add(first);
+ for (String second : alphabet) {
+ values.add(first + second);
+ for (String third : alphabet) {
+ values.add(first + second + third);
+ }
+ }
+ }
+
+ // Compare only the sign because Comparator does not prescribe the
magnitude of its result.
+ for (String left : values) {
+ for (String right : values) {
+ assertEquals(
+ Integer.signum(compareEncodedUtf8Bytes(left, right)),
+ Integer.signum(StringUtils.compareUtf8Bytes(left, right)));
Review Comment:
Fixed in 59f4e8db2478. Added a lazy failure message that reports both
operands as Unicode code-point arrays.
##########
hudi-io/src/test/java/org/apache/hudi/common/util/TestStringUtils.java:
##########
@@ -323,6 +323,66 @@ public void
testCompareUtf8BytesEmptyPrefixAndIdenticalStrings() {
assertEquals(0, StringUtils.compareUtf8Bytes("abc", "abc"));
}
+ @Test
+ public void testCompareUtf8BytesMatchesEncodedByteOrder() {
+ String[] alphabet = {
+ // One-byte UTF-8 characters, including the upper boundary.
+ "?",
+ "a",
+ String.valueOf((char) 0x007F),
+ // Two-byte UTF-8 lower and upper boundaries.
+ String.valueOf((char) 0x0080),
+ String.valueOf((char) 0x07FF),
+ // Three-byte UTF-8 boundaries around the surrogate range, plus U+FFFD.
+ String.valueOf((char) 0x0800),
+ String.valueOf((char) 0xD7FF),
+ String.valueOf((char) 0xE000),
+ String.valueOf((char) 0xFFFD),
+ // Four-byte UTF-8 supplementary characters, including two sharing a
high surrogate.
+ "😀", // U+1F600
+ new String(Character.toChars(0x20000)),
+ new String(Character.toChars(0x20001)),
+ new String(Character.toChars(0x10FFFF))
+ };
+
+ // Generate every sequence of one to three code points from the alphabet.
This covers cases
Review Comment:
Fixed in 59f4e8db2478. Updated the comment to say zero to three code points.
##########
hudi-io/src/main/java/org/apache/hudi/common/util/StringUtils.java:
##########
@@ -137,23 +137,30 @@ public static byte[] getUTF8Bytes(String str) {
* <p>Neither argument may be {@code null}; like {@link
String#compareTo(String)}, a {@code null}
* argument throws {@link NullPointerException}.
*
- * <p>Assumes well-formed UTF-16 input: {@code String#getBytes(UTF_8)}
replaces unpaired surrogates
- * with {@code '?'}, so strings differing only in unpaired surrogates
compare equal.
+ * <p>This comparison does not materialize the UTF-8 byte arrays. It
compares UTF-16 code units
+ * directly and handles supplementary characters specially to preserve UTF-8
byte order.
*
- * <p>Note: encodes both strings to UTF-8 on every call; for very large
sorts consider
- * pre-encoding keys to byte arrays once and comparing those.
+ * <p>Ported from Google Firebase Firestore's {@code compareUtf8Strings}.
*/
public static int compareUtf8Bytes(String s1, String s2) {
- byte[] b1 = getUTF8Bytes(s1);
- byte[] b2 = getUTF8Bytes(s2);
- int len = Math.min(b1.length, b2.length);
- for (int i = 0; i < len; i++) {
- int cmp = (b1[i] & 0xFF) - (b2[i] & 0xFF);
- if (cmp != 0) {
- return cmp;
+ // Source:
https://github.com/firebase/firebase-android-sdk/blame/f05e4bcb7f86f3b21833b1e0960d793b800d38d1/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java#L76-L132
Review Comment:
Fixed in 59f4e8db2478. Switched the pinned source link to blob and replaced
the IDE-specific suppression marker with a plain explanation of the intentional
identity check.
--
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]
