Skip to content

Commit 8ea9471

Browse files
schmidt-sebastianschmidt-sebastian
authored
Index-Free: Using readTime instead of updateTime (#686)
1 parent 016fd12 commit 8ea9471

File tree

9 files changed

+74
-39
lines changed

9 files changed

+74
-39
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,7 @@ public ImmutableSortedMap<DocumentKey, MaybeDocument> applyRemoteEvent(RemoteEve
392392
|| doc.getVersion().equals(SnapshotVersion.NONE)
393393
|| (authoritativeUpdates.contains(doc.getKey()) && !existingDoc.hasPendingWrites())
394394
|| doc.getVersion().compareTo(existingDoc.getVersion()) >= 0) {
395-
remoteDocuments.add(doc);
395+
remoteDocuments.add(doc, remoteEvent.getSnapshotVersion());
396396
changedDocs.put(key, doc);
397397
} else {
398398
Logger.debug(
@@ -629,7 +629,7 @@ private void applyWriteToRemoteDocuments(MutationBatchResult batchResult) {
629629
batch,
630630
remoteDoc);
631631
} else {
632-
remoteDocuments.add(doc);
632+
remoteDocuments.add(doc, batchResult.getCommitVersion());
633633
}
634634
}
635635
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,8 +117,8 @@ public int removeTargets(long upperBound, SparseArray<?> activeTargetIds) {
117117
public int removeOrphanedDocuments(long upperBound) {
118118
int count = 0;
119119
MemoryRemoteDocumentCache cache = persistence.getRemoteDocumentCache();
120-
for (Map.Entry<DocumentKey, MaybeDocument> entry : cache.getDocuments()) {
121-
DocumentKey key = entry.getKey();
120+
for (MaybeDocument doc : cache.getDocuments()) {
121+
DocumentKey key = doc.getKey();
122122
if (!isPinned(key, upperBound)) {
123123
cache.remove(key);
124124
orphanedSequenceNumbers.remove(key);

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

Lines changed: 42 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,38 +15,40 @@
1515
package com.google.firebase.firestore.local;
1616

1717
import static com.google.firebase.firestore.model.DocumentCollections.emptyDocumentMap;
18-
import static com.google.firebase.firestore.model.DocumentCollections.emptyMaybeDocumentMap;
1918
import static com.google.firebase.firestore.util.Assert.hardAssert;
2019

20+
import android.util.Pair;
21+
import androidx.annotation.NonNull;
2122
import androidx.annotation.Nullable;
2223
import com.google.firebase.database.collection.ImmutableSortedMap;
2324
import com.google.firebase.firestore.core.Query;
2425
import com.google.firebase.firestore.model.Document;
2526
import com.google.firebase.firestore.model.DocumentKey;
2627
import com.google.firebase.firestore.model.MaybeDocument;
2728
import com.google.firebase.firestore.model.ResourcePath;
29+
import com.google.firebase.firestore.model.SnapshotVersion;
2830
import java.util.HashMap;
2931
import java.util.Iterator;
3032
import java.util.Map;
3133

3234
/** In-memory cache of remote documents. */
3335
final class MemoryRemoteDocumentCache implements RemoteDocumentCache {
3436

35-
/** Underlying cache of documents. */
36-
private ImmutableSortedMap<DocumentKey, MaybeDocument> docs;
37+
/** Underlying cache of documents and their read times. */
38+
private ImmutableSortedMap<DocumentKey, Pair<MaybeDocument, SnapshotVersion>> docs;
3739

3840
private final MemoryPersistence persistence;
3941
private StatsCollector statsCollector;
4042

4143
MemoryRemoteDocumentCache(MemoryPersistence persistence, StatsCollector statsCollector) {
42-
docs = emptyMaybeDocumentMap();
44+
docs = ImmutableSortedMap.Builder.emptyMap(DocumentKey.comparator());
4345
this.statsCollector = statsCollector;
4446
this.persistence = persistence;
4547
}
4648

4749
@Override
48-
public void add(MaybeDocument document) {
49-
docs = docs.insert(document.getKey(), document);
50+
public void add(MaybeDocument document, SnapshotVersion readTime) {
51+
docs = docs.insert(document.getKey(), new Pair<>(document, readTime));
5052

5153
persistence.getIndexManager().addToCollectionParentIndex(document.getKey().getPath().popLast());
5254
}
@@ -61,7 +63,8 @@ public void remove(DocumentKey key) {
6163
@Override
6264
public MaybeDocument get(DocumentKey key) {
6365
statsCollector.recordRowsRead(STATS_TAG, 1);
64-
return docs.get(key);
66+
Pair<MaybeDocument, SnapshotVersion> entry = docs.get(key);
67+
return entry != null ? entry.first : null;
6568
}
6669

6770
@Override
@@ -89,12 +92,13 @@ public ImmutableSortedMap<DocumentKey, Document> getAllDocumentsMatchingQuery(Qu
8992
// we need to match the query against.
9093
ResourcePath queryPath = query.getPath();
9194
DocumentKey prefix = DocumentKey.fromPath(queryPath.append(""));
92-
Iterator<Map.Entry<DocumentKey, MaybeDocument>> iterator = docs.iteratorFrom(prefix);
95+
Iterator<Map.Entry<DocumentKey, Pair<MaybeDocument, SnapshotVersion>>> iterator =
96+
docs.iteratorFrom(prefix);
9397

9498
int rowsRead = 0;
9599

96100
while (iterator.hasNext()) {
97-
Map.Entry<DocumentKey, MaybeDocument> entry = iterator.next();
101+
Map.Entry<DocumentKey, Pair<MaybeDocument, SnapshotVersion>> entry = iterator.next();
98102

99103
++rowsRead;
100104

@@ -103,7 +107,7 @@ public ImmutableSortedMap<DocumentKey, Document> getAllDocumentsMatchingQuery(Qu
103107
break;
104108
}
105109

106-
MaybeDocument maybeDoc = entry.getValue();
110+
MaybeDocument maybeDoc = entry.getValue().first;
107111
if (!(maybeDoc instanceof Document)) {
108112
continue;
109113
}
@@ -119,8 +123,8 @@ public ImmutableSortedMap<DocumentKey, Document> getAllDocumentsMatchingQuery(Qu
119123
return result;
120124
}
121125

122-
ImmutableSortedMap<DocumentKey, MaybeDocument> getDocuments() {
123-
return docs;
126+
Iterable<MaybeDocument> getDocuments() {
127+
return new DocumentIterable();
124128
}
125129

126130
/**
@@ -140,10 +144,33 @@ private static long getKeySize(DocumentKey key) {
140144

141145
long getByteSize(LocalSerializer serializer) {
142146
long count = 0;
143-
for (Map.Entry<DocumentKey, MaybeDocument> entry : docs) {
144-
count += getKeySize(entry.getKey());
145-
count += serializer.encodeMaybeDocument(entry.getValue()).getSerializedSize();
147+
for (MaybeDocument doc : new DocumentIterable()) {
148+
count += getKeySize(doc.getKey());
149+
count += serializer.encodeMaybeDocument(doc).getSerializedSize();
146150
}
147151
return count;
148152
}
153+
154+
/**
155+
* A proxy that exposes an iterator over the current set of documents in the RemoteDocumentCache.
156+
*/
157+
private class DocumentIterable implements Iterable<MaybeDocument> {
158+
@NonNull
159+
@Override
160+
public Iterator<MaybeDocument> iterator() {
161+
Iterator<Map.Entry<DocumentKey, Pair<MaybeDocument, SnapshotVersion>>> iterator =
162+
MemoryRemoteDocumentCache.this.docs.iterator();
163+
return new Iterator<MaybeDocument>() {
164+
@Override
165+
public boolean hasNext() {
166+
return iterator.hasNext();
167+
}
168+
169+
@Override
170+
public MaybeDocument next() {
171+
return iterator.next().getValue().first;
172+
}
173+
};
174+
}
175+
}
149176
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.firebase.firestore.model.Document;
2121
import com.google.firebase.firestore.model.DocumentKey;
2222
import com.google.firebase.firestore.model.MaybeDocument;
23+
import com.google.firebase.firestore.model.SnapshotVersion;
2324
import java.util.Map;
2425

2526
/**
@@ -40,8 +41,9 @@ interface RemoteDocumentCache {
4041
* for the key, it will be replaced.
4142
*
4243
* @param maybeDocument A Document or NoDocument to put in the cache.
44+
* @param readTime The time at which the document was read or committed.
4345
*/
44-
void add(MaybeDocument maybeDocument);
46+
void add(MaybeDocument maybeDocument, SnapshotVersion readTime);
4547

4648
/** Removes the cached entry for the given key (no-op if no entry exists). */
4749
void remove(DocumentKey documentKey);

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.google.firebase.firestore.model.DocumentKey;
2727
import com.google.firebase.firestore.model.MaybeDocument;
2828
import com.google.firebase.firestore.model.ResourcePath;
29+
import com.google.firebase.firestore.model.SnapshotVersion;
2930
import com.google.firebase.firestore.util.BackgroundQueue;
3031
import com.google.firebase.firestore.util.Executors;
3132
import com.google.protobuf.InvalidProtocolBufferException;
@@ -50,16 +51,16 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache {
5051
}
5152

5253
@Override
53-
public void add(MaybeDocument maybeDocument) {
54+
public void add(MaybeDocument maybeDocument, SnapshotVersion readTime) {
5455
String path = pathForKey(maybeDocument.getKey());
55-
Timestamp timestamp = maybeDocument.getVersion().getTimestamp();
56+
Timestamp timestamp = readTime.getTimestamp();
5657
MessageLite message = serializer.encodeMaybeDocument(maybeDocument);
5758

5859
statsCollector.recordRowsWritten(STATS_TAG, 1);
5960

6061
db.execute(
6162
"INSERT OR REPLACE INTO remote_documents "
62-
+ "(path, update_time_seconds, update_time_nanos, contents) "
63+
+ "(path, read_time_seconds, read_time_nanos, contents) "
6364
+ "VALUES (?, ?, ?, ?)",
6465
path,
6566
timestamp.getSeconds(),

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ void runMigrations(int fromVersion, int toVersion) {
136136
}
137137

138138
if (fromVersion < 9 && toVersion >= 9) {
139-
addUpdateTime();
139+
addReadTime();
140140
}
141141

142142
/*
@@ -363,13 +363,13 @@ private void addSequenceNumber() {
363363
}
364364
}
365365

366-
private void addUpdateTime() {
367-
if (!tableContainsColumn("remote_documents", "update_time_seconds")) {
366+
private void addReadTime() {
367+
if (!tableContainsColumn("remote_documents", "read_time_seconds")) {
368368
hardAssert(
369-
!tableContainsColumn("remote_documents", "update_time_nanos"),
370-
"Table contained update_time_seconds, but is missing update_time_nanos");
371-
db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_seconds INTEGER");
372-
db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_nanos INTEGER");
369+
!tableContainsColumn("remote_documents", "read_time_nanos"),
370+
"Table contained read_time_nanos, but is missing read_time_seconds");
371+
db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_seconds INTEGER");
372+
db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_nanos INTEGER");
373373
}
374374
}
375375

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,8 @@ public void setUp() {
7373
}
7474

7575
private void addDocument(Document newDoc) {
76-
remoteDocuments.add(newDoc);
76+
// Use document version as read time as the IndexedQueryEngine does not rely on read time.
77+
remoteDocuments.add(newDoc, newDoc.getVersion());
7778
queryEngine.handleDocumentChange(
7879
deletedDoc(newDoc.getKey().toString(), ORIGINAL_VERSION), newDoc);
7980
}
@@ -85,7 +86,7 @@ private void removeDocument(Document oldDoc) {
8586
}
8687

8788
private void updateDocument(Document oldDoc, Document newDoc) {
88-
remoteDocuments.add(newDoc);
89+
remoteDocuments.add(newDoc, newDoc.getVersion());
8990
queryEngine.handleDocumentChange(oldDoc, newDoc);
9091
}
9192

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ private Document nextTestDocument() {
140140

141141
private Document cacheADocumentInTransaction() {
142142
Document doc = nextTestDocument();
143-
documentCache.add(doc);
143+
documentCache.add(doc, doc.getVersion());
144144
return doc;
145145
}
146146

@@ -555,7 +555,7 @@ public void testRemoveTargetsThenGC() {
555555
SnapshotVersion newVersion = version(3);
556556
Document doc =
557557
new Document(middleDocToUpdate, newVersion, Document.DocumentState.SYNCED, testValue);
558-
documentCache.add(doc);
558+
documentCache.add(doc, newVersion);
559559
updateTargetInTransaction(middleTarget);
560560
});
561561

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import static com.google.firebase.firestore.testutil.TestUtil.map;
2222
import static com.google.firebase.firestore.testutil.TestUtil.path;
2323
import static com.google.firebase.firestore.testutil.TestUtil.values;
24+
import static com.google.firebase.firestore.testutil.TestUtil.version;
2425
import static java.util.Arrays.asList;
2526
import static org.junit.Assert.assertEquals;
2627
import static org.junit.Assert.assertNotEquals;
@@ -32,6 +33,7 @@
3233
import com.google.firebase.firestore.model.DocumentKey;
3334
import com.google.firebase.firestore.model.MaybeDocument;
3435
import com.google.firebase.firestore.model.NoDocument;
36+
import com.google.firebase.firestore.model.SnapshotVersion;
3537
import java.util.ArrayList;
3638
import java.util.Arrays;
3739
import java.util.HashMap;
@@ -134,7 +136,7 @@ public void testSetAndReadLotsOfDocuments() {
134136
public void testSetAndReadDeletedDocument() {
135137
String path = "a/b";
136138
NoDocument deletedDoc = deletedDoc(path, 42);
137-
add(deletedDoc);
139+
add(deletedDoc, version(42));
138140
assertEquals(deletedDoc, get(path));
139141
}
140142

@@ -144,7 +146,7 @@ public void testSetDocumentToNewValue() {
144146
Document written = addTestDocumentAtPath(path);
145147

146148
Document newDoc = doc(path, 57, map("data", 5));
147-
add(newDoc);
149+
add(newDoc, version(57));
148150

149151
assertNotEquals(written, newDoc);
150152
assertEquals(newDoc, get(path));
@@ -182,12 +184,14 @@ public void testDocumentsMatchingQuery() {
182184

183185
private Document addTestDocumentAtPath(String path) {
184186
Document doc = doc(path, 42, map("data", 2));
185-
add(doc);
187+
add(doc, version(42));
186188
return doc;
187189
}
188190

189-
private void add(MaybeDocument doc) {
190-
persistence.runTransaction("add entry", () -> remoteDocumentCache.add(doc));
191+
// TODO(mrschmidt): Add a test uses different update and read times and verifies that we correctly
192+
// filter by read time
193+
private void add(MaybeDocument doc, SnapshotVersion readTime) {
194+
persistence.runTransaction("add entry", () -> remoteDocumentCache.add(doc, readTime));
191195
}
192196

193197
@Nullable

0 commit comments

Comments
 (0)