Skip to content

Commit 3418ef8

Browse files
milaGGLmilaGGL
authored
Revert the UTF-8 encoding in string sorting (#8782)
1 parent 4d2fc6e commit 3418ef8

File tree

6 files changed

+17
-226
lines changed

6 files changed

+17
-226
lines changed

‎.changeset/slimy-chicken-mix.md

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@firebase/firestore': patch
3+
'firebase': patch
4+
---
5+
6+
Reverted a change to use UTF-8 encoding in string comparisons which caused a performance issue. See [GitHub issue #8778](https://github.com/firebase/firebase-js-sdk/issues/8778)

‎packages/firestore/src/local/indexeddb_remote_document_cache.ts

-4
Original file line numberDiff line numberDiff line change
@@ -655,9 +655,5 @@ export function dbKeyComparator(l: DocumentKey, r: DocumentKey): number {
655655
return cmp;
656656
}
657657

658-
// TODO(b/329441702): Document IDs should be sorted by UTF-8 encoded byte
659-
// order, but IndexedDB sorts strings lexicographically. Document ID
660-
// comparison here still relies on primitive comparison to avoid mismatches
661-
// observed in snapshot listeners with Unicode characters in documentIds
662658
return primitiveComparator(left[left.length - 1], right[right.length - 1]);
663659
}

‎packages/firestore/src/model/path.ts

+8-3
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import { Integer } from '@firebase/webchannel-wrapper/bloom-blob';
1919

2020
import { debugAssert, fail } from '../util/assert';
2121
import { Code, FirestoreError } from '../util/error';
22-
import { primitiveComparator, compareUtf8Strings } from '../util/misc';
2322

2423
export const DOCUMENT_KEY_NAME = '__name__';
2524

@@ -182,7 +181,7 @@ abstract class BasePath<B extends BasePath<B>> {
182181
return comparison;
183182
}
184183
}
185-
return primitiveComparator(p1.length, p2.length);
184+
return Math.sign(p1.length - p2.length);
186185
}
187186

188187
private static compareSegments(lhs: string, rhs: string): number {
@@ -202,7 +201,13 @@ abstract class BasePath<B extends BasePath<B>> {
202201
);
203202
} else {
204203
// both non-numeric
205-
return compareUtf8Strings(lhs, rhs);
204+
if (lhs < rhs) {
205+
return -1;
206+
}
207+
if (lhs > rhs) {
208+
return 1;
209+
}
210+
return 0;
206211
}
207212
}
208213

‎packages/firestore/src/model/values.ts

+3-7
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,7 @@ import {
2525
Value
2626
} from '../protos/firestore_proto_api';
2727
import { fail } from '../util/assert';
28-
import {
29-
arrayEquals,
30-
primitiveComparator,
31-
compareUtf8Strings
32-
} from '../util/misc';
28+
import { arrayEquals, primitiveComparator } from '../util/misc';
3329
import { forEach, objectSize } from '../util/obj';
3430
import { isNegativeZero } from '../util/types';
3531

