Skip to content

Commit 712e5b8

Browse files
wu-huiwu-hui
authored
Remote document reads elision (#3246)
1 parent 1b8e6dd commit 712e5b8

File tree

6 files changed

+68
-23
lines changed

6 files changed

+68
-23
lines changed

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,16 @@ IndexManager getIndexManager() {
8989
*/
9090
Document getDocument(DocumentKey key) {
9191
Mutation overlay = documentOverlayCache.getOverlay(key);
92-
MutableDocument fromOverlay = remoteDocumentCache.get(key);
92+
// Only read from remote document cache if overlay is a patch.
93+
MutableDocument document =
94+
(overlay == null || overlay instanceof PatchMutation)
95+
? remoteDocumentCache.get(key)
96+
: MutableDocument.newInvalidDocument(key);
9397
if (overlay != null) {
94-
overlay.applyToLocalView(fromOverlay, null, Timestamp.now());
98+
overlay.applyToLocalView(document, null, Timestamp.now());
9599
}
96100

97-
return fromOverlay;
101+
return document;
98102
}
99103

100104
/**

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ public MutableDocument setHasCommittedMutations() {
159159

160160
public MutableDocument setHasLocalMutations() {
161161
this.documentState = DocumentState.HAS_LOCAL_MUTATIONS;
162+
this.version = SnapshotVersion.NONE;
162163
return this;
163164
}
164165

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

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,9 @@ public void testHandlesSetMutationThenDocument() {
389389
updateRemoteEvent(
390390
doc("foo/bar", 2, map("it", "changed")).setHasLocalMutations(),
391391
asList(targetId),
392-
emptyList()));
392+
emptyList(),
393+
asList(targetId),
394+
version(2)));
393395
assertChanged(doc("foo/bar", 2, map("foo", "bar")).setHasLocalMutations());
394396
assertContains(doc("foo/bar", 2, map("foo", "bar")).setHasLocalMutations());
395397
}
@@ -548,9 +550,10 @@ public void testHandlesPatchMutationThenDocumentThenAck() {
548550
int targetId = allocateQuery(query);
549551
applyRemoteEvent(
550552
addedRemoteEvent(
551-
doc("foo/bar", 1, map("it", "base")).setHasLocalMutations(),
553+
asList(doc("foo/bar", 1, map("it", "base")).setHasLocalMutations()),
552554
asList(targetId),
553-
emptyList()));
555+
emptyList(),
556+
version(1)));
554557
assertChanged(doc("foo/bar", 1, map("foo", "bar", "it", "base")).setHasLocalMutations());
555558
assertContains(doc("foo/bar", 1, map("foo", "bar", "it", "base")).setHasLocalMutations());
556559

@@ -692,7 +695,9 @@ public void testHandlesSetMutationThenPatchMutationThenDocumentThenAckThenAck()
692695
updateRemoteEvent(
693696
doc("foo/bar", 1, map("it", "base")).setHasLocalMutations(),
694697
asList(targetId),
695-
emptyList()));
698+
emptyList(),
699+
asList(targetId),
700+
version(1)));
696701
assertChanged(doc("foo/bar", 1, map("foo", "bar")).setHasLocalMutations());
697702
assertContains(doc("foo/bar", 1, map("foo", "bar")).setHasLocalMutations());
698703

@@ -1114,7 +1119,8 @@ public void testUsesTargetMappingToExecuteQueries() {
11141119
addedRemoteEvent(
11151120
asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 10, map("matches", true))),
11161121
asList(targetId),
1117-
emptyList()));
1122+
emptyList(),
1123+
version(10)));
11181124
applyRemoteEvent(noChangeEvent(targetId, 10));
11191125
updateViews(targetId, /* fromCache= */ false);
11201126

@@ -1137,7 +1143,10 @@ public void testIgnoresTargetMappingAfterExistenceFilterMismatch() {
11371143
// Persist a mapping with a single document
11381144
applyRemoteEvent(
11391145
addedRemoteEvent(
1140-
asList(doc("foo/a", 10, map("matches", true))), asList(targetId), emptyList()));
1146+
asList(doc("foo/a", 10, map("matches", true))),
1147+
asList(targetId),
1148+
emptyList(),
1149+
version(10)));
11411150
applyRemoteEvent(noChangeEvent(targetId, 10));
11421151
updateViews(targetId, /* fromCache= */ false);
11431152

@@ -1240,7 +1249,8 @@ public void testQueriesIncludeDocumentsFromOtherQueries() {
12401249
addedRemoteEvent(
12411250
asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 20, map("matches", true))),
12421251
asList(targetId),
1243-
emptyList()));
1252+
emptyList(),
1253+
version(20)));
12441254
releaseTarget(targetId);
12451255

