Skip to content

Commit 52f36c3

Browse files
schmidt-sebastianschmidt-sebastian
committed
Review
1 parent f19cd92 commit 52f36c3

File tree

13 files changed

+162
-75
lines changed

13 files changed

+162
-75
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ public void writeMutations(List<Mutation> mutations, TaskCompletionSource<Void>
275275
assertCallback("writeMutations");
276276

277277
LocalDocumentsResult result = localStore.writeLocally(mutations);
278-
addUserCallback(result.getLargestBatchId(), userTask);
278+
addUserCallback(result.getBatchId(), userTask);
279279

280280
emitNewSnapsAndNotifyLocalStore(result.getDocuments(), /*remoteEvent=*/ null);
281281
remoteStore.fillWritePipeline();

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -158,14 +158,12 @@ private IndexOffset getNewOffset(IndexOffset existingOffset, LocalDocumentsResul
158158
return IndexOffset.create(
159159
maxOffset.getReadTime(),
160160
maxOffset.getDocumentKey(),
161-
Math.max(lookupResult.getLargestBatchId(), existingOffset.getLargestBatchId()));
161+
Math.max(lookupResult.getBatchId(), existingOffset.getLargestBatchId()));
162162
}
163163

164164
/** Returns the lowest offset for the provided index group. */
165165
private IndexOffset getExistingOffset(Collection<FieldIndex> fieldIndexes) {
166-
if (fieldIndexes.isEmpty()) {
167-
return IndexOffset.NONE;
168-
}
166+
hardAssert(!fieldIndexes.isEmpty(), "Updating collection without indexes");
169167

170168
Iterator<FieldIndex> it = fieldIndexes.iterator();
171169
IndexOffset minOffset = it.next().getIndexState().getOffset();

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsResult.java

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,23 @@
1818
import com.google.firebase.firestore.model.Document;
1919
import com.google.firebase.firestore.model.DocumentKey;
2020

21-
/** The result of a applying local mutations. */
21+
/**
22+
* Represents a set of document along with their mutation batch ID.
23+
*
24+
* <p>This class is used when applying mutations to the local store and to propagate document
25+
* updates to the indexing table.
26+
*/
2227
public final class LocalDocumentsResult {
23-
private final int largestBatchId;
28+
private final int batchId;
2429
private final ImmutableSortedMap<DocumentKey, Document> documents;
2530

26-
LocalDocumentsResult(int largestBatchId, ImmutableSortedMap<DocumentKey, Document> documents) {
27-
this.largestBatchId = largestBatchId;
31+
LocalDocumentsResult(int batchId, ImmutableSortedMap<DocumentKey, Document> documents) {
32+
this.batchId = batchId;
2833
this.documents = documents;
2934
}
3035

31-
public int getLargestBatchId() {
32-
return largestBatchId;
36+
public int getBatchId() {
37+
return batchId;
3338
}
3439

3540
public ImmutableSortedMap<DocumentKey, Document> getDocuments() {

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.firebase.firestore.core.Query;
2525
import com.google.firebase.firestore.model.Document;
2626
import com.google.firebase.firestore.model.DocumentKey;
27+
import com.google.firebase.firestore.model.FieldIndex;
2728
import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
2829
import com.google.firebase.firestore.model.MutableDocument;
2930
import com.google.firebase.firestore.model.ResourcePath;
@@ -86,7 +87,7 @@ DocumentOverlayCache getDocumentOverlayCache() {
8687
*/
8788
Document getDocument(DocumentKey key) {
8889
Overlay overlay = documentOverlayCache.getOverlay(key);
89-
MutableDocument document = createBaseDocument(key, overlay);
90+
MutableDocument document = getBaseDocument(key, overlay);
9091
if (overlay != null) {
9192
overlay.getMutation().applyToLocalView(document, null, Timestamp.now());
9293
}
@@ -265,7 +266,7 @@ private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollection
265266
* @param collectionGroup The collection group for the documents.
266267
* @param offset The offset to index into.
267268
* @param count The number of documents to return
268-
* @return A LocalDocumentsRResult with the documents that follow the provided offset and the last
269+
* @return A LocalDocumentsResult with the documents that follow the provided offset and the last
269270
* processed batch id.
270271
*/
271272
LocalDocumentsResult getNextDocuments(String collectionGroup, IndexOffset offset, int count) {
@@ -277,11 +278,15 @@ LocalDocumentsResult getNextDocuments(String collectionGroup, IndexOffset offset
277278
collectionGroup, offset.getLargestBatchId(), count - docs.size())
278279
: Collections.emptyMap();
279280

280-
int largestBatchId = -1;
281+
int largestBatchId = FieldIndex.INITIAL_LARGEST_BATCH_ID;
281282
for (Overlay overlay : overlays.values()) {
282283
if (!docs.containsKey(overlay.getKey())) {
283-
docs.put(overlay.getKey(), createBaseDocument(overlay.getKey(), overlay));
284+
docs.put(overlay.getKey(), getBaseDocument(overlay.getKey(), overlay));
284285
}
286+
// The callsite will use the largest batch ID together with the latest read time to create
287+
// a new index offset. Since we only process batch IDs if all remote documents have been read,
288+
// no overlay will increase the overall read time. This is why we only need to special case
289+
// the batch id.
285290
largestBatchId = Math.max(largestBatchId, overlay.getLargestBatchId());
286291
}
287292

@@ -322,7 +327,7 @@ private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollection
322327
}
323328

324329
/** Returns a base document that can be used to apply `overlay`. */
325-
private MutableDocument createBaseDocument(DocumentKey key, @Nullable Overlay overlay) {
330+
private MutableDocument getBaseDocument(DocumentKey key, @Nullable Overlay overlay) {
326331
return (overlay == null || overlay.getMutation() instanceof PatchMutation)
327332
? remoteDocumentCache.get(key)
328333
: MutableDocument.newInvalidDocument(key);

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,8 @@ && needsRefill(
162162
return appendRemainingResults(
163163
previousResults,
164164
query,
165-
IndexOffset.create(lastLimboFreeSnapshotVersion, FieldIndex.INITIAL_LARGEST_BATCH_ID));
165+
IndexOffset.createSuccessor(
166+
lastLimboFreeSnapshotVersion, FieldIndex.INITIAL_LARGEST_BATCH_ID));
166167
}
167168

168169
/** Applies the query filter and sorting to the provided documents. */

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ public static IndexOffset create(
141141
/**
142142
* Creates an offset that matches all documents with a read time higher than {@code readTime}.
143143
*/
144-
public static IndexOffset create(SnapshotVersion readTime, int largestBatchId) {
144+
public static IndexOffset createSuccessor(SnapshotVersion readTime, int largestBatchId) {
145145
// We want to create an offset that matches all documents with a read time greater than
146146
// the provided read time. To do so, we technically need to create an offset for
147147
// `(readTime, MAX_DOCUMENT_KEY)`. While we could use Unicode codepoints to generate

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java

Lines changed: 78 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import static junit.framework.TestCase.assertTrue;
3131

3232
import com.google.firebase.firestore.auth.User;
33+
import com.google.firebase.firestore.core.Query;
3334
import com.google.firebase.firestore.core.Target;
3435
import com.google.firebase.firestore.model.DocumentKey;
3536
import com.google.firebase.firestore.model.FieldIndex;
@@ -365,58 +366,38 @@ public void testBackfillMutationsFromHighWaterMark() {
365366

366367
@Test
367368
public void testBackfillUpdatesExistingDocToNewValue() {
368-
backfiller.setMaxDocumentsToProcess(1);
369-
addFieldIndex("coll1", "foo");
370-
FieldIndex fieldIndex = indexManager.getFieldIndexes("coll1").iterator().next();
369+
Query queryA = query("coll").filter(filter("foo", "==", 2));
370+
addFieldIndex("coll", "foo");
371+
372+
addDoc("coll/doc", version(10), "foo", 1);
371373

372-
addDoc("coll1/docA", version(10), "foo", 1);
373374
int documentsProcessed = backfiller.backfill();
374375
assertEquals(1, documentsProcessed);
375-
Target target = query("coll1").filter(filter("foo", "==", 2)).toTarget();
376-
Set<DocumentKey> matching = indexManager.getDocumentsMatchingTarget(fieldIndex, target);
377-
assertEquals(0, matching.size());
376+
verifyQueryResults(queryA);
378377

379378
// Update doc to new remote version with new value.
380-
addDoc("coll1/docA", version(40), "foo", 2);
379+
addDoc("coll/doc", version(40), "foo", 2);
381380
documentsProcessed = backfiller.backfill();
382-
assertEquals(1, documentsProcessed);
383-
matching = indexManager.getDocumentsMatchingTarget(fieldIndex, target);
384-
assertEquals(1, matching.size());
381+
382+
verifyQueryResults(queryA, "coll/doc");
385383
}
386384

387385
@Test
388386
public void testBackfillUpdatesDocsThatNoLongerMatch() {
389-
backfiller.setMaxDocumentsToProcess(1);
390-
addFieldIndex("coll1", "foo");
391-
FieldIndex fieldIndex = indexManager.getFieldIndexes("coll1").iterator().next();
387+
Query queryA = query("coll").filter(filter("foo", ">", 0));
388+
addFieldIndex("coll", "foo");
389+
addDoc("coll/doc", version(10), "foo", 1);
392390

393-
addDoc("coll1/docA", version(10), "foo", 1);
394391
int documentsProcessed = backfiller.backfill();
395392
assertEquals(1, documentsProcessed);
396-
Target target = query("coll1").filter(filter("foo", ">", 0)).toTarget();
397-
Set<DocumentKey> matching = indexManager.getDocumentsMatchingTarget(fieldIndex, target);
398-
assertEquals(1, matching.size());
393+
verifyQueryResults(queryA, "coll/doc");
399394

400395
// Update doc to new remote version with new value that doesn't match field index.
401-
addDoc("coll1/docA", version(40), "foo", -1);
402-
documentsProcessed = backfiller.backfill();
403-
assertEquals(1, documentsProcessed);
404-
matching = indexManager.getDocumentsMatchingTarget(fieldIndex, target);
405-
assertTrue(matching.isEmpty());
406-
}
407-
408-
@Test
409-
public void testBackfillUpdatesToLatestReadTimeInCollection() {
410-
// check that offset is set to rdc latest read time if no docs match current coll
411-
addFieldIndex("coll1", "foo");
412-
addDoc("coll1/doc", version(5), "foo", 1);
396+
addDoc("coll/doc", version(40), "foo", -1);
413397

414-
int documentsProcessed = backfiller.backfill();
398+
documentsProcessed = backfiller.backfill();
415399
assertEquals(1, documentsProcessed);
416-
417-
// Offset should be set to latest read time in the cache if no documents were indexed.
418-
Iterator<FieldIndex> it = indexManager.getFieldIndexes("coll1").iterator();
419-
assertEquals(version(5), it.next().getIndexState().getOffset().getReadTime());
400+
verifyQueryResults(queryA);
420401
}
421402

422403
@Test
@@ -434,7 +415,7 @@ public void testBackfillDoesNotProcessSameDocumentTwice() {
434415
}
435416

436417
@Test
437-
public void testBackfillAppliesOverlayToRemoteDoc() {
418+
public void testBackfillAppliesSetToRemoteDoc() {
438419
addFieldIndex("coll", "foo");
439420
addDoc("coll/doc", version(5), "boo", 1);
440421

@@ -446,14 +427,35 @@ public void testBackfillAppliesOverlayToRemoteDoc() {
446427
documentsProcessed = backfiller.backfill();
447428
assertEquals(1, documentsProcessed);
448429

449-
FieldIndex fieldIndex = indexManager.getFieldIndexes("coll").iterator().next();
450-
Target target = query("coll").filter(filter("foo", "==", 1)).toTarget();
451-
Set<DocumentKey> matching = indexManager.getDocumentsMatchingTarget(fieldIndex, target);
452-
assertEquals(1, matching.size());
430+
verifyQueryResults("coll", "coll/doc");
453431
}
454432

455433
@Test
456-
public void testBackfillAppliesDeleteMutationOnRemoteDoc() {
434+
public void testBackfillAppliesPatchToRemoteDoc() {
435+
Query queryA = query("coll").orderBy(orderBy("a"));
436+
Query queryB = query("coll").orderBy(orderBy("b"));
437+
438+
addFieldIndex("coll", "a");
439+
addFieldIndex("coll", "b");
440+
addDoc("coll/doc", version(5), "a", 1);
441+
442+
int documentsProcessed = backfiller.backfill();
443+
assertEquals(1, documentsProcessed);
444+
445+
verifyQueryResults(queryA, "coll/doc");
446+
verifyQueryResults(queryB);
447+
448+
Mutation patch = patchMutation("coll/doc", map("b", 1));
449+
addMutationToOverlay("coll/doc", patch);
450+
documentsProcessed = backfiller.backfill();
451+
assertEquals(1, documentsProcessed);
452+
453+
verifyQueryResults(queryA, "coll/doc");
454+
verifyQueryResults(queryB, "coll/doc");
455+
}
456+
457+
@Test
458+
public void testBackfillAppliesDeleteToRemoteDoc() {
457459
addFieldIndex("coll", "foo");
458460
addDoc("coll/doc", version(5), "foo", 1);
459461

@@ -471,6 +473,28 @@ public void testBackfillAppliesDeleteMutationOnRemoteDoc() {
471473
assertTrue(matching.isEmpty());
472474
}
473475

476+
@Test
477+
public void testReindexesDocumentsWhenNewIndexIsAdded() {
478+
Query queryA = query("coll").orderBy(orderBy("a"));
479+
Query queryB = query("coll").orderBy(orderBy("b"));
480+
481+
addFieldIndex("coll", "a");
482+
addDoc("coll/doc1", version(1), "a", 1);
483+
addDoc("coll/doc2", version(1), "b", 1);
484+
485+
int documentsProcessed = backfiller.backfill();
486+
assertEquals(2, documentsProcessed);
487+
verifyQueryResults(queryA, "coll/doc1");
488+
verifyQueryResults(queryB);
489+
490+
addFieldIndex("coll", "b");
491+
documentsProcessed = backfiller.backfill();
492+
assertEquals(2, documentsProcessed);
493+
494+
verifyQueryResults(queryA, "coll/doc1");
495+
verifyQueryResults(queryB, "coll/doc2");
496+
}
497+
474498
private void addFieldIndex(String collectionGroup, String fieldName) {
475499
FieldIndex fieldIndex =
476500
fieldIndex(collectionGroup, fieldName, FieldIndex.Segment.Kind.ASCENDING);
@@ -499,12 +523,20 @@ private void addFieldIndex(String collectionGroup, String fieldName, long sequen
499523
indexManager.addFieldIndex(fieldIndex);
500524
}
501525

502-
private void verifyQueryResults(String collectionGroup, String... expectedKeys) {
503-
Target target = query(collectionGroup).orderBy(orderBy("foo")).toTarget();
526+
private void verifyQueryResults(Query query, String... expectedKeys) {
527+
Target target = query.toTarget();
504528
FieldIndex persistedIndex = indexManager.getFieldIndex(target);
505-
Set<DocumentKey> actualKeys = indexManager.getDocumentsMatchingTarget(persistedIndex, target);
506-
assertThat(actualKeys)
507-
.containsExactlyElementsIn(Arrays.stream(expectedKeys).map(TestUtil::key).toArray());
529+
if (persistedIndex != null) {
530+
Set<DocumentKey> actualKeys = indexManager.getDocumentsMatchingTarget(persistedIndex, target);
531+
assertThat(actualKeys)
532+
.containsExactlyElementsIn(Arrays.stream(expectedKeys).map(TestUtil::key).toArray());
533+
} else {
534+
assertEquals(0, expectedKeys.length);
535+
}
536+
}
537+
538+
private void verifyQueryResults(String collectionGroup, String... expectedKeys) {
539+
verifyQueryResults(query(collectionGroup).orderBy(orderBy("foo")), expectedKeys);
508540
}
509541

510542
/** Creates a document and adds it to the RemoteDocumentCache. */

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexingEnabledSQLiteLocalStoreTest.java

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@
2020
import static com.google.firebase.firestore.testutil.TestUtil.doc;
2121
import static com.google.firebase.firestore.testutil.TestUtil.fieldIndex;
2222
import static com.google.firebase.firestore.testutil.TestUtil.filter;
23+
import static com.google.firebase.firestore.testutil.TestUtil.key;
2324
import static com.google.firebase.firestore.testutil.TestUtil.map;
2425
import static com.google.firebase.firestore.testutil.TestUtil.query;
2526
import static com.google.firebase.firestore.testutil.TestUtil.setMutation;
2627
import static com.google.firebase.firestore.testutil.TestUtil.updateRemoteEvent;
28+
import static com.google.firebase.firestore.testutil.TestUtil.version;
2729
import static java.util.Collections.emptyList;
2830
import static java.util.Collections.singletonList;
2931

@@ -55,7 +57,7 @@ public static void afterClass() {
5557
}
5658

5759
@Test
58-
public void testConfiguresIndexes() {
60+
public void testAddsIndexes() {
5961
FieldIndex indexA = fieldIndex("coll", 0, FieldIndex.INITIAL_STATE, "a", Kind.ASCENDING);
6062
FieldIndex indexB = fieldIndex("coll", 1, FieldIndex.INITIAL_STATE, "b", Kind.DESCENDING);
6163
FieldIndex indexC =
@@ -70,6 +72,50 @@ public void testConfiguresIndexes() {
7072
assertThat(fieldIndexes).containsExactly(indexA, indexC);
7173
}
7274

75+
@Test
76+
public void testRemovesIndexes() {
77+
FieldIndex indexA = fieldIndex("coll", 0, FieldIndex.INITIAL_STATE, "a", Kind.ASCENDING);
78+
FieldIndex indexB = fieldIndex("coll", 1, FieldIndex.INITIAL_STATE, "b", Kind.DESCENDING);
79+
80+
configureFieldIndexes(Arrays.asList(indexA, indexB));
81+
Collection<FieldIndex> fieldIndexes = getFieldIndexes();
82+
assertThat(fieldIndexes).containsExactly(indexA, indexB);
83+
84+
configureFieldIndexes(singletonList(indexA));
85+
fieldIndexes = getFieldIndexes();
86+
assertThat(fieldIndexes).containsExactly(indexA);
87+
}
88+
89+
@Test
90+
public void testDoesNotResetIndexWhenSameIndexIsAdded() {
91+
FieldIndex indexA = fieldIndex("coll", 0, FieldIndex.INITIAL_STATE, "a", Kind.ASCENDING);
92+
93+
configureFieldIndexes(singletonList(indexA));
94+
Collection<FieldIndex> fieldIndexes = getFieldIndexes();
95+
assertThat(fieldIndexes).containsExactly(indexA);
96+
97+
Query query = query("coll").filter(filter("a", "==", 1));
98+
int targetId = allocateQuery(query);
99+
applyRemoteEvent(addedRemoteEvent(doc("coll/a", 10, map("a", 1)), targetId));
100+
101+
backfillIndexes();
102+
FieldIndex updatedIndexA =
103+
fieldIndex(
104+
"coll",
105+
0,
106+
FieldIndex.IndexState.create(1, version(10), key("coll/a"), -1),
107+
"a",
108+
Kind.ASCENDING);
109+
110+
fieldIndexes = getFieldIndexes();
111+
assertThat(fieldIndexes).containsExactly(updatedIndexA);
112+
113+
// Re-add the same index. We do not reset the index to its initial state.
114+
configureFieldIndexes(singletonList(indexA));
115+
fieldIndexes = getFieldIndexes();
116+
assertThat(fieldIndexes).containsExactly(updatedIndexA);
117+
}
118+
73119
@Test
74120
public void testUsesIndexes() {
75121
FieldIndex index = fieldIndex("coll", 0, FieldIndex.INITIAL_STATE, "matches", Kind.ASCENDING);

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ private void writeMutations(List<Mutation> mutations) {
144144
LocalDocumentsResult result = localStore.writeLocally(mutations);
145145
batches.add(
146146
new MutationBatch(
147-
result.getLargestBatchId(), Timestamp.now(), Collections.emptyList(), mutations));
147+
result.getBatchId(), Timestamp.now(), Collections.emptyList(), mutations));
148148
lastChanges = result.getDocuments();
149149
}
150150

0 commit comments

Comments
 (0)