Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use lazy encoding in utf-8 encoded string comparison #6706

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+364 −43
Open

Use lazy encoding in utf-8 encoded string comparison #6706

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
44050e9
Use lazy encoding in utf-8 encoded string comparison
milaGGL Feb 18, 2025
2ca341c
Update CHANGELOG.md
milaGGL Feb 19, 2025
003a529
Merge branch 'main' into mila/fix-utf8-encoded-string-comparison
milaGGL Feb 19, 2025
b740bcc
UtilTest.java: add test for compareUtf8Strings(), which fails and nee…
dconeybe Feb 19, 2025
4c4d1a7
UtilTest.java: make sure that most generated strings have a common pr…
dconeybe Feb 19, 2025
ec37ebd
fix the failing test
milaGGL Feb 19, 2025
391867a
simplify the code
milaGGL Feb 19, 2025
8159f25
format
milaGGL Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions firebase-firestore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# Unreleased
* [fixed] Use lazy encoding in UTF-8 encoded byte comparison for strings to solve performance issues. [#6706](//github.com/firebase/firebase-android-sdk/pull/6706)


# 25.1.2
Expand Down
160 changes: 120 additions & 40 deletions firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1658,17 +1658,33 @@ public void sdkOrdersQueryByDocumentIdTheSameWayOnlineAndOffline() {
public void snapshotListenerSortsUnicodeStringsAsServer() {
Map<String, Map<String, Object>> testDocs =
map(
"a", map("value", "Łukasiewicz"),
"b", map("value", "Sierpiński"),
"c", map("value", "岩澤"),
"d", map("value", "🄟"),
"e", map("value", "Ｐ"),
"f", map("value", "︒"),
"g", map("value", "🐵"));
"a",
map("value", "Łukasiewicz"),
"b",
map("value", "Sierpiński"),
"c",
map("value", "岩澤"),
"d",
map("value", "🄟"),
"e",
map("value", "Ｐ"),
"f",
map("value", "︒"),
"g",
map("value", "🐵"),
"h",
map("value", "你好"),
"i",
map("value", "你顥"),
"j",
map("value", "😁"),
"k",
map("value", "😀"));

CollectionReference colRef = testCollectionWithDocs(testDocs);
Query orderedQuery = colRef.orderBy("value");
List<String> expectedDocIds = Arrays.asList("b", "a", "c", "f", "e", "d", "g");
List<String> expectedDocIds =
Arrays.asList("b", "a", "h", "i", "c", "f", "e", "d", "g", "k", "j");

QuerySnapshot getSnapshot = waitFor(orderedQuery.get());
List<String> getSnapshotDocIds =
Expand Down Expand Up @@ -1699,17 +1715,33 @@ public void snapshotListenerSortsUnicodeStringsAsServer() {
public void snapshotListenerSortsUnicodeStringsInArrayAsServer() {
Map<String, Map<String, Object>> testDocs =
map(
"a", map("value", Arrays.asList("Łukasiewicz")),
"b", map("value", Arrays.asList("Sierpiński")),
"c", map("value", Arrays.asList("岩澤")),
"d", map("value", Arrays.asList("🄟")),
"e", map("value", Arrays.asList("Ｐ")),
"f", map("value", Arrays.asList("︒")),
"g", map("value", Arrays.asList("🐵")));
"a",
map("value", Arrays.asList("Łukasiewicz")),
"b",
map("value", Arrays.asList("Sierpiński")),
"c",
map("value", Arrays.asList("岩澤")),
"d",
map("value", Arrays.asList("🄟")),
"e",
map("value", Arrays.asList("Ｐ")),
"f",
map("value", Arrays.asList("︒")),
"g",
map("value", Arrays.asList("🐵")),
"h",
map("value", Arrays.asList("你好")),
"i",
map("value", Arrays.asList("你顥")),
"j",
map("value", Arrays.asList("😁")),
"k",
map("value", Arrays.asList("😀")));

CollectionReference colRef = testCollectionWithDocs(testDocs);
Query orderedQuery = colRef.orderBy("value");
List<String> expectedDocIds = Arrays.asList("b", "a", "c", "f", "e", "d", "g");
List<String> expectedDocIds =
Arrays.asList("b", "a", "h", "i", "c", "f", "e", "d", "g", "k", "j");

QuerySnapshot getSnapshot = waitFor(orderedQuery.get());
List<String> getSnapshotDocIds =
Expand Down Expand Up @@ -1740,17 +1772,33 @@ public void snapshotListenerSortsUnicodeStringsInArrayAsServer() {
public void snapshotListenerSortsUnicodeStringsInMapAsServer() {
Map<String, Map<String, Object>> testDocs =
map(
"a", map("value", map("foo", "Łukasiewicz")),
"b", map("value", map("foo", "Sierpiński")),
"c", map("value", map("foo", "岩澤")),
"d", map("value", map("foo", "🄟")),
"e", map("value", map("foo", "Ｐ")),
"f", map("value", map("foo", "︒")),
"g", map("value", map("foo", "🐵")));
"a",
map("value", map("foo", "Łukasiewicz")),
"b",
map("value", map("foo", "Sierpiński")),
"c",
map("value", map("foo", "岩澤")),
"d",
map("value", map("foo", "🄟")),
"e",
map("value", map("foo", "Ｐ")),
"f",
map("value", map("foo", "︒")),
"g",
map("value", map("foo", "🐵")),
"h",
map("value", map("foo", "你好")),
"i",
map("value", map("foo", "你顥")),
"j",
map("value", map("foo", "😁")),
"k",
map("value", map("foo", "😀")));

CollectionReference colRef = testCollectionWithDocs(testDocs);
Query orderedQuery = colRef.orderBy("value");
List<String> expectedDocIds = Arrays.asList("b", "a", "c", "f", "e", "d", "g");
List<String> expectedDocIds =
Arrays.asList("b", "a", "h", "i", "c", "f", "e", "d", "g", "k", "j");

QuerySnapshot getSnapshot = waitFor(orderedQuery.get());
List<String> getSnapshotDocIds =
Expand Down Expand Up @@ -1781,17 +1829,33 @@ public void snapshotListenerSortsUnicodeStringsInMapAsServer() {
public void snapshotListenerSortsUnicodeStringsInMapKeyAsServer() {
Map<String, Map<String, Object>> testDocs =
map(
"a", map("value", map("Łukasiewicz", "foo")),
"b", map("value", map("Sierpiński", "foo")),
"c", map("value", map("岩澤", "foo")),
"d", map("value", map("🄟", "foo")),
"e", map("value", map("Ｐ", "foo")),
"f", map("value", map("︒", "foo")),
"g", map("value", map("🐵", "foo")));
"a",
map("value", map("Łukasiewicz", "foo")),
"b",
map("value", map("Sierpiński", "foo")),
"c",
map("value", map("岩澤", "foo")),
"d",
map("value", map("🄟", "foo")),
"e",
map("value", map("Ｐ", "foo")),
"f",
map("value", map("︒", "foo")),
"g",
map("value", map("🐵", "foo")),
"h",
map("value", map("你好", "foo")),
"i",
map("value", map("你顥", "foo")),
"j",
map("value", map("😁", "foo")),
"k",
map("value", map("😀", "foo")));

CollectionReference colRef = testCollectionWithDocs(testDocs);
Query orderedQuery = colRef.orderBy("value");
List<String> expectedDocIds = Arrays.asList("b", "a", "c", "f", "e", "d", "g");
List<String> expectedDocIds =
Arrays.asList("b", "a", "h", "i", "c", "f", "e", "d", "g", "k", "j");

QuerySnapshot getSnapshot = waitFor(orderedQuery.get());
List<String> getSnapshotDocIds =
Expand Down Expand Up @@ -1822,18 +1886,34 @@ public void snapshotListenerSortsUnicodeStringsInMapKeyAsServer() {
public void snapshotListenerSortsUnicodeStringsInDocumentKeyAsServer() {
Map<String, Map<String, Object>> testDocs =
map(
"Łukasiewicz", map("value", "foo"),
"Sierpiński", map("value", "foo"),
"岩澤", map("value", "foo"),
"🄟", map("value", "foo"),
"Ｐ", map("value", "foo"),
"︒", map("value", "foo"),
"🐵", map("value", "foo"));
"Łukasiewicz",
map("value", "foo"),
"Sierpiński",
map("value", "foo"),
"岩澤",
map("value", "foo"),
"🄟",
map("value", "foo"),
"Ｐ",
map("value", "foo"),
"︒",
map("value", "foo"),
"🐵",
map("value", "foo"),
"你好",
map("value", "foo"),
"你顥",
map("value", "foo"),
"😁",
map("value", "foo"),
"😀",
map("value", "foo"));

CollectionReference colRef = testCollectionWithDocs(testDocs);
Query orderedQuery = colRef.orderBy(FieldPath.documentId());
List<String> expectedDocIds =
Arrays.asList("Sierpiński", "Łukasiewicz", "岩澤", "︒", "Ｐ", "🄟", "🐵");
Arrays.asList(
"Sierpiński", "Łukasiewicz", "你好", "你顥", "岩澤", "︒", "Ｐ", "🄟", "🐵", "😀", "😁");

QuerySnapshot getSnapshot = waitFor(orderedQuery.get());
List<String> getSnapshotDocIds =
Expand Down
45 changes: 42 additions & 3 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@
import io.grpc.Status;
import io.grpc.StatusException;
import io.grpc.StatusRuntimeException;

import java.nio.charset.StandardCharsets;
import java.security.SecureRandom;
import java.sql.SQLOutput;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -87,9 +90,45 @@ public static int compareIntegers(int i1, int i2) {

/** Compare strings in UTF-8 encoded byte order */
public static int compareUtf8Strings(String left, String right) {
ByteString leftBytes = ByteString.copyFromUtf8(left);
ByteString rightBytes = ByteString.copyFromUtf8(right);
return compareByteStrings(leftBytes, rightBytes);
int i = 0;
while (i < left.length() && i < right.length()) {
int leftCodePoint = left.codePointAt(i);
int rightCodePoint = right.codePointAt(i);

if (leftCodePoint != rightCodePoint) {
if (leftCodePoint < 128 && rightCodePoint < 128) {
// ASCII comparison
return Integer.compare(leftCodePoint, rightCodePoint);
} else {
// substring and do UTF-8 encoded byte comparison
byte[] leftBytes = getUtf8SafeBytes(left, i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be wrong to always take (index, index+2), and compare those?

meaning, getting rid of getUtf8SafeBytes()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the unit test results, yes, somehow it is problematic for java, while js managed to handle it nicely. But, increasing i one by one + getUtf8SafeBytes() solved the problem.

byte[] rightBytes = getUtf8SafeBytes(right, i);
int comp = compareByteArrays(leftBytes,rightBytes);
if(comp !=0) {
return comp;
}
}
}

// Increment by 2 for surrogate pairs, 1 otherwise
i += Character.charCount(leftCodePoint);
}

// Compare lengths if all characters are equal
return Integer.compare(left.length(), right.length());
}

private static byte[] getUtf8SafeBytes(String str, int index) {
int firstCodePoint = str.codePointAt(index);
String sub;
if (firstCodePoint > 0xffff) {
// It's a surrogate pair, return the whole pair
sub = str.substring(index, index + 2);
} else {
// It's a single code point, return it
sub = str.substring(index, index + 1);
}
return sub.getBytes(StandardCharsets.UTF_8);
}

/**
Expand Down
Loading
Loading