@@ -255,7 +251,7 @@ export function valueCompare(left: Value, right: Value): number {
255251
getLocalWriteTime(right)
256252
);
257253
case TypeOrder.StringValue:
258-
return compareUtf8Strings(left.stringValue!, right.stringValue!);
254+
return primitiveComparator(left.stringValue!, right.stringValue!);
259255
case TypeOrder.BlobValue:
260256
return compareBlobs(left.bytesValue!, right.bytesValue!);
261257
case TypeOrder.RefValue:
@@ -404,7 +400,7 @@ function compareMaps(left: MapValue, right: MapValue): number {
404400
rightKeys.sort();
405401

406402
for (let i = 0; i < leftKeys.length && i < rightKeys.length; ++i) {
407-
const keyCompare = compareUtf8Strings(leftKeys[i], rightKeys[i]);
403+
const keyCompare = primitiveComparator(leftKeys[i], rightKeys[i]);
408404
if (keyCompare !== 0) {
409405
return keyCompare;
410406
}

‎packages/firestore/src/util/misc.ts

-17
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
*/
1717

1818
import { randomBytes } from '../platform/random_bytes';
19-
import { newTextEncoder } from '../platform/text_serializer';
2019

2120
import { debugAssert } from './assert';
2221

@@ -75,22 +74,6 @@ export interface Equatable<T> {
7574
isEqual(other: T): boolean;
7675
}
7776

78-
/** Compare strings in UTF-8 encoded byte order */
79-
export function compareUtf8Strings(left: string, right: string): number {
80-
// Convert the string to UTF-8 encoded bytes
81-
const encodedLeft = newTextEncoder().encode(left);
82-
const encodedRight = newTextEncoder().encode(right);
83-
84-
for (let i = 0; i < Math.min(encodedLeft.length, encodedRight.length); i++) {
85-
const comparison = primitiveComparator(encodedLeft[i], encodedRight[i]);
86-
if (comparison !== 0) {
87-
return comparison;
88-
}
89-
}
90-
91-
return primitiveComparator(encodedLeft.length, encodedRight.length);
92-
}
93-
9477
export interface Iterable<V> {
9578
forEach: (cb: (v: V) => void) => void;
9679
}

‎packages/firestore/test/integration/api/database.test.ts

-195
Original file line numberDiff line numberDiff line change
@@ -2424,199 +2424,4 @@ apiDescribe('Database', persistence => {
24242424
});
24252425
});
24262426
});
2427-
2428-
describe('Sort unicode strings', () => {
2429-
const expectedDocs = ['b', 'a', 'c', 'f', 'e', 'd', 'g'];
2430-
it('snapshot listener sorts unicode strings the same as server', async () => {
2431-
const testDocs = {
2432-
'a': { value: 'Łukasiewicz' },
2433-
'b': { value: 'Sierpiński' },
2434-
'c': { value: '岩澤' },
2435-
'd': { value: '🄟' },
2436-
'e': { value: 'Ｐ' },
2437-
'f': { value: '︒' },
2438-
'g': { value: '🐵' }
2439-
};
2440-
2441-
return withTestCollection(persistence, testDocs, async collectionRef => {
2442-
const orderedQuery = query(collectionRef, orderBy('value'));
2443-
2444-
const getSnapshot = await getDocsFromServer(orderedQuery);
2445-
expect(toIds(getSnapshot)).to.deep.equal(expectedDocs);
2446-
2447-
const storeEvent = new EventsAccumulator<QuerySnapshot>();
2448-
const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent);
2449-
const watchSnapshot = await storeEvent.awaitEvent();
2450-
expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot));
2451-
2452-
unsubscribe();
2453-
2454-
await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs);
2455-
});
2456-
});
2457-
2458-
it('snapshot listener sorts unicode strings in array the same as server', async () => {
2459-
const testDocs = {
2460-
'a': { value: ['Łukasiewicz'] },
2461-
'b': { value: ['Sierpiński'] },
2462-
'c': { value: ['岩澤'] },
2463-
'd': { value: ['🄟'] },
2464-
'e': { value: ['Ｐ'] },
2465-
'f': { value: ['︒'] },
2466-
'g': { value: ['🐵'] }
2467-
};
2468-
2469-
return withTestCollection(persistence, testDocs, async collectionRef => {
2470-
const orderedQuery = query(collectionRef, orderBy('value'));
2471-
2472-
const getSnapshot = await getDocsFromServer(orderedQuery);
2473-
expect(toIds(getSnapshot)).to.deep.equal(expectedDocs);
2474-
2475-
const storeEvent = new EventsAccumulator<QuerySnapshot>();
2476-
const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent);
2477-
const watchSnapshot = await storeEvent.awaitEvent();
2478-
expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot));
2479-
2480-
unsubscribe();
2481-
2482-
await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs);
2483-
});
2484-
});
2485-
2486-
it('snapshot listener sorts unicode strings in map the same as server', async () => {
2487-
const testDocs = {
2488-
'a': { value: { foo: 'Łukasiewicz' } },
2489-
'b': { value: { foo: 'Sierpiński' } },
2490-
'c': { value: { foo: '岩澤' } },
2491-
'd': { value: { foo: '🄟' } },
2492-
'e': { value: { foo: 'Ｐ' } },
2493-
'f': { value: { foo: '︒' } },
2494-
'g': { value: { foo: '🐵' } }
2495-
};
2496-
2497-
return withTestCollection(persistence, testDocs, async collectionRef => {
2498-
const orderedQuery = query(collectionRef, orderBy('value'));
2499-
2500-
const getSnapshot = await getDocsFromServer(orderedQuery);
2501-
expect(toIds(getSnapshot)).to.deep.equal(expectedDocs);
2502-
2503-
const storeEvent = new EventsAccumulator<QuerySnapshot>();
2504-
const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent);
2505-
const watchSnapshot = await storeEvent.awaitEvent();
2506-
expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot));
2507-
2508-
unsubscribe();
2509-
2510-
await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs);
2511-
});
2512-
});
2513-
2514-
it('snapshot listener sorts unicode strings in map key the same as server', async () => {
2515-
const testDocs = {
2516-
'a': { value: { 'Łukasiewicz': true } },
2517-
'b': { value: { 'Sierpiński': true } },
2518-
'c': { value: { '岩澤': true } },
2519-
'd': { value: { '🄟': true } },
2520-
'e': { value: { 'Ｐ': true } },
2521-
'f': { value: { '︒': true } },
2522-
'g': { value: { '🐵': true } }
2523-
};
2524-
2525-
return withTestCollection(persistence, testDocs, async collectionRef => {
2526-
const orderedQuery = query(collectionRef, orderBy('value'));
2527-
2528-
const getSnapshot = await getDocsFromServer(orderedQuery);
2529-
expect(toIds(getSnapshot)).to.deep.equal(expectedDocs);
2530-
2531-
const storeEvent = new EventsAccumulator<QuerySnapshot>();
2532-
const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent);
2533-
const watchSnapshot = await storeEvent.awaitEvent();
2534-
expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot));
2535-
2536-
unsubscribe();
2537-
2538-
await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs);
2539-
});
2540-
});
2541-
2542-
it('snapshot listener sorts unicode strings in document key the same as server', async () => {
2543-
const testDocs = {
2544-
'Łukasiewicz': { value: true },
2545-
'Sierpiński': { value: true },
2546-
'岩澤': { value: true },
2547-
'🄟': { value: true },
2548-
'Ｐ': { value: true },
2549-
'︒': { value: true },
2550-
'🐵': { value: true }
2551-
};
2552-
2553-
return withTestCollection(persistence, testDocs, async collectionRef => {
2554-
const orderedQuery = query(collectionRef, orderBy(documentId()));
2555-
2556-
const getSnapshot = await getDocsFromServer(orderedQuery);
2557-
const expectedDocs = [
2558-
'Sierpiński',
2559-
'Łukasiewicz',
2560-
'岩澤',
2561-
'︒',
2562-
'Ｐ',
2563-
'🄟',
2564-
'🐵'
2565-
];
2566-
expect(toIds(getSnapshot)).to.deep.equal(expectedDocs);
2567-
2568-
const storeEvent = new EventsAccumulator<QuerySnapshot>();
2569-
const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent);
2570-
const watchSnapshot = await storeEvent.awaitEvent();
2571-
expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot));
2572-
2573-
unsubscribe();
2574-
2575-
await checkOnlineAndOfflineResultsMatch(orderedQuery, ...expectedDocs);
2576-
});
2577-
});
2578-
2579-
// eslint-disable-next-line no-restricted-properties
2580-
(persistence.storage === 'indexeddb' ? it.skip : it)(
2581-
'snapshot listener sorts unicode strings in document key the same as server with persistence',
2582-
async () => {
2583-
const testDocs = {
2584-
'Łukasiewicz': { value: true },
2585-
'Sierpiński': { value: true },
2586-
'岩澤': { value: true },
2587-
'🄟': { value: true },
2588-
'Ｐ': { value: true },
2589-
'︒': { value: true },
2590-
'🐵': { value: true }
2591-
};
2592-
2593-
return withTestCollection(
2594-
persistence,
2595-
testDocs,
2596-
async collectionRef => {
2597-
const orderedQuery = query(collectionRef, orderBy('value'));
2598-
2599-
const getSnapshot = await getDocsFromServer(orderedQuery);
2600-
expect(toIds(getSnapshot)).to.deep.equal([
2601-
'Sierpiński',
2602-
'Łukasiewicz',
2603-
'岩澤',
2604-
'︒',
2605-
'Ｐ',
2606-
'🄟',
2607-
'🐵'
2608-
]);
2609-
2610-
const storeEvent = new EventsAccumulator<QuerySnapshot>();
2611-
const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent);
2612-
const watchSnapshot = await storeEvent.awaitEvent();
2613-
// TODO: IndexedDB sorts string lexicographically, and misses the document with ID '🄟','🐵'
2614-
expect(toIds(watchSnapshot)).to.deep.equal(toIds(getSnapshot));
2615-
2616-
unsubscribe();
2617-
}
2618-
);
2619-
}
2620-
);
2621-
});
26222427
});

0 commit comments

Comments
 (0)