12461256
// Run the original query again and ensure that both the original matches as well as all new
@@ -1265,7 +1275,8 @@ public void testQueriesFilterDocumentsThatNoLongerMatch() {
12651275
addedRemoteEvent(
12661276
asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 10, map("matches", true))),
12671277
asList(targetId),
1268-
emptyList()));
1278+
emptyList(),
1279+
version(10)));
12691280
applyRemoteEvent(noChangeEvent(targetId, 10));
12701281
updateViews(targetId, /* fromCache=*/ false);
12711282
releaseTarget(targetId);
@@ -1277,7 +1288,8 @@ public void testQueriesFilterDocumentsThatNoLongerMatch() {
12771288
addedRemoteEvent(
12781289
asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 20, map("matches", false))),
12791290
asList(targetId),
1280-
emptyList()));
1291+
emptyList(),
1292+
version(20)));
12811293
releaseTarget(targetId);
12821294

12831295
// Re-run the filtered query and verify that the modified document is no longer returned.
@@ -1601,7 +1613,8 @@ public void testOnlyPersistsUpdatesForDocumentsWhenVersionChanges() {
16011613
addedRemoteEvent(
16021614
asList(doc("foo/bar", 1, map("val", "new")), doc("foo/baz", 2, map("val", "new"))),
16031615
asList(2),
1604-
emptyList()));
1616+
emptyList(),
1617+
version(2)));
16051618

16061619
assertChanged(doc("foo/baz", 2, map("val", "new")));
16071620
// The update for foo/bar is ignored.

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

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,20 @@ private void addDocument(MutableDocument... docs) {
146146
});
147147
}
148148

