danny0405 commented on code in PR #19414:
URL: https://github.com/apache/hudi/pull/19414#discussion_r3681748814
##########
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:
Confirmed on head 769684d: Java encodes a lone \uD800 as byte 0x3F, but this
comparator orders it after z, so the method no longer universally matches the
bytes HFile receives. The well-formed-UTF-16 precondition and a malformed-input
regression test should stay explicit.
##########
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:
Confirmed against the pinned Firebase source: this control flow is copied
from a file carrying Copyright 2018 Google LLC and an Apache-2.0 header, while
the Hudi root LICENSE records comparable method-level ports. Please add the
attribution before merge.
##########
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:
Confirmed on head 769684d: (null, null) now returns 0, whereas either
one-sided-null case throws and the previous implementation rejected all nulls.
This is a real documented-contract regression; please preserve fail-fast
behavior and test all three null combinations.
--
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]
