cshuo commented on code in PR #19414:
URL: https://github.com/apache/hudi/pull/19414#discussion_r3682226094
##########
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}.
Review Comment:
Fixed in 59f4e8db2478. Added the Google Firebase Android SDK attribution for
the port to the root LICENSE.
##########
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.
Review Comment:
Fixed in 59f4e8db2478. Restored the well-formed UTF-16 precondition in the
Javadoc and added regression coverage for the documented unpaired-surrogate
behavior.
##########
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
+ // noinspection StringEquality
+ if (s1 == s2) {
Review Comment:
Fixed in 59f4e8db2478. The identity shortcut now applies only to non-null
references, preserving NullPointerException for all three null combinations,
which are covered by tests.
##########
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) {
Review Comment:
Tracked separately in #19417 with both affected LSM comparison sites and a
non-ASCII merge regression-test plan. No LSM changes were added to this PR.
##########
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)));
+ }
+ }
+ }
+
+ private static int compareEncodedUtf8Bytes(String left, String right) {
Review Comment:
Fixed in 59f4e8db2478. The exhaustive test now uses pre-encoded
UTF8StringKey instances and their production HFile comparison path as the
oracle.
--
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]