149+
/**
150+
* Adds the provided documents to the remote document cache in a event of the given snapshot
151+
* version.
152+
*/
153+
private void addDocumentWithEventVersion(SnapshotVersion eventVersion, MutableDocument... docs) {
154+
persistence.runTransaction(
155+
"addDocument",
156+
() -> {
157+
for (MutableDocument doc : docs) {
158+
remoteDocumentCache.add(doc, eventVersion);
159+
}
160+
});
161+
}
162+
149163
/** Adds a mutation to the mutation queue. */
150164
private void addMutation(Mutation mutation) {
151165
persistence.runTransaction(
@@ -213,7 +227,7 @@ public void filtersNonMatchingInitialResults() throws Exception {
213227
persistQueryMapping(MATCHING_DOC_A.getKey(), MATCHING_DOC_B.getKey());
214228

215229
// Add a mutated document that is not yet part of query's set of remote keys.
216-
addDocument(PENDING_NON_MATCHING_DOC_A);
230+
addDocumentWithEventVersion(version(1), PENDING_NON_MATCHING_DOC_A);
217231

218232
DocumentSet docs =
219233
expectOptimizedCollectionScan(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
@@ -297,7 +311,7 @@ public void doesNotUseInitialResultsForLimitQueryWhenLastDocumentHasPendingWrite
297311

298312
// Add a query mapping for a document that matches, but that sorts below another document due to
299313
// a pending write.
300-
addDocument(PENDING_MATCHING_DOC_A);
314+
addDocumentWithEventVersion(version(1), PENDING_MATCHING_DOC_A);
301315
persistQueryMapping(PENDING_MATCHING_DOC_A.getKey());
302316

303317
addDocument(MATCHING_DOC_B);
@@ -317,7 +331,7 @@ public void doesNotUseInitialResultsForLimitToLastQueryWhenLastDocumentHasPendin
317331

318332
// Add a query mapping for a document that matches, but that sorts below another document due to
319333
// a pending write.
320-
addDocument(PENDING_MATCHING_DOC_A);
334+
addDocumentWithEventVersion(version(1), PENDING_MATCHING_DOC_A);
321335
persistQueryMapping(PENDING_MATCHING_DOC_A.getKey());
322336

323337
addDocument(MATCHING_DOC_B);
@@ -375,7 +389,8 @@ public void limitQueriesUseInitialResultsIfLastDocumentInLimitIsUnchanged() thro
375389
persistQueryMapping(key("coll/a"), key("coll/b"));
376390

377391
// Update "coll/a" but make sure it still sorts before "coll/b"
378-
addDocument(doc("coll/a", 1, map("order", 2)).setHasLocalMutations());
392+
addDocumentWithEventVersion(
393+
version(1), doc("coll/a", 1, map("order", 2)).setHasLocalMutations());
379394

380395
// Since the last document in the limit didn't change (and hence we know that all documents
381396
// written prior to query execution still sort after "coll/b"), we should use an Index-Free

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ public void testCreateOverlayFromSet() {
9999
assertEquals(
100100
setMutation("foo/bar", map("foo", "bar")),
101101
persistence.getDocumentOverlay(User.UNAUTHENTICATED).getOverlay(key("foo/bar")));
102+
// Version is 0 because of remote document elision.
102103
assertContains(doc("foo/bar", 2, map("foo", "bar")).setHasLocalMutations());
103104

104105
SQLiteOverlayMigrationManager migrationManager =
@@ -122,6 +123,7 @@ public void testCreateOverlayFromDelete() {
122123
assertEquals(
123124
deleteMutation("foo/bar"),
124125
persistence.getDocumentOverlay(User.UNAUTHENTICATED).getOverlay(key("foo/bar")));
126+
// Version is 0 because of remote document elision.
125127
assertContains(deletedDoc("foo/bar", 2).setHasLocalMutations());
126128

127129
SQLiteOverlayMigrationManager migrationManager =

‎firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -424,7 +424,7 @@ public static RemoteEvent noChangeEvent(int targetId, int version, ByteString re
424424

425425
public static RemoteEvent addedRemoteEvent(
426426
MutableDocument doc, List<Integer> updatedInTargets, List<Integer> removedFromTargets) {
427-
return addedRemoteEvent(singletonList(doc), updatedInTargets, removedFromTargets);
427+
return addedRemoteEvent(singletonList(doc), updatedInTargets, removedFromTargets, null);
428428
}
429429

430430
public static RemoteEvent existenceFilterEvent(int targetId, int count, int version) {
@@ -444,7 +444,8 @@ public static RemoteEvent existenceFilterEvent(int targetId, int count, int vers
444444
public static RemoteEvent addedRemoteEvent(
445445
List<MutableDocument> docs,
446446
List<Integer> updatedInTargets,
447-
List<Integer> removedFromTargets) {
447+
List<Integer> removedFromTargets,
448+
@Nullable SnapshotVersion eventVersion) {
448449
Preconditions.checkArgument(!docs.isEmpty(), "Cannot pass empty docs array");
449450

450451
WatchChangeAggregator aggregator =
@@ -471,26 +472,35 @@ public TargetData getTargetDataForTarget(int targetId) {
471472
version = doc.getVersion().compareTo(version) > 0 ? doc.getVersion() : version;
472473
}
473474

474-
return aggregator.createRemoteEvent(version);
475+
return aggregator.createRemoteEvent(eventVersion == null ? version : eventVersion);
475476
}
476477

477478
public static RemoteEvent addedRemoteEvent(MutableDocument doc, Integer targetId) {
478-
return addedRemoteEvent(singletonList(doc), singletonList(targetId), emptyList());
479+
return addedRemoteEvent(singletonList(doc), singletonList(targetId), emptyList(), null);
479480
}
480481

481482
public static RemoteEvent updateRemoteEvent(
482483
MutableDocument doc, List<Integer> updatedInTargets, List<Integer> removedFromTargets) {
483484
List<Integer> activeTargets = new ArrayList<>();
484485
activeTargets.addAll(updatedInTargets);
485486
activeTargets.addAll(removedFromTargets);
486-
return updateRemoteEvent(doc, updatedInTargets, removedFromTargets, activeTargets);
487+
return updateRemoteEvent(doc, updatedInTargets, removedFromTargets, activeTargets, null);
487488
}
488489

489490
public static RemoteEvent updateRemoteEvent(
490491
MutableDocument doc,
491492
List<Integer> updatedInTargets,
492493
List<Integer> removedFromTargets,
493494
List<Integer> activeTargets) {
495+
return updateRemoteEvent(doc, updatedInTargets, removedFromTargets, activeTargets, null);
496+
}
497+
498+
public static RemoteEvent updateRemoteEvent(
499+
MutableDocument doc,
500+
List<Integer> updatedInTargets,
501+
List<Integer> removedFromTargets,
502+
List<Integer> activeTargets,
503+
@Nullable SnapshotVersion eventVersion) {
494504
DocumentChange change =
495505
new DocumentChange(updatedInTargets, removedFromTargets, doc.getKey(), doc);
496506
WatchChangeAggregator aggregator =
@@ -509,7 +519,7 @@ public TargetData getTargetDataForTarget(int targetId) {
509519
}
510520
});
511521
aggregator.handleDocumentChange(change);
512-
return aggregator.createRemoteEvent(doc.getVersion());
522+
return aggregator.createRemoteEvent(eventVersion == null ? doc.getVersion() : eventVersion);
513523
}
514524

515525
public static SetMutation setMutation(String path, Map<String, Object> values) {

0 commit comments

Comments
 (0)