From cf225f9d59861193fd0afd62f5aa006bb0bedb49 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 24 Nov 2021 11:17:23 -0700 Subject: [PATCH 01/20] Migrate remote_documents table to use dedicated collection_path column --- .../local/SQLiteRemoteDocumentCache.java | 121 ++++++++---------- .../firestore/local/SQLiteSchema.java | 70 +++++++++- .../firestore/local/SQLiteSchemaTest.java | 31 ++++- 3 files changed, 149 insertions(+), 73 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index e41c316a391..43c0d349116 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -30,6 +30,7 @@ import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.MessageLite; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -57,15 +58,16 @@ public void add(MutableDocument document, SnapshotVersion readTime) { !readTime.equals(SnapshotVersion.NONE), "Cannot add document to the RemoteDocumentCache with a read time of zero"); - String path = pathForKey(document.getKey()); + DocumentKey documentKey = document.getKey(); Timestamp timestamp = readTime.getTimestamp(); MessageLite message = serializer.encodeMaybeDocument(document); db.execute( "INSERT OR REPLACE INTO remote_documents " - + "(path, read_time_seconds, read_time_nanos, contents) " - + "VALUES (?, ?, ?, ?)", - path, + + "(collection_path, document_id, read_time_seconds, read_time_nanos, contents) " + + "VALUES (?, ?, ?, ?, ?)", + collectionForKey(documentKey), + documentKey.getPath().getLastSegment(), timestamp.getSeconds(), timestamp.getNanoseconds(), message.toByteArray()); @@ -75,56 +77,63 @@ public void add(MutableDocument document, SnapshotVersion readTime) { @Override public void remove(DocumentKey documentKey) { - String path = pathForKey(documentKey); - - db.execute("DELETE FROM remote_documents WHERE path = ?", path); + db.execute( + "DELETE FROM remote_documents WHERE collection_path = ? AND document_id = ?", + collectionForKey(documentKey), + documentKey.getPath().getLastSegment()); } @Override public MutableDocument get(DocumentKey documentKey) { - String path = pathForKey(documentKey); - MutableDocument document = db.query( - "SELECT contents, read_time_seconds, read_time_nanos " - + "FROM remote_documents " - + "WHERE path = ?") - .binding(path) + "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " + + "WHERE collection_path = ? AND document_id = ?") + .binding(collectionForKey(documentKey), documentKey.getPath().getLastSegment()) .firstValue(row -> decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2))); return document != null ? document : MutableDocument.newInvalidDocument(documentKey); } @Override public Map getAll(Iterable documentKeys) { - List args = new ArrayList<>(); - for (DocumentKey key : documentKeys) { - args.add(EncodedPath.encode(key.getPath())); - } - Map results = new HashMap<>(); + + // We issue one query by collection, so first we have to sort the keys into collection buckets. + Map> collectionToDocumentIds = new HashMap<>(); for (DocumentKey key : documentKeys) { + ResourcePath path = key.getPath(); + List documentIds = collectionToDocumentIds.get(path.popLast()); + if (documentIds == null) { + documentIds = new ArrayList<>(); + collectionToDocumentIds.put(path.popLast(), documentIds); + } + documentIds.add(path.getLastSegment()); + // Make sure each key has a corresponding entry, which is null in case the document is not // found. results.put(key, MutableDocument.newInvalidDocument(key)); } - SQLitePersistence.LongQuery longQuery = - new SQLitePersistence.LongQuery( - db, - "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " - + "WHERE path IN (", - args, - ") ORDER BY path"); - - while (longQuery.hasMoreSubqueries()) { - longQuery - .performNextSubquery() - .forEach( - row -> { - MutableDocument decoded = - decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2)); - results.put(decoded.getKey(), decoded); - }); + for (Map.Entry> entry : collectionToDocumentIds.entrySet()) { + SQLitePersistence.LongQuery longQuery = + new SQLitePersistence.LongQuery( + db, + "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " + + "WHERE collection_path = ? AND document_id IN (", + Collections.singletonList(EncodedPath.encode(entry.getKey())), + entry.getValue(), + ")"); + + while (longQuery.hasMoreSubqueries()) { + longQuery + .performNextSubquery() + .forEach( + row -> { + MutableDocument decoded = + decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2)); + results.put(decoded.getKey(), decoded); + }); + } } return results; @@ -137,12 +146,7 @@ public ImmutableSortedMap getAllDocumentsMatchingQ !query.isCollectionGroupQuery(), "CollectionGroup queries should be handled in LocalDocumentsView"); - // Use the query path as a prefix for testing if a document matches the query. - ResourcePath prefix = query.getPath(); - int immediateChildrenPathLength = prefix.length() + 1; - - String prefixPath = EncodedPath.encode(prefix); - String prefixSuccessorPath = EncodedPath.prefixSuccessor(prefixPath); + String collectionPath = EncodedPath.encode(query.getPath()); Timestamp readTime = sinceReadTime.getTimestamp(); BackgroundQueue backgroundQueue = new BackgroundQueue(); @@ -155,43 +159,30 @@ public ImmutableSortedMap getAllDocumentsMatchingQ if (sinceReadTime.equals(SnapshotVersion.NONE)) { sqlQuery = db.query( - "SELECT path, contents, read_time_seconds, read_time_nanos " - + "FROM remote_documents WHERE path >= ? AND path < ?") - .binding(prefixPath, prefixSuccessorPath); + "SELECT contents, read_time_seconds, read_time_nanos " + + "FROM remote_documents WHERE collection_path = ?") + .binding(collectionPath); } else { // Execute an index-free query and filter by read time. This is safe since all document // changes to queries that have a lastLimboFreeSnapshotVersion (`sinceReadTime`) have a read // time set. sqlQuery = db.query( - "SELECT path, contents, read_time_seconds, read_time_nanos " - + "FROM remote_documents WHERE path >= ? AND path < ?" + "SELECT contents, read_time_seconds, read_time_nanos " + + "FROM remote_documents WHERE collection_path = ? " + "AND (read_time_seconds > ? OR (read_time_seconds = ? AND read_time_nanos > ?))") .binding( - prefixPath, - prefixSuccessorPath, + collectionPath, readTime.getSeconds(), readTime.getSeconds(), readTime.getNanoseconds()); } sqlQuery.forEach( row -> { - // TODO: Actually implement a single-collection query - // - // The query is actually returning any path that starts with the query path prefix - // which may include documents in subcollections. For example, a query on 'rooms' - // will return rooms/abc/messages/xyx but we shouldn't match it. Fix this by - // discarding rows with document keys more than one segment longer than the query - // path. - ResourcePath path = EncodedPath.decodeResourcePath(row.getString(0)); - if (path.length() != immediateChildrenPathLength) { - return; - } - // Store row values in array entries to provide the correct context inside the executor. - final byte[] rawDocument = row.getBlob(1); - final int[] readTimeSeconds = {row.getInt(2)}; - final int[] readTimeNanos = {row.getInt(3)}; + final byte[] rawDocument = row.getBlob(0); + final int[] readTimeSeconds = {row.getInt(1)}; + final int[] readTimeNanos = {row.getInt(2)}; // Since scheduling background tasks incurs overhead, we only dispatch to a // background thread if there are still some documents remaining. @@ -228,8 +219,8 @@ public SnapshotVersion getLatestReadTime() { return latestReadTime != null ? latestReadTime : SnapshotVersion.NONE; } - private String pathForKey(DocumentKey key) { - return EncodedPath.encode(key.getPath()); + private String collectionForKey(DocumentKey key) { + return EncodedPath.encode(key.getPath().popLast()); } private MutableDocument decodeMaybeDocument( diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index 76b158eceff..d320dc22b70 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -49,7 +49,7 @@ class SQLiteSchema { * The version of the schema. Increase this by one for each migration added to runMigrations * below. */ - static final int VERSION = 12; + static final int VERSION = 13; static final int OVERLAY_SUPPORT_VERSION = VERSION + 1; @@ -57,13 +57,13 @@ class SQLiteSchema { static final int INDEXING_SUPPORT_VERSION = OVERLAY_SUPPORT_VERSION + 1; /** - * The batch size for the sequence number migration in `ensureSequenceNumbers()`. + * The batch size for data migrations. * *

This addresses https://github.com/firebase/firebase-android-sdk/issues/370, where a customer * reported that schema migrations failed for clients with thousands of documents. The number has * been chosen based on manual experiments. */ - private static final int SEQUENCE_NUMBER_BATCH_SIZE = 100; + private static final int MIGRATION_BATCH_SIZE = 100; private final SQLiteDatabase db; @@ -170,6 +170,11 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { if (fromVersion < 12 && toVersion >= 12) { createBundleCache(); } + + if (fromVersion < 13 && toVersion >= 13) { + rewriteDocumentKeys(); + } + /* * Adding a new migration? READ THIS FIRST! * @@ -394,9 +399,6 @@ private void createFieldIndex() { + "directional_value BLOB, " // index values for equality and inequalities + "document_name TEXT, " + "PRIMARY KEY (index_id, uid, array_value, directional_value, document_name))"); - - db.execSQL( - "CREATE INDEX read_time ON remote_documents(read_time_seconds, read_time_nanos)"); }); } @@ -489,7 +491,7 @@ private void ensureSequenceNumbers() { + "SELECT TD.path FROM target_documents AS TD " + "WHERE RD.path = TD.path AND TD.target_id = 0" + ") LIMIT ?") - .binding(SEQUENCE_NUMBER_BATCH_SIZE); + .binding(MIGRATION_BATCH_SIZE); boolean[] resultsRemaining = new boolean[1]; @@ -604,6 +606,60 @@ private void rewriteCanonicalIds() { }); } + /** + * Migrates the remote_documents table to contain a distinct column for the document's collection + * path and its id. + */ + private void rewriteDocumentKeys() { + // SQLite does not support dropping a primary key. To create a new primary key on + // collection_path and document_id we need to create a new table :( + db.execSQL("ALTER TABLE remote_documents RENAME TO tmp;"); + db.execSQL( + "CREATE TABLE remote_documents (" + + "collection_path TEXT, " + + "document_id TEXT, " + + "read_time_nanos INTEGER, " + + "read_time_seconds INTEGER, " + + "contents BLOB, " + + "PRIMARY KEY (collection_path, document_id))"); + db.execSQL( + "CREATE INDEX remote_documents_read_time ON remote_documents (read_time_nanos, read_time_seconds)"); + db.execSQL( + "INSERT INTO remote_documents (collection_path, read_time_nanos, read_time_seconds, contents) " + + "SELECT path AS collection_path, read_time_nanos, read_time_seconds, contents FROM tmp"); + db.execSQL("DROP TABLE tmp;"); + + // Process each entry to split the document key into collection_path and document_id + SQLitePersistence.Query documentsToMigrate = + new SQLitePersistence.Query( + db, + "SELECT collection_path FROM remote_documents WHERE document_id IS NULL LIMIT ?") + .binding(MIGRATION_BATCH_SIZE); + SQLiteStatement insertKey = + db.compileStatement( + "UPDATE remote_documents SET collection_path = ?, document_id = ? WHERE collection_path = ?"); + + boolean[] resultsRemaining = new boolean[1]; + + do { + resultsRemaining[0] = false; + + documentsToMigrate.forEach( + row -> { + resultsRemaining[0] = true; + + String encodedPath = row.getString(0); + ResourcePath decodedPath = EncodedPath.decodeResourcePath(encodedPath); + + insertKey.clearBindings(); + insertKey.bindString(1, EncodedPath.encode(decodedPath.popLast())); + insertKey.bindString(2, decodedPath.getLastSegment()); + insertKey.bindString(3, encodedPath); + hardAssert(insertKey.executeUpdateDelete() != -1, "Failed to update document path"); + }); + } while (resultsRemaining[0]); + } + private void createBundleCache() { ifTablesDontExist( new String[] {"bundles", "named_queries"}, diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index c4a32d47cf7..5a501ae49a1 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -548,7 +548,7 @@ public void rewritesCanonicalIds() { QueryPurpose.LISTEN); db.execSQL( - "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?,?, ?)", + "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (? ,?, ?)", new Object[] { 2, "invalid_canonical_id", serializer.encodeTargetData(initialTargetData).toByteArray() }); @@ -571,6 +571,35 @@ public void rewritesCanonicalIds() { }); } + @Test + public void rewritesDocumentKeys() { + schema.runSchemaUpgrades(0, 12); + + ResourcePath path = path("coll/docA"); + db.execSQL( + "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?,?, ?, ?)", + new Object[] {EncodedPath.encode(path), 1, 2, new byte[] {3}}); + + schema.runSchemaUpgrades(12, 13); + new SQLitePersistence.Query( + db, + "SELECT collection_path, document_id, read_time_seconds, read_time_nanos, contents FROM remote_documents") + .forEach( + cursor -> { + String encodedCollectionPath = cursor.getString(0); + String documentId = cursor.getString(1); + long readTimeSeconds = cursor.getLong(2); + int readTimeNanos = cursor.getInt(3); + byte[] contents = cursor.getBlob(4); + + assertEquals(path("coll"), EncodedPath.decodeResourcePath(encodedCollectionPath)); + assertEquals("docA", documentId); + assertEquals(1, readTimeSeconds); + assertEquals(2, readTimeNanos); + assertArrayEquals(new byte[] {3}, contents); + }); + } + @Test public void createsBundlesTable() { schema.runSchemaUpgrades(); From 7f16305b05a140a023c3cec85e16416e89884bd1 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 24 Nov 2021 16:00:58 -0700 Subject: [PATCH 02/20] Add timer Changes to be committed: --- .../google/firebase/firestore/local/SQLiteSchema.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index d320dc22b70..81742219329 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -103,6 +103,8 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { * requirements for new migrations. */ + long startTime = System.currentTimeMillis(); + if (fromVersion < 1 && toVersion >= 1) { createV1MutationQueue(); createV1TargetCache(); @@ -199,6 +201,13 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { Preconditions.checkState(Persistence.INDEXING_SUPPORT_ENABLED); createFieldIndex(); } + + Logger.debug( + "SQLiteSchema", + "Migration from version %s to %s took %s milliseconds", + fromVersion, + toVersion, + System.currentTimeMillis() - startTime); } /** From ba26b877e622caf2f06965d368d7b840f796ce52 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 29 Nov 2021 09:22:41 -0700 Subject: [PATCH 03/20] Add DocumentId helpers --- .../google/firebase/firestore/ConformanceTest.java | 2 +- .../firestore/conformance/TestCaseConverter.java | 2 +- .../firebase/firestore/DocumentReference.java | 4 ++-- .../firebase/firestore/DocumentSnapshot.java | 2 +- .../firestore/local/MemoryMutationQueue.java | 2 +- .../firestore/local/MemoryRemoteDocumentCache.java | 2 +- .../firestore/local/SQLiteMutationQueue.java | 2 +- .../firestore/local/SQLiteRemoteDocumentCache.java | 14 +++++--------- .../firebase/firestore/model/DocumentKey.java | 10 ++++++++++ .../firebase/firestore/testutil/TestUtil.java | 2 +- 10 files changed, 24 insertions(+), 18 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ConformanceTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ConformanceTest.java index fcc0642e566..dc585fad67c 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ConformanceTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ConformanceTest.java @@ -284,6 +284,6 @@ private com.google.firebase.firestore.FieldPath formatFieldPath(String serverFor } private String getId(Document document) { - return DocumentKey.fromName(document.getName()).getPath().getLastSegment(); + return DocumentKey.fromName(document.getName()).getDocumentId(); } } diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/conformance/TestCaseConverter.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/conformance/TestCaseConverter.java index 659f1465129..ac585ddad3c 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/conformance/TestCaseConverter.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/conformance/TestCaseConverter.java @@ -48,7 +48,7 @@ private List convertDatabaseContents(DatastoreTestTrace.FirestoreV1A if (response.hasDocument()) { Document document = response.getDocument(); DocumentKey documentKey = DocumentKey.fromName(document.getName()); - String collectionId = documentKey.getPath().popLast().getLastSegment(); + String collectionId = documentKey.getCollectionGroup(); List documents = collections.computeIfAbsent(collectionId, name -> new ArrayList<>()); documents.add(document); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index 1e91a331208..54a129e2179 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -96,7 +96,7 @@ public FirebaseFirestore getFirestore() { @NonNull public String getId() { - return key.getPath().getLastSegment(); + return key.getDocumentId(); } /** @@ -106,7 +106,7 @@ public String getId() { */ @NonNull public CollectionReference getParent() { - return new CollectionReference(key.getPath().popLast(), firestore); + return new CollectionReference(key.getCollection(), firestore); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java index 5cd8cb8c9b9..26e9094fdfc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java @@ -102,7 +102,7 @@ static DocumentSnapshot fromNoDocument( /** @return The id of the document. */ @NonNull public String getId() { - return key.getPath().getLastSegment(); + return key.getDocumentId(); } /** @return The metadata for this document snapshot. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java index d5f993cd432..3dea721ebfd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java @@ -152,7 +152,7 @@ public MutationBatch addMutationBatch( batchesByDocumentKey = batchesByDocumentKey.insert(new DocumentReference(mutation.getKey(), batchId)); - indexManager.addToCollectionParentIndex(mutation.getKey().getPath().popLast()); + indexManager.addToCollectionParentIndex(mutation.getKey().getCollection()); } return batch; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java index ff86084bcba..e4ce72b162a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java @@ -58,7 +58,7 @@ public void add(MutableDocument document, SnapshotVersion readTime) { docs = docs.insert(document.getKey(), new Pair<>(document.clone(), readTime)); latestReadTime = readTime.compareTo(latestReadTime) > 0 ? readTime : latestReadTime; - indexManager.addToCollectionParentIndex(document.getKey().getPath().popLast()); + indexManager.addToCollectionParentIndex(document.getKey().getCollection()); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java index a19e992fc8e..d97ba54c2f1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java @@ -215,7 +215,7 @@ public MutationBatch addMutationBatch( String path = EncodedPath.encode(key.getPath()); db.execute(indexInserter, uid, path, batchId); - indexManager.addToCollectionParentIndex(key.getPath().popLast()); + indexManager.addToCollectionParentIndex(key.getCollection()); } return batch; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 43c0d349116..b37080f4518 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -66,21 +66,21 @@ public void add(MutableDocument document, SnapshotVersion readTime) { "INSERT OR REPLACE INTO remote_documents " + "(collection_path, document_id, read_time_seconds, read_time_nanos, contents) " + "VALUES (?, ?, ?, ?, ?)", - collectionForKey(documentKey), + EncodedPath.encode(documentKey.getCollection()), documentKey.getPath().getLastSegment(), timestamp.getSeconds(), timestamp.getNanoseconds(), message.toByteArray()); - indexManager.addToCollectionParentIndex(document.getKey().getPath().popLast()); + indexManager.addToCollectionParentIndex(document.getKey().getCollection()); } @Override public void remove(DocumentKey documentKey) { db.execute( "DELETE FROM remote_documents WHERE collection_path = ? AND document_id = ?", - collectionForKey(documentKey), - documentKey.getPath().getLastSegment()); + EncodedPath.encode(documentKey.getCollection()), + documentKey.getDocumentId()); } @Override @@ -89,7 +89,7 @@ public MutableDocument get(DocumentKey documentKey) { db.query( "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " + "WHERE collection_path = ? AND document_id = ?") - .binding(collectionForKey(documentKey), documentKey.getPath().getLastSegment()) + .binding(EncodedPath.encode(documentKey.getCollection()), documentKey.getDocumentId()) .firstValue(row -> decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2))); return document != null ? document : MutableDocument.newInvalidDocument(documentKey); } @@ -219,10 +219,6 @@ public SnapshotVersion getLatestReadTime() { return latestReadTime != null ? latestReadTime : SnapshotVersion.NONE; } - private String collectionForKey(DocumentKey key) { - return EncodedPath.encode(key.getPath().popLast()); - } - private MutableDocument decodeMaybeDocument( byte[] bytes, int readTimeSeconds, int readTimeNanos) { try { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java index c06be7abc1f..90faaec55b8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java @@ -113,6 +113,16 @@ public String getCollectionGroup() { return path.getSegment(path.length() - 2); } + /** Returns the fully qualified path to the parent collection. */ + public ResourcePath getCollection() { + return path.popLast(); + } + + /** Returns the ID for this document key (i.e. the last path segment). */ + public String getDocumentId() { + return path.getLastSegment(); + } + /** Returns true if the document is in the specified collectionId. */ public boolean hasCollectionId(String collectionId) { return path.length() >= 2 && path.segments.get(path.length() - 2).equals(collectionId); diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index 66590e28dfc..f7ac5d87b69 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -440,7 +440,7 @@ public ImmutableSortedSet getRemoteKeysForTarget(int targetId) { @Override public TargetData getTargetDataForTarget(int targetId) { - ResourcePath collectionPath = docs.get(0).getKey().getPath().popLast(); + ResourcePath collectionPath = docs.get(0).getKey().getCollection(); return targetData(targetId, QueryPurpose.LISTEN, collectionPath.toString()); } }); From 60a424056e706e841f54050154304c4476f81235 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 29 Nov 2021 09:34:10 -0700 Subject: [PATCH 04/20] Add one more test --- .../firestore/local/SQLiteSchema.java | 2 +- .../firestore/local/SQLiteSchemaTest.java | 28 ++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index 81742219329..ee159356702 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -63,7 +63,7 @@ class SQLiteSchema { * reported that schema migrations failed for clients with thousands of documents. The number has * been chosen based on manual experiments. */ - private static final int MIGRATION_BATCH_SIZE = 100; + @VisibleForTesting static final int MIGRATION_BATCH_SIZE = 100; private final SQLiteDatabase db; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index 5a501ae49a1..5d529527012 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -577,7 +577,7 @@ public void rewritesDocumentKeys() { ResourcePath path = path("coll/docA"); db.execSQL( - "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?,?, ?, ?)", + "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)", new Object[] {EncodedPath.encode(path), 1, 2, new byte[] {3}}); schema.runSchemaUpgrades(12, 13); @@ -600,6 +600,32 @@ public void rewritesDocumentKeys() { }); } + @Test + public void usesMultipleBatchesToRewriteDocumentKeys() { + schema.runSchemaUpgrades(0, 12); + + for (int i = 0; i < SQLiteSchema.MIGRATION_BATCH_SIZE + 1; ++i) { + ResourcePath path = path(String.format("coll/doc%03d", i)); + db.execSQL( + "INSERT INTO remote_documents (path) VALUES (?)", + new Object[] {EncodedPath.encode(path)}); + } + + schema.runSchemaUpgrades(12, 13); + + int[] current = new int[] {0}; + + new SQLitePersistence.Query(db, "SELECT document_id FROM remote_documents ORDER by document_id") + .forEach( + cursor -> { + String documentId = cursor.getString(0); + assertEquals(String.format("doc%03d", current[0]), documentId); + ++current[0]; + }); + + assertEquals(current[0], SQLiteSchema.MIGRATION_BATCH_SIZE + 1); + } + @Test public void createsBundlesTable() { schema.runSchemaUpgrades(); From 9a1253006e9d80415e141b38cad266935ce72ce7 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 29 Nov 2021 09:39:05 -0700 Subject: [PATCH 05/20] Rename --- .../com/google/firebase/firestore/DocumentReference.java | 2 +- .../firebase/firestore/local/MemoryMutationQueue.java | 2 +- .../firestore/local/MemoryRemoteDocumentCache.java | 2 +- .../firebase/firestore/local/SQLiteMutationQueue.java | 2 +- .../firestore/local/SQLiteRemoteDocumentCache.java | 8 ++++---- .../com/google/firebase/firestore/model/DocumentKey.java | 2 +- .../com/google/firebase/firestore/testutil/TestUtil.java | 2 +- 7 files changed, 10 insertions(+), 10 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index 54a129e2179..9b5901c3f6a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -106,7 +106,7 @@ public String getId() { */ @NonNull public CollectionReference getParent() { - return new CollectionReference(key.getCollection(), firestore); + return new CollectionReference(key.getCollectionPath(), firestore); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java index 3dea721ebfd..3f980390cae 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java @@ -152,7 +152,7 @@ public MutationBatch addMutationBatch( batchesByDocumentKey = batchesByDocumentKey.insert(new DocumentReference(mutation.getKey(), batchId)); - indexManager.addToCollectionParentIndex(mutation.getKey().getCollection()); + indexManager.addToCollectionParentIndex(mutation.getKey().getCollectionPath()); } return batch; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java index e4ce72b162a..e58fe040cea 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java @@ -58,7 +58,7 @@ public void add(MutableDocument document, SnapshotVersion readTime) { docs = docs.insert(document.getKey(), new Pair<>(document.clone(), readTime)); latestReadTime = readTime.compareTo(latestReadTime) > 0 ? readTime : latestReadTime; - indexManager.addToCollectionParentIndex(document.getKey().getCollection()); + indexManager.addToCollectionParentIndex(document.getKey().getCollectionPath()); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java index d97ba54c2f1..4fdc611b5d5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java @@ -215,7 +215,7 @@ public MutationBatch addMutationBatch( String path = EncodedPath.encode(key.getPath()); db.execute(indexInserter, uid, path, batchId); - indexManager.addToCollectionParentIndex(key.getCollection()); + indexManager.addToCollectionParentIndex(key.getCollectionPath()); } return batch; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index b37080f4518..5f468314b49 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -66,20 +66,20 @@ public void add(MutableDocument document, SnapshotVersion readTime) { "INSERT OR REPLACE INTO remote_documents " + "(collection_path, document_id, read_time_seconds, read_time_nanos, contents) " + "VALUES (?, ?, ?, ?, ?)", - EncodedPath.encode(documentKey.getCollection()), + EncodedPath.encode(documentKey.getCollectionPath()), documentKey.getPath().getLastSegment(), timestamp.getSeconds(), timestamp.getNanoseconds(), message.toByteArray()); - indexManager.addToCollectionParentIndex(document.getKey().getCollection()); + indexManager.addToCollectionParentIndex(document.getKey().getCollectionPath()); } @Override public void remove(DocumentKey documentKey) { db.execute( "DELETE FROM remote_documents WHERE collection_path = ? AND document_id = ?", - EncodedPath.encode(documentKey.getCollection()), + EncodedPath.encode(documentKey.getCollectionPath()), documentKey.getDocumentId()); } @@ -89,7 +89,7 @@ public MutableDocument get(DocumentKey documentKey) { db.query( "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " + "WHERE collection_path = ? AND document_id = ?") - .binding(EncodedPath.encode(documentKey.getCollection()), documentKey.getDocumentId()) + .binding(EncodedPath.encode(documentKey.getCollectionPath()), documentKey.getDocumentId()) .firstValue(row -> decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2))); return document != null ? document : MutableDocument.newInvalidDocument(documentKey); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java index 90faaec55b8..c1cfcce3a05 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/DocumentKey.java @@ -114,7 +114,7 @@ public String getCollectionGroup() { } /** Returns the fully qualified path to the parent collection. */ - public ResourcePath getCollection() { + public ResourcePath getCollectionPath() { return path.popLast(); } diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index f7ac5d87b69..d50cd65d09b 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -440,7 +440,7 @@ public ImmutableSortedSet getRemoteKeysForTarget(int targetId) { @Override public TargetData getTargetDataForTarget(int targetId) { - ResourcePath collectionPath = docs.get(0).getKey().getCollection(); + ResourcePath collectionPath = docs.get(0).getKey().getCollectionPath(); return targetData(targetId, QueryPurpose.LISTEN, collectionPath.toString()); } }); From 07500fc43e3790dabb21342a54ea45653b67bc96 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 29 Nov 2021 10:37:03 -0700 Subject: [PATCH 06/20] Format --- .../firebase/firestore/local/SQLiteRemoteDocumentCache.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 5f468314b49..fec026d07cb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -89,7 +89,8 @@ public MutableDocument get(DocumentKey documentKey) { db.query( "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " + "WHERE collection_path = ? AND document_id = ?") - .binding(EncodedPath.encode(documentKey.getCollectionPath()), documentKey.getDocumentId()) + .binding( + EncodedPath.encode(documentKey.getCollectionPath()), documentKey.getDocumentId()) .firstValue(row -> decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2))); return document != null ? document : MutableDocument.newInvalidDocument(documentKey); } From 7671330e60ab28d17b32d369270bca077cbf564c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 29 Nov 2021 12:36:42 -0700 Subject: [PATCH 07/20] Don't delete column --- .../local/SQLiteRemoteDocumentCache.java | 81 ++++++++----------- .../firestore/local/SQLiteSchema.java | 48 ++++------- .../firestore/local/SQLiteSchemaTest.java | 55 ++++++------- 3 files changed, 77 insertions(+), 107 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index fec026d07cb..a739a616f99 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -23,14 +23,12 @@ import com.google.firebase.firestore.model.DocumentCollections; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MutableDocument; -import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.util.BackgroundQueue; import com.google.firebase.firestore.util.Executors; import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.MessageLite; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -64,10 +62,10 @@ public void add(MutableDocument document, SnapshotVersion readTime) { db.execute( "INSERT OR REPLACE INTO remote_documents " - + "(collection_path, document_id, read_time_seconds, read_time_nanos, contents) " + + "(path, parent_path, read_time_seconds, read_time_nanos, contents) " + "VALUES (?, ?, ?, ?, ?)", + EncodedPath.encode(documentKey.getPath()), EncodedPath.encode(documentKey.getCollectionPath()), - documentKey.getPath().getLastSegment(), timestamp.getSeconds(), timestamp.getNanoseconds(), message.toByteArray()); @@ -77,20 +75,20 @@ public void add(MutableDocument document, SnapshotVersion readTime) { @Override public void remove(DocumentKey documentKey) { - db.execute( - "DELETE FROM remote_documents WHERE collection_path = ? AND document_id = ?", - EncodedPath.encode(documentKey.getCollectionPath()), - documentKey.getDocumentId()); + String path = EncodedPath.encode(documentKey.getPath()); + + db.execute("DELETE FROM remote_documents WHERE path = ?", path); } @Override public MutableDocument get(DocumentKey documentKey) { + String path = EncodedPath.encode(documentKey.getPath()); + MutableDocument document = db.query( - "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " - + "WHERE collection_path = ? AND document_id = ?") - .binding( - EncodedPath.encode(documentKey.getCollectionPath()), documentKey.getDocumentId()) + "SELECT contents, read_time_seconds, read_time_nanos " + + "FROM remote_documents WHERE path = ?") + .binding(path) .firstValue(row -> decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2))); return document != null ? document : MutableDocument.newInvalidDocument(documentKey); } @@ -98,43 +96,32 @@ public MutableDocument get(DocumentKey documentKey) { @Override public Map getAll(Iterable documentKeys) { Map results = new HashMap<>(); - - // We issue one query by collection, so first we have to sort the keys into collection buckets. - Map> collectionToDocumentIds = new HashMap<>(); + List bindVars = new ArrayList<>(); for (DocumentKey key : documentKeys) { - ResourcePath path = key.getPath(); - List documentIds = collectionToDocumentIds.get(path.popLast()); - if (documentIds == null) { - documentIds = new ArrayList<>(); - collectionToDocumentIds.put(path.popLast(), documentIds); - } - documentIds.add(path.getLastSegment()); + bindVars.add(EncodedPath.encode(key.getPath())); // Make sure each key has a corresponding entry, which is null in case the document is not // found. results.put(key, MutableDocument.newInvalidDocument(key)); } - for (Map.Entry> entry : collectionToDocumentIds.entrySet()) { - SQLitePersistence.LongQuery longQuery = - new SQLitePersistence.LongQuery( - db, - "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " - + "WHERE collection_path = ? AND document_id IN (", - Collections.singletonList(EncodedPath.encode(entry.getKey())), - entry.getValue(), - ")"); - - while (longQuery.hasMoreSubqueries()) { - longQuery - .performNextSubquery() - .forEach( - row -> { - MutableDocument decoded = - decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2)); - results.put(decoded.getKey(), decoded); - }); - } + SQLitePersistence.LongQuery longQuery = + new SQLitePersistence.LongQuery( + db, + "SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents " + + "WHERE path IN (", + bindVars, + ") ORDER BY path"); + + while (longQuery.hasMoreSubqueries()) { + longQuery + .performNextSubquery() + .forEach( + row -> { + MutableDocument decoded = + decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2)); + results.put(decoded.getKey(), decoded); + }); } return results; @@ -147,7 +134,7 @@ public ImmutableSortedMap getAllDocumentsMatchingQ !query.isCollectionGroupQuery(), "CollectionGroup queries should be handled in LocalDocumentsView"); - String collectionPath = EncodedPath.encode(query.getPath()); + String parentPath = EncodedPath.encode(query.getPath()); Timestamp readTime = sinceReadTime.getTimestamp(); BackgroundQueue backgroundQueue = new BackgroundQueue(); @@ -161,8 +148,8 @@ public ImmutableSortedMap getAllDocumentsMatchingQ sqlQuery = db.query( "SELECT contents, read_time_seconds, read_time_nanos " - + "FROM remote_documents WHERE collection_path = ?") - .binding(collectionPath); + + "FROM remote_documents WHERE parent_path = ?") + .binding(parentPath); } else { // Execute an index-free query and filter by read time. This is safe since all document // changes to queries that have a lastLimboFreeSnapshotVersion (`sinceReadTime`) have a read @@ -170,10 +157,10 @@ public ImmutableSortedMap getAllDocumentsMatchingQ sqlQuery = db.query( "SELECT contents, read_time_seconds, read_time_nanos " - + "FROM remote_documents WHERE collection_path = ? " + + "FROM remote_documents WHERE parent_path = ? " + "AND (read_time_seconds > ? OR (read_time_seconds = ? AND read_time_nanos > ?))") .binding( - collectionPath, + parentPath, readTime.getSeconds(), readTime.getSeconds(), readTime.getNanoseconds()); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index ee159356702..c02a3dd3e53 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -174,7 +174,8 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { } if (fromVersion < 13 && toVersion >= 13) { - rewriteDocumentKeys(); + addParentPath(); + ensureParentPath(); } /* @@ -408,6 +409,9 @@ private void createFieldIndex() { + "directional_value BLOB, " // index values for equality and inequalities + "document_name TEXT, " + "PRIMARY KEY (index_id, uid, array_value, directional_value, document_name))"); + + db.execSQL( + "CREATE INDEX read_time ON remote_documents(read_time_seconds, read_time_nanos)"); }); } @@ -440,6 +444,13 @@ private void addSequenceNumber() { } } + private void addParentPath() { + if (!tableContainsColumn("remote_documents", "parent_path")) { + db.execSQL("ALTER TABLE remote_documents ADD COLUMN parent_path TEXT"); + db.execSQL("CREATE INDEX parent_path_index ON remote_documents(parent_path)"); + } + } + private boolean hasReadTime() { boolean hasReadTimeSeconds = tableContainsColumn("remote_documents", "read_time_seconds"); boolean hasReadTimeNanos = tableContainsColumn("remote_documents", "read_time_nanos"); @@ -615,38 +626,14 @@ private void rewriteCanonicalIds() { }); } - /** - * Migrates the remote_documents table to contain a distinct column for the document's collection - * path and its id. - */ - private void rewriteDocumentKeys() { - // SQLite does not support dropping a primary key. To create a new primary key on - // collection_path and document_id we need to create a new table :( - db.execSQL("ALTER TABLE remote_documents RENAME TO tmp;"); - db.execSQL( - "CREATE TABLE remote_documents (" - + "collection_path TEXT, " - + "document_id TEXT, " - + "read_time_nanos INTEGER, " - + "read_time_seconds INTEGER, " - + "contents BLOB, " - + "PRIMARY KEY (collection_path, document_id))"); - db.execSQL( - "CREATE INDEX remote_documents_read_time ON remote_documents (read_time_nanos, read_time_seconds)"); - db.execSQL( - "INSERT INTO remote_documents (collection_path, read_time_nanos, read_time_seconds, contents) " - + "SELECT path AS collection_path, read_time_nanos, read_time_seconds, contents FROM tmp"); - db.execSQL("DROP TABLE tmp;"); - - // Process each entry to split the document key into collection_path and document_id + /** Fill the remote_document's parent path column. */ + private void ensureParentPath() { SQLitePersistence.Query documentsToMigrate = new SQLitePersistence.Query( - db, - "SELECT collection_path FROM remote_documents WHERE document_id IS NULL LIMIT ?") + db, "SELECT path FROM remote_documents WHERE parent_path IS NULL LIMIT ?") .binding(MIGRATION_BATCH_SIZE); SQLiteStatement insertKey = - db.compileStatement( - "UPDATE remote_documents SET collection_path = ?, document_id = ? WHERE collection_path = ?"); + db.compileStatement("UPDATE remote_documents SET parent_path = ? WHERE path = ?"); boolean[] resultsRemaining = new boolean[1]; @@ -662,8 +649,7 @@ private void rewriteDocumentKeys() { insertKey.clearBindings(); insertKey.bindString(1, EncodedPath.encode(decodedPath.popLast())); - insertKey.bindString(2, decodedPath.getLastSegment()); - insertKey.bindString(3, encodedPath); + insertKey.bindString(2, encodedPath); hardAssert(insertKey.executeUpdateDelete() != -1, "Failed to update document path"); }); } while (resultsRemaining[0]); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index 5d529527012..f4104fe1266 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -16,6 +16,7 @@ import static com.google.firebase.firestore.local.EncodedPath.decodeResourcePath; import static com.google.firebase.firestore.local.EncodedPath.encode; +import static com.google.firebase.firestore.local.SQLiteSchema.VERSION; import static com.google.firebase.firestore.testutil.TestUtil.filter; import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; @@ -144,7 +145,7 @@ public void migrationsDontDeleteTablesOrColumns() { // deletes an existing table or column, which would be very likely to break old versions of the // SDK relying on that table or column. Map> tables = new HashMap<>(); - for (int toVersion = 1; toVersion <= SQLiteSchema.VERSION; toVersion++) { + for (int toVersion = 1; toVersion <= VERSION; toVersion++) { schema.runSchemaUpgrades(toVersion - 1, toVersion); Map> newTables = getCurrentSchema(); assertNoRemovals(tables, newTables, toVersion); @@ -154,10 +155,10 @@ public void migrationsDontDeleteTablesOrColumns() { @Test public void canRecoverFromDowngrades() { - for (int downgradeVersion = 0; downgradeVersion < SQLiteSchema.VERSION; downgradeVersion++) { + for (int downgradeVersion = 0; downgradeVersion < VERSION; downgradeVersion++) { // Upgrade schema to current, then upgrade from `downgradeVersion` to current schema.runSchemaUpgrades(); - schema.runSchemaUpgrades(downgradeVersion, SQLiteSchema.VERSION); + schema.runSchemaUpgrades(downgradeVersion, VERSION); } } @@ -437,6 +438,8 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() { SQLiteRemoteDocumentCache remoteDocumentCache = createRemoteDocumentCache(); + schema.runSchemaUpgrades(10, VERSION); + // Verify that queries with SnapshotVersion.NONE return all results, regardless of whether the // read time has been set. ImmutableSortedMap results = @@ -572,36 +575,32 @@ public void rewritesCanonicalIds() { } @Test - public void rewritesDocumentKeys() { + public void addParentPaths() { schema.runSchemaUpgrades(0, 12); - ResourcePath path = path("coll/docA"); - db.execSQL( - "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)", - new Object[] {EncodedPath.encode(path), 1, 2, new byte[] {3}}); + ResourcePath paths[] = new ResourcePath[] {path("collA/doc"), path("collB/doc")}; + + for (ResourcePath path : paths) { + db.execSQL( + "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)", + new Object[] {EncodedPath.encode(path)}); + } schema.runSchemaUpgrades(12, 13); - new SQLitePersistence.Query( - db, - "SELECT collection_path, document_id, read_time_seconds, read_time_nanos, contents FROM remote_documents") + + int[] current = new int[] {0}; + new SQLitePersistence.Query(db, "SELECT parent_path FROM remote_documents ORDER BY path") .forEach( cursor -> { - String encodedCollectionPath = cursor.getString(0); - String documentId = cursor.getString(1); - long readTimeSeconds = cursor.getLong(2); - int readTimeNanos = cursor.getInt(3); - byte[] contents = cursor.getBlob(4); - - assertEquals(path("coll"), EncodedPath.decodeResourcePath(encodedCollectionPath)); - assertEquals("docA", documentId); - assertEquals(1, readTimeSeconds); - assertEquals(2, readTimeNanos); - assertArrayEquals(new byte[] {3}, contents); + ResourcePath parentPath = EncodedPath.decodeResourcePath(cursor.getString(0)); + assertEquals(paths[current[0]].popLast(), parentPath); + ++current[0]; }); + assertEquals(2, current[0]); } @Test - public void usesMultipleBatchesToRewriteDocumentKeys() { + public void usesMultipleBatchesToAddParentPaths() { schema.runSchemaUpgrades(0, 12); for (int i = 0; i < SQLiteSchema.MIGRATION_BATCH_SIZE + 1; ++i) { @@ -614,16 +613,14 @@ public void usesMultipleBatchesToRewriteDocumentKeys() { schema.runSchemaUpgrades(12, 13); int[] current = new int[] {0}; - - new SQLitePersistence.Query(db, "SELECT document_id FROM remote_documents ORDER by document_id") + new SQLitePersistence.Query(db, "SELECT parent_path FROM remote_documents ORDER by path") .forEach( cursor -> { - String documentId = cursor.getString(0); - assertEquals(String.format("doc%03d", current[0]), documentId); + ResourcePath parentPath = EncodedPath.decodeResourcePath(cursor.getString(0)); + assertEquals(path("coll"), parentPath); ++current[0]; }); - - assertEquals(current[0], SQLiteSchema.MIGRATION_BATCH_SIZE + 1); + assertEquals(SQLiteSchema.MIGRATION_BATCH_SIZE + 1, current[0]); } @Test From b25191f58070b29315f41441dc3110e5acd23bec Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 30 Nov 2021 09:15:17 -0700 Subject: [PATCH 08/20] Changelog --- firebase-firestore/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 1b19e9d6981..9255ea43d7a 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -3,6 +3,10 @@ by opting into a release at [go/firebase-android-release](http:go/firebase-android-release) (Googlers only). # Unreleased +- [changed] Improved performance for queries with collections that contain + subcollections. + +# 24.0.0 - [feature] Added support for Firebase AppCheck. # 23.0.4 From 723defc6f82f8ed716b976f2b9f01e62ee3435e2 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 30 Nov 2021 13:43:25 -0700 Subject: [PATCH 09/20] Use PathLength --- .../local/SQLiteRemoteDocumentCache.java | 108 +++++++++--------- .../firestore/local/SQLiteSchema.java | 22 ++-- .../firestore/local/SQLiteSchemaTest.java | 16 ++- 3 files changed, 74 insertions(+), 72 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index c868473b4b4..8fca8e81088 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -63,10 +63,10 @@ public void add(MutableDocument document, SnapshotVersion readTime) { db.execute( "INSERT OR REPLACE INTO remote_documents " - + "(path, parent_path, read_time_seconds, read_time_nanos, contents) " + + "(path, path_length, read_time_seconds, read_time_nanos, contents) " + "VALUES (?, ?, ?, ?, ?)", EncodedPath.encode(documentKey.getPath()), - EncodedPath.encode(documentKey.getCollectionPath()), + documentKey.getPath().length(), timestamp.getSeconds(), timestamp.getNanoseconds(), message.toByteArray()); @@ -135,61 +135,65 @@ public ImmutableSortedMap getAllDocumentsMatchingQ !query.isCollectionGroupQuery(), "CollectionGroup queries should be handled in LocalDocumentsView"); - String parentPath = EncodedPath.encode(query.getPath()); - BackgroundQueue backgroundQueue = new BackgroundQueue(); + StringBuilder sql = + new StringBuilder( + "SELECT contents, read_time_seconds, read_time_nanos " + + "FROM remote_documents WHERE path >= ? AND path < ? AND path_length = ?"); - ImmutableSortedMap[] matchingDocuments = - (ImmutableSortedMap[]) - new ImmutableSortedMap[] {DocumentCollections.emptyMutableDocumentMap()}; + Object[] bindVars = new Object[3 + (FieldIndex.IndexOffset.NONE.equals(offset) ? 0 : 6)]; - SQLitePersistence.Query sqlQuery; - if (FieldIndex.IndexOffset.NONE.equals(offset)) { - sqlQuery = - db.query( - "SELECT contents, read_time_seconds, read_time_nanos " - + "FROM remote_documents WHERE parent_path = ?") - .binding(parentPath); - } else { + String prefix = EncodedPath.encode(query.getPath()); + + int i = 0; + bindVars[i++] = prefix; + bindVars[i++] = EncodedPath.prefixSuccessor(prefix); + bindVars[i++] = query.getPath().length() + 1; + + if (!FieldIndex.IndexOffset.NONE.equals(offset)) { Timestamp readTime = offset.getReadTime().getTimestamp(); DocumentKey documentKey = offset.getDocumentKey(); - sqlQuery = - db.query( - "SELECT contents, read_time_seconds, read_time_nanos " - + "FROM remote_documents WHERE parent_path = ? AND (" - + "read_time_seconds > ? OR (" - + "read_time_seconds = ? AND read_time_nanos > ?) OR (" - + "read_time_seconds = ? AND read_time_nanos = ? and path > ?))") - .binding( - parentPath, - readTime.getSeconds(), - readTime.getSeconds(), - readTime.getNanoseconds(), - readTime.getSeconds(), - readTime.getNanoseconds(), - EncodedPath.encode(documentKey.getPath())); + sql.append( + " AND (read_time_seconds > ? OR (" + + "read_time_seconds = ? AND read_time_nanos > ?) OR (" + + "read_time_seconds = ? AND read_time_nanos = ? and path > ?))"); + bindVars[i++] = readTime.getSeconds(); + bindVars[i++] = readTime.getSeconds(); + bindVars[i++] = readTime.getNanoseconds(); + bindVars[i++] = readTime.getSeconds(); + bindVars[i++] = readTime.getNanoseconds(); + bindVars[i] = EncodedPath.encode(documentKey.getPath()); } - sqlQuery.forEach( - row -> { - // Store row values in array entries to provide the correct context inside the executor. - final byte[] rawDocument = row.getBlob(0); - final int[] readTimeSeconds = {row.getInt(1)}; - final int[] readTimeNanos = {row.getInt(2)}; - - // Since scheduling background tasks incurs overhead, we only dispatch to a - // background thread if there are still some documents remaining. - Executor executor = row.isLast() ? Executors.DIRECT_EXECUTOR : backgroundQueue; - executor.execute( - () -> { - MutableDocument document = - decodeMaybeDocument(rawDocument, readTimeSeconds[0], readTimeNanos[0]); - if (document.isFoundDocument() && query.matches(document)) { - synchronized (SQLiteRemoteDocumentCache.this) { - matchingDocuments[0] = matchingDocuments[0].insert(document.getKey(), document); - } - } - }); - }); + + ImmutableSortedMap[] results = + (ImmutableSortedMap[]) + new ImmutableSortedMap[] {DocumentCollections.emptyMutableDocumentMap()}; + BackgroundQueue backgroundQueue = new BackgroundQueue(); + + db.query(sql.toString()) + .binding(bindVars) + .forEach( + row -> { + // Store row values in array entries to provide the correct context inside the + // executor. + final byte[] rawDocument = row.getBlob(0); + final int[] readTimeSeconds = {row.getInt(1)}; + final int[] readTimeNanos = {row.getInt(2)}; + + // Since scheduling background tasks incurs overhead, we only dispatch to a + // background thread if there are still some documents remaining. + Executor executor = row.isLast() ? Executors.DIRECT_EXECUTOR : backgroundQueue; + executor.execute( + () -> { + MutableDocument document = + decodeMaybeDocument(rawDocument, readTimeSeconds[0], readTimeNanos[0]); + if (document.isFoundDocument() && query.matches(document)) { + synchronized (SQLiteRemoteDocumentCache.this) { + results[0] = results[0].insert(document.getKey(), document); + } + } + }); + }); try { backgroundQueue.drain(); @@ -197,7 +201,7 @@ public ImmutableSortedMap getAllDocumentsMatchingQ fail("Interrupted while deserializing documents", e); } - return matchingDocuments[0]; + return results[0]; } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index 943df8609cb..ae10014ebca 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -174,8 +174,8 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { } if (fromVersion < 13 && toVersion >= 13) { - addParentPath(); - ensureParentPath(); + addPathLength(); + ensurePathLength(); } /* @@ -447,10 +447,10 @@ private void addSequenceNumber() { } } - private void addParentPath() { - if (!tableContainsColumn("remote_documents", "parent_path")) { - db.execSQL("ALTER TABLE remote_documents ADD COLUMN parent_path TEXT"); - db.execSQL("CREATE INDEX parent_path_index ON remote_documents(parent_path)"); + private void addPathLength() { + if (!tableContainsColumn("remote_documents", "path_length")) { + // The "path_length" column store the number of segments in the path. + db.execSQL("ALTER TABLE remote_documents ADD COLUMN path_length INTEGER"); } } @@ -629,14 +629,14 @@ private void rewriteCanonicalIds() { }); } - /** Fill the remote_document's parent path column. */ - private void ensureParentPath() { + /** Fill the remote_document's path_length column. */ + private void ensurePathLength() { SQLitePersistence.Query documentsToMigrate = new SQLitePersistence.Query( - db, "SELECT path FROM remote_documents WHERE parent_path IS NULL LIMIT ?") + db, "SELECT path FROM remote_documents WHERE path_length IS NULL LIMIT ?") .binding(MIGRATION_BATCH_SIZE); SQLiteStatement insertKey = - db.compileStatement("UPDATE remote_documents SET parent_path = ? WHERE path = ?"); + db.compileStatement("UPDATE remote_documents SET path_length = ? WHERE path = ?"); boolean[] resultsRemaining = new boolean[1]; @@ -651,7 +651,7 @@ private void ensureParentPath() { ResourcePath decodedPath = EncodedPath.decodeResourcePath(encodedPath); insertKey.clearBindings(); - insertKey.bindString(1, EncodedPath.encode(decodedPath.popLast())); + insertKey.bindLong(1, decodedPath.length()); insertKey.bindString(2, encodedPath); hardAssert(insertKey.executeUpdateDelete() != -1, "Failed to update document path"); }); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index 5049fb4bcb6..810d3649aa8 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -578,10 +578,10 @@ public void rewritesCanonicalIds() { } @Test - public void addParentPaths() { + public void addPathLengths() { schema.runSchemaUpgrades(0, 12); - ResourcePath paths[] = new ResourcePath[] {path("collA/doc"), path("collB/doc")}; + ResourcePath paths[] = new ResourcePath[] {path("collA/doc"), path("collA/doc/collB/doc")}; for (ResourcePath path : paths) { db.execSQL( @@ -592,18 +592,17 @@ public void addParentPaths() { schema.runSchemaUpgrades(12, 13); int[] current = new int[] {0}; - new SQLitePersistence.Query(db, "SELECT parent_path FROM remote_documents ORDER BY path") + new SQLitePersistence.Query(db, "SELECT path_length FROM remote_documents ORDER BY path") .forEach( cursor -> { - ResourcePath parentPath = EncodedPath.decodeResourcePath(cursor.getString(0)); - assertEquals(paths[current[0]].popLast(), parentPath); + assertEquals(paths[current[0]].length(), cursor.getInt(0)); ++current[0]; }); assertEquals(2, current[0]); } @Test - public void usesMultipleBatchesToAddParentPaths() { + public void usesMultipleBatchesToAddPathLengths() { schema.runSchemaUpgrades(0, 12); for (int i = 0; i < SQLiteSchema.MIGRATION_BATCH_SIZE + 1; ++i) { @@ -616,11 +615,10 @@ public void usesMultipleBatchesToAddParentPaths() { schema.runSchemaUpgrades(12, 13); int[] current = new int[] {0}; - new SQLitePersistence.Query(db, "SELECT parent_path FROM remote_documents ORDER by path") + new SQLitePersistence.Query(db, "SELECT path_length FROM remote_documents ORDER by path") .forEach( cursor -> { - ResourcePath parentPath = EncodedPath.decodeResourcePath(cursor.getString(0)); - assertEquals(path("coll"), parentPath); + assertEquals(2, cursor.getInt(0)); ++current[0]; }); assertEquals(SQLiteSchema.MIGRATION_BATCH_SIZE + 1, current[0]); From fc00ff69ba38bd3093150370b0ecfe598bc146c0 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 1 Dec 2021 10:55:56 -0700 Subject: [PATCH 10/20] Fetch 50 documents --- .../firestore/local/IndexBackfiller.java | 67 ++++------ .../firestore/local/IndexManager.java | 3 +- .../firestore/local/LocalDocumentsView.java | 11 +- .../firestore/local/MemoryIndexManager.java | 3 +- .../local/MemoryRemoteDocumentCache.java | 40 +++++- .../firestore/local/RemoteDocumentCache.java | 18 ++- .../firestore/local/SQLiteIndexManager.java | 29 +++-- .../firestore/local/SQLitePersistence.java | 4 + .../local/SQLiteRemoteDocumentCache.java | 118 ++++++++++++------ .../firebase/firestore/model/FieldIndex.java | 5 + .../firestore/local/CountingQueryEngine.java | 8 ++ .../firestore/local/IndexBackfillerTest.java | 6 +- .../local/IndexedQueryEngineTest.java | 6 +- .../local/RemoteDocumentCacheTestCase.java | 51 ++++++++ .../local/SQLiteIndexManagerTest.java | 3 +- .../firebase/firestore/testutil/TestUtil.java | 9 +- 16 files changed, 267 insertions(+), 114 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java index ff3caf18390..58fff876637 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java @@ -19,19 +19,15 @@ import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; import com.google.firebase.database.collection.ImmutableSortedMap; -import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex; import com.google.firebase.firestore.model.FieldIndex.IndexOffset; -import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.util.AsyncQueue; import com.google.firebase.firestore.util.Logger; -import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.HashSet; -import java.util.List; +import java.util.Iterator; import java.util.Map; import java.util.Set; import java.util.concurrent.TimeUnit; @@ -138,22 +134,18 @@ private int writeIndexEntries(LocalDocumentsView localDocumentsView) { /** Writes entries for the fetched field indexes. */ private int writeEntriesForCollectionGroup( LocalDocumentsView localDocumentsView, String collectionGroup, int entriesRemainingUnderCap) { - Query query = new Query(ResourcePath.EMPTY, collectionGroup); + // TODO(indexing): Support mutation batch Ids when sorting and writing indexes. // Use the earliest offset of all field indexes to query the local cache. IndexOffset existingOffset = getExistingOffset(indexManager.getFieldIndexes(collectionGroup)); - - // TODO(indexing): Use limit queries to only fetch the required number of entries. - // TODO(indexing): Support mutation batch Ids when sorting and writing indexes. ImmutableSortedMap documents = - localDocumentsView.getDocumentsMatchingQuery(query, existingOffset); - - List oldestDocuments = getOldestDocuments(documents, entriesRemainingUnderCap); - indexManager.updateIndexEntries(oldestDocuments); + localDocumentsView.getDocuments(collectionGroup, existingOffset, entriesRemainingUnderCap); + indexManager.updateIndexEntries(documents); - IndexOffset newOffset = getNewOffset(oldestDocuments, existingOffset); + IndexOffset newOffset = getNewOffset(documents, existingOffset); indexManager.updateCollectionGroup(collectionGroup, newOffset); - return oldestDocuments.size(); + + return documents.size(); } /** Returns the lowest offset for the provided index group. */ @@ -168,37 +160,22 @@ private IndexOffset getExistingOffset(Collection fieldIndexes) { return lowestOffset == null ? IndexOffset.NONE : lowestOffset; } - /** - * Returns the offset for the index based on the newly indexed documents. - * - * @param documents a list of documents sorted by read time and key (ascending) - * @param currentOffset the current offset of the index group - */ - private IndexOffset getNewOffset(List documents, IndexOffset currentOffset) { - IndexOffset latestOffset = - documents.isEmpty() - ? IndexOffset.create(remoteDocumentCache.getLatestReadTime()) - : IndexOffset.create( - documents.get(documents.size() - 1).getReadTime(), - documents.get(documents.size() - 1).getKey()); - // Make sure the index does not go back in time - latestOffset = latestOffset.compareTo(currentOffset) > 0 ? latestOffset : currentOffset; - return latestOffset; - } - - /** Returns up to {@code count} documents sorted by read time and key. */ - private List getOldestDocuments( - ImmutableSortedMap documents, int count) { - List oldestDocuments = new ArrayList<>(); - for (Map.Entry entry : documents) { - oldestDocuments.add(entry.getValue()); + /** Returns the offset for the index based on the newly indexed documents. */ + private IndexOffset getNewOffset( + ImmutableSortedMap documents, IndexOffset currentOffset) { + if (documents.isEmpty()) { + return IndexOffset.create(remoteDocumentCache.getLatestReadTime()); + } else { + IndexOffset latestOffset = currentOffset; + Iterator> it = documents.iterator(); + while (it.hasNext()) { + IndexOffset newOffset = IndexOffset.fromDocument(it.next().getValue()); + if (newOffset.compareTo(latestOffset) > 0) { + latestOffset = newOffset; + } + } + return latestOffset; } - Collections.sort( - oldestDocuments, - (l, r) -> - IndexOffset.create(l.getReadTime(), l.getKey()) - .compareTo(IndexOffset.create(r.getReadTime(), r.getKey()))); - return oldestDocuments.subList(0, Math.min(count, oldestDocuments.size())); } @VisibleForTesting diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java index 7868134d1b6..0307779523e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java @@ -15,6 +15,7 @@ package com.google.firebase.firestore.local; import androidx.annotation.Nullable; +import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.core.Target; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; @@ -97,5 +98,5 @@ public interface IndexManager { void updateCollectionGroup(String collectionGroup, FieldIndex.IndexOffset offset); /** Updates the index entries for the provided documents. */ - void updateIndexEntries(Collection documents); + void updateIndexEntries(ImmutableSortedMap documents); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java index b79d73efd1c..2155a3a0a4c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java @@ -237,10 +237,13 @@ void recalculateAndSaveOverlays(Set documentKeys) { recalculateAndSaveOverlays(docs); } - // TODO: The Querying implementation here should move 100% to the query engines. - // Instead, we should just provide a getCollectionDocuments() method here that return all the - // documents in a given collection so that query engine can do that and then filter in - // memory. + /** Gets the local view of the next {@code count} documents based on their read time. */ + ImmutableSortedMap getDocuments( + String collectionGroup, IndexOffset offset, int count) { + Map docs = + remoteDocumentCache.getAll(collectionGroup, offset, count); + return getLocalViewOfDocuments(docs, new HashSet<>()); + } /** * Performs a query against the local view of all documents. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java index d235c650130..c1758262fd6 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java @@ -16,6 +16,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import androidx.annotation.Nullable; +import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.core.Target; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; @@ -98,7 +99,7 @@ public Collection getFieldIndexes() { } @Override - public void updateIndexEntries(Collection documents) { + public void updateIndexEntries(ImmutableSortedMap documents) { // Field indices are not supported with memory persistence. } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java index a8a7e29cdfb..1ebfba4c3db 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java @@ -27,7 +27,10 @@ import com.google.firebase.firestore.model.SnapshotVersion; import java.util.HashMap; import java.util.Iterator; +import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.TreeSet; /** In-memory cache of remote documents. */ final class MemoryRemoteDocumentCache implements RemoteDocumentCache { @@ -81,6 +84,41 @@ public Map getAll(Iterable keys) { return result; } + @Override + public Map getAll( + String collectionGroup, IndexOffset offset, int count) { + List collectionParents = indexManager.getCollectionParents(collectionGroup); + Set allDocuments = + new TreeSet<>((l, r) -> IndexOffset.fromDocument(l).compareTo(IndexOffset.fromDocument(r))); + Map matchingDocuments = new HashMap<>(); + + for (ResourcePath collectionParent : collectionParents) { + ResourcePath documentParent = collectionParent.append(collectionGroup); + Iterator> iterator = + docs.iteratorFrom(DocumentKey.fromPath(documentParent.append(""))); + while (iterator.hasNext()) { + Map.Entry entry = iterator.next(); + DocumentKey key = entry.getKey(); + + if (!documentParent.isPrefixOf(key.getPath())) { + break; + } + + if (IndexOffset.fromDocument(entry.getValue()).compareTo(offset) > 0) { + allDocuments.add(entry.getValue()); + } + } + } + + Iterator it = allDocuments.iterator(); + while (it.hasNext() && matchingDocuments.size() < count) { + MutableDocument document = it.next(); + matchingDocuments.put(document.getKey(), document); + } + + return matchingDocuments; + } + @Override public ImmutableSortedMap getAllDocumentsMatchingQuery( Query query, IndexOffset offset) { @@ -108,7 +146,7 @@ public ImmutableSortedMap getAllDocumentsMatchingQ continue; } - if (IndexOffset.create(doc.getReadTime(), doc.getKey()).compareTo(offset) <= 0) { + if (IndexOffset.fromDocument(doc).compareTo(offset) <= 0) { continue; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java index 701ddda9d9b..dd04ccca613 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java @@ -17,7 +17,7 @@ import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DocumentKey; -import com.google.firebase.firestore.model.FieldIndex; +import com.google.firebase.firestore.model.FieldIndex.IndexOffset; import com.google.firebase.firestore.model.MutableDocument; import com.google.firebase.firestore.model.SnapshotVersion; import java.util.Map; @@ -65,6 +65,20 @@ interface RemoteDocumentCache { */ Map getAll(Iterable documentKeys); + /** + * Looks up the next {@code count} documents for a collection group based on the provided offset. + * The ordering is based on the document's read time and key. + * + *

This method may return more results than requested if a collection group scan scans more + * than 100 collections. + * + * @param collectionGroup The collection group to scan. + * @param offset The offset to start the scan at. + * @param count The number of results to return. + * @return A map with next set of documents. + */ + Map getAll(String collectionGroup, IndexOffset offset, int count); + /** * Executes a query against the cached Document entries * @@ -78,7 +92,7 @@ interface RemoteDocumentCache { * @return The set of matching documents. */ ImmutableSortedMap getAllDocumentsMatchingQuery( - Query query, FieldIndex.IndexOffset offset); + Query query, IndexOffset offset); /** Returns the latest read time of any document in the cache. */ SnapshotVersion getLatestReadTime(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java index 5aad19168b3..a81274a10eb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java @@ -23,6 +23,8 @@ import androidx.annotation.Nullable; import com.google.firebase.Timestamp; +import com.google.firebase.database.collection.ImmutableSortedMap; +import com.google.firebase.database.collection.LLRBNode; import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.Bound; import com.google.firebase.firestore.core.FieldFilter; @@ -222,19 +224,22 @@ public void deleteFieldIndex(FieldIndex index) { } @Override - public void updateIndexEntries(Collection documents) { + public void updateIndexEntries(ImmutableSortedMap documents) { hardAssert(started, "IndexManager not started"); - for (Document document : documents) { - Collection fieldIndexes = getFieldIndexes(document.getKey().getCollectionGroup()); - for (FieldIndex fieldIndex : fieldIndexes) { - SortedSet existingEntries = - getExistingIndexEntries(document.getKey(), fieldIndex); - SortedSet newEntries = computeIndexEntries(document, fieldIndex); - if (!existingEntries.equals(newEntries)) { - updateEntries(document, existingEntries, newEntries); - } - } - } + documents.inOrderTraversal( + new LLRBNode.NodeVisitor() { + @Override + public void visitEntry(DocumentKey key, Document document) { + Collection fieldIndexes = getFieldIndexes(key.getCollectionGroup()); + for (FieldIndex fieldIndex : fieldIndexes) { + SortedSet existingEntries = getExistingIndexEntries(key, fieldIndex); + SortedSet newEntries = computeIndexEntries(document, fieldIndex); + if (!existingEntries.equals(newEntries)) { + updateEntries(document, existingEntries, newEntries); + } + } + } + }); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index f15d14f4ec4..1977ed066bd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -54,6 +54,10 @@ * helper routines that make dealing with SQLite much more pleasant. */ public final class SQLitePersistence extends Persistence { + /** + * The maximum number of bind args for a single statement. Set to 900 instead of 999 for safety. + */ + public static final int MAX_ARGS = 900; /** * Creates the database name that is used to identify the database to be used with a Firestore diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 8fca8e81088..2c9a69b0aa1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -16,26 +16,32 @@ import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; +import static com.google.firebase.firestore.util.Util.repeatSequence; +import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DocumentCollections; import com.google.firebase.firestore.model.DocumentKey; -import com.google.firebase.firestore.model.FieldIndex; +import com.google.firebase.firestore.model.FieldIndex.IndexOffset; import com.google.firebase.firestore.model.MutableDocument; +import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.util.BackgroundQueue; import com.google.firebase.firestore.util.Executors; import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.MessageLite; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; final class SQLiteRemoteDocumentCache implements RemoteDocumentCache { + /** The number of bind args per collection group in {@link #getAll(String, IndexOffset, int)} */ + @VisibleForTesting static final int BINDS_PER_STATEMENT = 9; private final SQLitePersistence db; private final LocalSerializer serializer; @@ -129,68 +135,86 @@ public Map getAll(Iterable documentKe } @Override - public ImmutableSortedMap getAllDocumentsMatchingQuery( - final Query query, FieldIndex.IndexOffset offset) { - hardAssert( - !query.isCollectionGroupQuery(), - "CollectionGroup queries should be handled in LocalDocumentsView"); - - StringBuilder sql = - new StringBuilder( - "SELECT contents, read_time_seconds, read_time_nanos " - + "FROM remote_documents WHERE path >= ? AND path < ? AND path_length = ?"); + public Map getAll( + String collectionGroup, IndexOffset offset, int count) { + List collectionParents = indexManager.getCollectionParents(collectionGroup); + List collections = new ArrayList<>(collectionParents.size()); + for (ResourcePath collectionParent : collectionParents) { + collections.add(collectionParent.append(collectionGroup)); + } - Object[] bindVars = new Object[3 + (FieldIndex.IndexOffset.NONE.equals(offset) ? 0 : 6)]; + if (collections.isEmpty()) { + return Collections.emptyMap(); + } else if (BINDS_PER_STATEMENT * collections.size() < SQLitePersistence.MAX_ARGS) { + return getAll(collections, offset, count); + } else { + // We need to fan out our collection scan since SQLite only supports 999 binds per statement. + Map results = new HashMap<>(); + int pageSize = SQLitePersistence.MAX_ARGS / BINDS_PER_STATEMENT; + for (int i = 0; i < collections.size(); i += pageSize) { + results.putAll( + getAll( + collections.subList(i, Math.min(collections.size(), i + pageSize)), offset, count)); + } + return results; + } + } - String prefix = EncodedPath.encode(query.getPath()); + /** + * Returns the next {@code count} documents from the provided collections, ordered by read time. + */ + private Map getAll( + List collections, IndexOffset offset, int count) { + Timestamp readTime = offset.getReadTime().getTimestamp(); + DocumentKey documentKey = offset.getDocumentKey(); + StringBuilder sql = + repeatSequence( + "SELECT contents, read_time_seconds, read_time_nanos, path " + + "FROM remote_documents " + + "WHERE path >= ? AND path < ? AND path_length = ? " + + "AND (read_time_seconds > ? OR ( " + + "read_time_seconds = ? AND read_time_nanos > ?) OR ( " + + "read_time_seconds = ? AND read_time_nanos = ? and path > ?)) ", + collections.size(), + " UNION "); + sql.append("ORDER BY read_time_seconds, read_time_nanos, path LIMIT ?"); + + Object[] bindVars = new Object[BINDS_PER_STATEMENT * collections.size() + 1]; int i = 0; - bindVars[i++] = prefix; - bindVars[i++] = EncodedPath.prefixSuccessor(prefix); - bindVars[i++] = query.getPath().length() + 1; - - if (!FieldIndex.IndexOffset.NONE.equals(offset)) { - Timestamp readTime = offset.getReadTime().getTimestamp(); - DocumentKey documentKey = offset.getDocumentKey(); - - sql.append( - " AND (read_time_seconds > ? OR (" - + "read_time_seconds = ? AND read_time_nanos > ?) OR (" - + "read_time_seconds = ? AND read_time_nanos = ? and path > ?))"); + for (ResourcePath collection : collections) { + String prefixPath = EncodedPath.encode(collection); + bindVars[i++] = prefixPath; + bindVars[i++] = EncodedPath.prefixSuccessor(prefixPath); + bindVars[i++] = collection.length() + 1; bindVars[i++] = readTime.getSeconds(); bindVars[i++] = readTime.getSeconds(); bindVars[i++] = readTime.getNanoseconds(); bindVars[i++] = readTime.getSeconds(); bindVars[i++] = readTime.getNanoseconds(); - bindVars[i] = EncodedPath.encode(documentKey.getPath()); + bindVars[i++] = EncodedPath.encode(documentKey.getPath()); } + bindVars[i] = count; - ImmutableSortedMap[] results = - (ImmutableSortedMap[]) - new ImmutableSortedMap[] {DocumentCollections.emptyMutableDocumentMap()}; BackgroundQueue backgroundQueue = new BackgroundQueue(); + Map[] results = + (HashMap[]) (new HashMap[] {new HashMap()}); db.query(sql.toString()) .binding(bindVars) .forEach( row -> { - // Store row values in array entries to provide the correct context inside the - // executor. final byte[] rawDocument = row.getBlob(0); final int[] readTimeSeconds = {row.getInt(1)}; final int[] readTimeNanos = {row.getInt(2)}; - // Since scheduling background tasks incurs overhead, we only dispatch to a - // background thread if there are still some documents remaining. Executor executor = row.isLast() ? Executors.DIRECT_EXECUTOR : backgroundQueue; executor.execute( () -> { MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds[0], readTimeNanos[0]); - if (document.isFoundDocument() && query.matches(document)) { - synchronized (SQLiteRemoteDocumentCache.this) { - results[0] = results[0].insert(document.getKey(), document); - } + synchronized (SQLiteRemoteDocumentCache.this) { + results[0].put(document.getKey(), document); } }); }); @@ -204,6 +228,26 @@ public ImmutableSortedMap getAllDocumentsMatchingQ return results[0]; } + @Override + public ImmutableSortedMap getAllDocumentsMatchingQuery( + final Query query, IndexOffset offset) { + hardAssert( + !query.isCollectionGroupQuery(), + "CollectionGroup queries should be handled in LocalDocumentsView"); + + Map allDocuments = + getAll(Collections.singletonList(query.getPath()), offset, Integer.MAX_VALUE); + + ImmutableSortedMap matchingDocuments = + DocumentCollections.emptyMutableDocumentMap(); + for (MutableDocument document : allDocuments.values()) { + if (document.isFoundDocument() && query.matches(document)) { + matchingDocuments = matchingDocuments.insert(document.getKey(), document); + } + } + return matchingDocuments; + } + @Override public SnapshotVersion getLatestReadTime() { SnapshotVersion latestReadTime = diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java index b2fe8e806cf..e3d074cf653 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java @@ -145,6 +145,11 @@ public static IndexOffset create(SnapshotVersion readTime) { return new AutoValue_FieldIndex_IndexOffset(successor, DocumentKey.empty()); } + /** Creates a new offset based on the provided document. */ + public static IndexOffset fromDocument(Document document) { + return new AutoValue_FieldIndex_IndexOffset(document.getReadTime(), document.getKey()); + } + /** * Returns the latest read time version that has been indexed by Firestore for this field index. */ diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java index babe9bb5d24..3ed87d137ca 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java @@ -149,6 +149,14 @@ public Map getAll(Iterable documentKe return result; } + @Override + public Map getAll( + String collectionGroup, IndexOffset offset, int count) { + Map result = subject.getAll(collectionGroup, offset, count); + documentsReadByQuery[0] += result.size(); + return result; + } + @Override public ImmutableSortedMap getAllDocumentsMatchingQuery( Query query, IndexOffset offset) { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java index 7a268674c98..983017002e9 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java @@ -27,6 +27,7 @@ import com.google.firebase.firestore.core.Target; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex; +import com.google.firebase.firestore.model.FieldIndex.IndexOffset; import com.google.firebase.firestore.model.MutableDocument; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.testutil.TestUtil; @@ -123,6 +124,7 @@ public void testBackfillWritesLatestReadTimeToFieldIndexOnCompletion() { @Test public void testBackfillFetchesDocumentsAfterEarliestReadTime() { + addDoc("latest/doc", "foo", version(10)); addFieldIndex("coll1", "foo", version(10)); // Documents before earliest read time should not be fetched. @@ -130,9 +132,9 @@ public void testBackfillFetchesDocumentsAfterEarliestReadTime() { int documentsProcessed = backfiller.backfill(); assertEquals(0, documentsProcessed); - // Read time of index should not change. + // Read time should be the highest read time from the cache. Iterator it = indexManager.getFieldIndexes("coll1").iterator(); - assertEquals(version(10), it.next().getIndexState().getOffset().getReadTime()); + assertEquals(IndexOffset.create(version(10)), it.next().getIndexState().getOffset()); // Documents that are after the earliest read time but before field index read time are fetched. addDoc("coll1/docB", "boo", version(19)); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java index f84e94345a6..95650107423 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java @@ -15,6 +15,7 @@ package com.google.firebase.firestore.local; import static com.google.firebase.firestore.testutil.TestUtil.doc; +import static com.google.firebase.firestore.testutil.TestUtil.docMap; import static com.google.firebase.firestore.testutil.TestUtil.filter; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.query; @@ -27,7 +28,6 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MutableDocument; import com.google.firebase.firestore.model.SnapshotVersion; -import java.util.Collections; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; @@ -80,10 +80,10 @@ public void combinesIndexedWithNonIndexedResults() { MutableDocument doc3 = doc("coll/c", 3, map("foo", true)); remoteDocuments.add(doc1, doc1.getVersion()); - indexManager.updateIndexEntries(Collections.singletonList(doc1)); + indexManager.updateIndexEntries(docMap(doc1)); remoteDocuments.add(doc2, doc2.getVersion()); - indexManager.updateIndexEntries(Collections.singletonList(doc2)); + indexManager.updateIndexEntries(docMap(doc2)); remoteDocuments.add(doc3, doc3.getVersion()); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java index b8a72a1b981..dee6fe13ecc 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java @@ -246,6 +246,57 @@ public void testDocumentsMatchingUsesReadTimeNotUpdateTime() { assertThat(values(results)).containsExactly(doc("b/old", 1, DOC_DATA)); } + @Test + public void testNextDocumentsFromCollectionGroup() { + addTestDocumentAtPath("a/1"); + addTestDocumentAtPath("a/2"); + addTestDocumentAtPath("b/3"); + + Map results = + remoteDocumentCache.getAll("a", IndexOffset.NONE, Integer.MAX_VALUE); + assertThat(results.keySet()).containsExactly(key("a/1"), key("a/2")); + } + + @Test + public void testNextDocumentsFromCollectionGroupWithLimit() { + addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11); + addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 1, /* readTime= */ 12); + addTestDocumentAtPath("a/3", /* updateTime= */ 1, /* readTime= */ 13); + + Map results = + remoteDocumentCache.getAll("a", IndexOffset.NONE, 2); + assertThat(results.keySet()).containsExactly(key("a/1"), key("b/2/a/2")); + } + + @Test + public void testNextDocumentsFromCollectionGroupWithOffset() { + addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11); + addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 2, /* readTime= = */ 12); + addTestDocumentAtPath("a/3", /* updateTime= */ 3, /* readTime= = */ 13); + + Map results = + remoteDocumentCache.getAll("a", IndexOffset.create(version(11)), 2); + assertThat(results.keySet()).containsExactly(key("b/2/a/2"), key("a/3")); + } + + @Test + public void testNextDocumentsForNonExistingCollectionGroup() { + Map results = + remoteDocumentCache.getAll("a", IndexOffset.create(version(11)), 2); + assertThat(results).isEmpty(); + } + + @Test + public void testNextDocumentsForLargeCollectionGroup() { + int size = 999 / SQLiteRemoteDocumentCache.BINDS_PER_STATEMENT + 1; + for (int i = 0; i < size; ++i) { + addTestDocumentAtPath("a/" + i + "/b/doc"); + } + Map results = + remoteDocumentCache.getAll("b", IndexOffset.NONE, size); + assertThat(results).hasSize(size); + } + @Test public void testLatestReadTime() { SnapshotVersion latestReadTime = remoteDocumentCache.getLatestReadTime(); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java index 035751b73ca..ab692c0a82c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java @@ -20,6 +20,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.bound; import static com.google.firebase.firestore.testutil.TestUtil.deletedDoc; import static com.google.firebase.firestore.testutil.TestUtil.doc; +import static com.google.firebase.firestore.testutil.TestUtil.docMap; import static com.google.firebase.firestore.testutil.TestUtil.fieldIndex; import static com.google.firebase.firestore.testutil.TestUtil.filter; import static com.google.firebase.firestore.testutil.TestUtil.key; @@ -707,7 +708,7 @@ private void verifySequenceNumber( } private void addDocs(Document... docs) { - indexManager.updateIndexEntries(Arrays.asList(docs)); + indexManager.updateIndexEntries(docMap(docs)); } private void addDoc(String key, Map data) { diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index d50cd65d09b..3a78f06e079 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -16,7 +16,6 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.firebase.firestore.model.DocumentCollections.emptyDocumentMap; -import static com.google.firebase.firestore.model.DocumentCollections.emptyMutableDocumentMap; import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; @@ -223,10 +222,10 @@ public static MutableDocument unknownDoc(String key, long version) { return MutableDocument.newUnknownDocument(key(key), version(version)); } - public static ImmutableSortedMap docMap( - MutableDocument[] documents) { - ImmutableSortedMap map = emptyMutableDocumentMap(); - for (MutableDocument maybeDocument : documents) { + public static ImmutableSortedMap docMap(T... documents) { + ImmutableSortedMap map = + (ImmutableSortedMap) emptyDocumentMap(); + for (T maybeDocument : documents) { map = map.insert(maybeDocument.getKey(), maybeDocument); } return map; From 759a50ba53d56968fbe72da9d7443f836862a273 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 2 Dec 2021 10:23:51 -0700 Subject: [PATCH 11/20] Simplify --- .../firestore/local/SQLiteIndexManager.java | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java index a81274a10eb..ac541f30322 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java @@ -226,20 +226,16 @@ public void deleteFieldIndex(FieldIndex index) { @Override public void updateIndexEntries(ImmutableSortedMap documents) { hardAssert(started, "IndexManager not started"); - documents.inOrderTraversal( - new LLRBNode.NodeVisitor() { - @Override - public void visitEntry(DocumentKey key, Document document) { - Collection fieldIndexes = getFieldIndexes(key.getCollectionGroup()); - for (FieldIndex fieldIndex : fieldIndexes) { - SortedSet existingEntries = getExistingIndexEntries(key, fieldIndex); - SortedSet newEntries = computeIndexEntries(document, fieldIndex); - if (!existingEntries.equals(newEntries)) { - updateEntries(document, existingEntries, newEntries); - } - } - } - }); + for (Map.Entry entry : documents) { + Collection fieldIndexes = getFieldIndexes(entry.getKey().getCollectionGroup()); + for (FieldIndex fieldIndex : fieldIndexes) { + SortedSet existingEntries = getExistingIndexEntries(entry.getKey(), fieldIndex); + SortedSet newEntries = computeIndexEntries(entry.getValue(), fieldIndex); + if (!existingEntries.equals(newEntries)) { + updateEntries(entry.getValue(), existingEntries, newEntries); + } + } + } } /** From eca7547fc92deb194aec5ae05cc57599f5be4dc0 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 2 Dec 2021 11:23:26 -0700 Subject: [PATCH 12/20] Optimize query execution by deferring query evaluation --- .../firestore/local/LocalDocumentsView.java | 60 +++------------- .../local/MemoryRemoteDocumentCache.java | 47 ++++--------- .../firestore/local/RemoteDocumentCache.java | 15 ++-- .../firestore/local/SQLiteIndexManager.java | 1 - .../local/SQLiteRemoteDocumentCache.java | 23 +------ .../firestore/local/SQLiteSchema.java | 12 +++- .../firestore/model/MutableDocument.java | 10 ++- .../firestore/local/CountingQueryEngine.java | 13 ++-- .../firestore/local/LocalStoreTestCase.java | 19 ++--- .../local/RemoteDocumentCacheTestCase.java | 69 +++++++++---------- .../firestore/local/SQLiteSchemaTest.java | 23 +++---- 11 files changed, 109 insertions(+), 183 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java index 2155a3a0a4c..da03c208b7c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java @@ -322,26 +322,21 @@ private ImmutableSortedMap getDocumentsMatchingCollection /** Queries the remote documents and overlays by doing a full collection scan. */ private ImmutableSortedMap getDocumentsMatchingCollectionQueryFromOverlayCache(Query query, IndexOffset offset) { - ImmutableSortedMap remoteDocuments = - remoteDocumentCache.getAllDocumentsMatchingQuery(query, offset); + Map remoteDocuments = + remoteDocumentCache.getAll(query.getPath(), offset); Map overlays = documentOverlayCache.getOverlays(query.getPath(), -1); - // As documents might match the query because of their overlay we need to include all documents - // in the result. - Set missingDocuments = new HashSet<>(); + // As documents might match the query because of their overlay we need to include documents + // for all overlays in the initial document set. for (Map.Entry entry : overlays.entrySet()) { if (!remoteDocuments.containsKey(entry.getKey())) { - missingDocuments.add(entry.getKey()); + remoteDocuments.put(entry.getKey(), MutableDocument.newInvalidDocument(entry.getKey())); } } - for (Map.Entry entry : - remoteDocumentCache.getAll(missingDocuments).entrySet()) { - remoteDocuments = remoteDocuments.insert(entry.getKey(), entry.getValue()); - } // Apply the overlays and match against the query. ImmutableSortedMap results = emptyDocumentMap(); - for (Map.Entry docEntry : remoteDocuments) { + for (Map.Entry docEntry : remoteDocuments.entrySet()) { Mutation overlay = overlays.get(docEntry.getKey()); if (overlay != null) { overlay.applyToLocalView(docEntry.getValue(), null, Timestamp.now()); @@ -358,14 +353,12 @@ private ImmutableSortedMap getDocumentsMatchingCollection /** Queries the remote documents and mutation queue, by doing a full collection scan. */ private ImmutableSortedMap getDocumentsMatchingCollectionQueryFromMutationQueue(Query query, IndexOffset offset) { - ImmutableSortedMap remoteDocuments = - remoteDocumentCache.getAllDocumentsMatchingQuery(query, offset); + Map remoteDocuments = + remoteDocumentCache.getAll(query.getPath(), offset); // TODO(indexing): We should plumb sinceReadTime through to the mutation queue List matchingBatches = mutationQueue.getAllMutationBatchesAffectingQuery(query); - remoteDocuments = addMissingBaseDocuments(matchingBatches, remoteDocuments); - for (MutationBatch batch : matchingBatches) { for (Mutation mutation : batch.getMutations()) { // Only process documents belonging to the collection. @@ -378,18 +371,18 @@ private ImmutableSortedMap getDocumentsMatchingCollection if (document == null) { // Create invalid document to apply mutations on top of document = MutableDocument.newInvalidDocument(key); - remoteDocuments = remoteDocuments.insert(key, document); + remoteDocuments.put(key, document); } mutation.applyToLocalView( document, FieldMask.fromSet(new HashSet<>()), batch.getLocalWriteTime()); if (!document.isFoundDocument()) { - remoteDocuments = remoteDocuments.remove(key); + remoteDocuments.remove(key); } } } ImmutableSortedMap results = emptyDocumentMap(); - for (Map.Entry docEntry : remoteDocuments) { + for (Map.Entry docEntry : remoteDocuments.entrySet()) { // Finally, insert the documents that still match the query if (query.matches(docEntry.getValue())) { results = results.insert(docEntry.getKey(), docEntry.getValue()); @@ -398,35 +391,4 @@ private ImmutableSortedMap getDocumentsMatchingCollection return results; } - - /** - * It is possible that a {@code PatchMutation} can make a document match a query, even if the - * version in the {@code RemoteDocumentCache} is not a match yet (waiting for server to ack). To - * handle this, we find all document keys affected by the {@code PatchMutation}s that are not in - * {@code existingDocs} yet, and back fill them via {@code remoteDocumentCache.getAll}, otherwise - * those {@code PatchMutation}s will be ignored because no base document can be found, and lead to - * missing results for the query. - */ - private ImmutableSortedMap addMissingBaseDocuments( - List matchingBatches, - ImmutableSortedMap existingDocs) { - HashSet missingDocKeys = new HashSet<>(); - for (MutationBatch batch : matchingBatches) { - for (Mutation mutation : batch.getMutations()) { - if (mutation instanceof PatchMutation && !existingDocs.containsKey(mutation.getKey())) { - missingDocKeys.add(mutation.getKey()); - } - } - } - - ImmutableSortedMap mergedDocs = existingDocs; - Map missingDocs = remoteDocumentCache.getAll(missingDocKeys); - for (Map.Entry entry : missingDocs.entrySet()) { - if (entry.getValue().isFoundDocument()) { - mergedDocs = mergedDocs.insert(entry.getKey(), entry.getValue()); - } - } - - return mergedDocs; - } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java index 1ebfba4c3db..e00bce2a0cc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java @@ -14,12 +14,10 @@ package com.google.firebase.firestore.local; -import static com.google.firebase.firestore.model.DocumentCollections.emptyMutableDocumentMap; import static com.google.firebase.firestore.util.Assert.hardAssert; import androidx.annotation.NonNull; import com.google.firebase.database.collection.ImmutableSortedMap; -import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex.IndexOffset; import com.google.firebase.firestore.model.MutableDocument; @@ -90,26 +88,11 @@ public Map getAll( List collectionParents = indexManager.getCollectionParents(collectionGroup); Set allDocuments = new TreeSet<>((l, r) -> IndexOffset.fromDocument(l).compareTo(IndexOffset.fromDocument(r))); - Map matchingDocuments = new HashMap<>(); - for (ResourcePath collectionParent : collectionParents) { - ResourcePath documentParent = collectionParent.append(collectionGroup); - Iterator> iterator = - docs.iteratorFrom(DocumentKey.fromPath(documentParent.append(""))); - while (iterator.hasNext()) { - Map.Entry entry = iterator.next(); - DocumentKey key = entry.getKey(); - - if (!documentParent.isPrefixOf(key.getPath())) { - break; - } - - if (IndexOffset.fromDocument(entry.getValue()).compareTo(offset) > 0) { - allDocuments.add(entry.getValue()); - } - } + allDocuments.addAll(getAll(collectionParent.append(collectionGroup), offset).values()); } + Map matchingDocuments = new HashMap<>(); Iterator it = allDocuments.iterator(); while (it.hasNext() && matchingDocuments.size() < count) { MutableDocument document = it.next(); @@ -120,41 +103,35 @@ public Map getAll( } @Override - public ImmutableSortedMap getAllDocumentsMatchingQuery( - Query query, IndexOffset offset) { - hardAssert( - !query.isCollectionGroupQuery(), - "CollectionGroup queries should be handled in LocalDocumentsView"); - ImmutableSortedMap result = emptyMutableDocumentMap(); + public Map getAll(ResourcePath collection, IndexOffset offset) { + Map result = new HashMap<>(); // Documents are ordered by key, so we can use a prefix scan to narrow down the documents // we need to match the query against. - ResourcePath queryPath = query.getPath(); - DocumentKey prefix = DocumentKey.fromPath(queryPath.append("")); + DocumentKey prefix = DocumentKey.fromPath(collection.append("")); Iterator> iterator = docs.iteratorFrom(prefix); while (iterator.hasNext()) { Map.Entry entry = iterator.next(); + MutableDocument doc = entry.getValue(); DocumentKey key = entry.getKey(); - if (!queryPath.isPrefixOf(key.getPath())) { + if (!collection.isPrefixOf(key.getPath())) { + // We are now scanning the next collection. Abort. break; } - MutableDocument doc = entry.getValue(); - if (!doc.isFoundDocument()) { + if (key.getPath().length() > collection.length() + 1) { + // Exclude entries from subcollections. continue; } if (IndexOffset.fromDocument(doc).compareTo(offset) <= 0) { + // The document sorts before the offset. continue; } - if (!query.matches(doc)) { - continue; - } - - result = result.insert(doc.getKey(), doc.clone()); + result.put(doc.getKey(), doc.clone()); } return result; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java index dd04ccca613..17040a58119 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java @@ -14,11 +14,10 @@ package com.google.firebase.firestore.local; -import com.google.firebase.database.collection.ImmutableSortedMap; -import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex.IndexOffset; import com.google.firebase.firestore.model.MutableDocument; +import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.model.SnapshotVersion; import java.util.Map; @@ -80,19 +79,13 @@ interface RemoteDocumentCache { Map getAll(String collectionGroup, IndexOffset offset, int count); /** - * Executes a query against the cached Document entries + * Returns the documents from the provided collection. * - *

Implementations may return extra documents if convenient. The results should be re-filtered - * by the consumer before presenting them to the user. - * - *

Cached entries for non-existing documents have no bearing on query results. - * - * @param query The query to match documents against. + * @param collection The collection to read. * @param offset The read time and document key to start scanning at (exclusive). * @return The set of matching documents. */ - ImmutableSortedMap getAllDocumentsMatchingQuery( - Query query, IndexOffset offset); + Map getAll(ResourcePath collection, IndexOffset offset); /** Returns the latest read time of any document in the cache. */ SnapshotVersion getLatestReadTime(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java index ac541f30322..ced9950e402 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java @@ -24,7 +24,6 @@ import androidx.annotation.Nullable; import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedMap; -import com.google.firebase.database.collection.LLRBNode; import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.Bound; import com.google.firebase.firestore.core.FieldFilter; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 2c9a69b0aa1..2dcc3130e46 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -20,9 +20,6 @@ import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; -import com.google.firebase.database.collection.ImmutableSortedMap; -import com.google.firebase.firestore.core.Query; -import com.google.firebase.firestore.model.DocumentCollections; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex.IndexOffset; import com.google.firebase.firestore.model.MutableDocument; @@ -229,23 +226,9 @@ private Map getAll( } @Override - public ImmutableSortedMap getAllDocumentsMatchingQuery( - final Query query, IndexOffset offset) { - hardAssert( - !query.isCollectionGroupQuery(), - "CollectionGroup queries should be handled in LocalDocumentsView"); - - Map allDocuments = - getAll(Collections.singletonList(query.getPath()), offset, Integer.MAX_VALUE); - - ImmutableSortedMap matchingDocuments = - DocumentCollections.emptyMutableDocumentMap(); - for (MutableDocument document : allDocuments.values()) { - if (document.isFoundDocument() && query.matches(document)) { - matchingDocuments = matchingDocuments.insert(document.getKey(), document); - } - } - return matchingDocuments; + public Map getAll( + final ResourcePath collection, IndexOffset offset) { + return getAll(Collections.singletonList(collection), offset, Integer.MAX_VALUE); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index ae10014ebca..93f8651829c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -49,7 +49,7 @@ class SQLiteSchema { * The version of the schema. Increase this by one for each migration added to runMigrations * below. */ - static final int VERSION = 13; + static final int VERSION = 14; static final int OVERLAY_SUPPORT_VERSION = VERSION + 1; @@ -178,6 +178,10 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { ensurePathLength(); } + if (fromVersion < 14 && toVersion >= 14) { + ensureReadTime(); + } + /* * Adding a new migration? READ THIS FIRST! * @@ -658,6 +662,12 @@ private void ensurePathLength() { } while (resultsRemaining[0]); } + /** Initialize the remote_document's read_time column with 0 values if they are not set. */ + private void ensureReadTime() { + db.execSQL( + "UPDATE remote_documents SET read_time_seconds = 0, read_time_nanos = 0 WHERE read_time_seconds IS NULL"); + } + private void createBundleCache() { ifTablesDontExist( new String[] {"bundles", "named_queries"}, diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/MutableDocument.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/MutableDocument.java index e359c992578..0d55286d788 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/MutableDocument.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/MutableDocument.java @@ -62,22 +62,25 @@ private enum DocumentState { private final DocumentKey key; private DocumentType documentType; private SnapshotVersion version; - private SnapshotVersion readTime = SnapshotVersion.NONE; + private SnapshotVersion readTime; private ObjectValue value; private DocumentState documentState; private MutableDocument(DocumentKey key) { this.key = key; + this.readTime = SnapshotVersion.NONE; } private MutableDocument( DocumentKey key, DocumentType documentType, SnapshotVersion version, + SnapshotVersion readTime, ObjectValue value, DocumentState documentState) { this.key = key; this.version = version; + this.readTime = readTime; this.documentType = documentType; this.documentState = documentState; this.value = value; @@ -92,6 +95,7 @@ public static MutableDocument newInvalidDocument(DocumentKey documentKey) { documentKey, DocumentType.INVALID, SnapshotVersion.NONE, + SnapshotVersion.NONE, new ObjectValue(), DocumentState.SYNCED); } @@ -226,7 +230,7 @@ public boolean isUnknownDocument() { @Override @NonNull public MutableDocument clone() { - return new MutableDocument(key, documentType, version, value.clone(), documentState); + return new MutableDocument(key, documentType, version, readTime, value.clone(), documentState); } @Override @@ -238,6 +242,8 @@ public boolean equals(Object o) { if (!key.equals(document.key)) return false; if (!version.equals(document.version)) return false; + // TODO(mrschmidt): Include readTime (requires a lot of test updates) + // if (!readTime.equals(document.readTime)) return false; if (!documentType.equals(document.documentType)) return false; if (!documentState.equals(document.documentState)) return false; return value.equals(document.value); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java index 3ed87d137ca..4be8f2b3b99 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java @@ -23,6 +23,7 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex.IndexOffset; import com.google.firebase.firestore.model.MutableDocument; +import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.MutationBatch; @@ -84,9 +85,9 @@ QueryEngine getSubject() { /** * Returns the number of documents returned by the RemoteDocumentCache's - * `getDocumentsMatchingQuery()` API (since the last call to `resetCounts()`) + * `getAll()` API (since the last call to `resetCounts()`) */ - int getDocumentsReadByQuery() { + int getDocumentsReadByColllection() { return documentsReadByQuery[0]; } @@ -102,7 +103,7 @@ int getDocumentsReadByKey() { * Returns the number of mutations returned by the MutationQueue's * `getAllMutationBatchesAffectingQuery()` API (since the last call to `resetCounts()`) */ - int getMutationsReadByQuery() { + int getMutationsReadByCollection() { return mutationsReadByQuery[0]; } @@ -158,10 +159,8 @@ public Map getAll( } @Override - public ImmutableSortedMap getAllDocumentsMatchingQuery( - Query query, IndexOffset offset) { - ImmutableSortedMap result = - subject.getAllDocumentsMatchingQuery(query, offset); + public Map getAll(ResourcePath collection, IndexOffset offset) { + Map result = subject.getAll(collection, offset); documentsReadByQuery[0] += result.size(); return result; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index bd891da0924..223c18409e9 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -317,8 +317,9 @@ private void assertHasNamedQuery(NamedQuery expectedNamedQuery) { * Asserts the expected numbers of mutations read by the MutationQueue since the last call to * `resetPersistenceStats()`. */ - private void assertMutationsRead(int byKey, int byQuery) { - assertEquals("Mutations read (by query)", byQuery, queryEngine.getMutationsReadByQuery()); + private void assertMutationsRead(int byKey, int byCollection) { + assertEquals( + "Mutations read (by collection)", byCollection, queryEngine.getMutationsReadByCollection()); assertEquals("Mutations read (by key)", byKey, queryEngine.getMutationsReadByKey()); } @@ -326,9 +327,11 @@ private void assertMutationsRead(int byKey, int byQuery) { * Asserts the expected numbers of documents read by the RemoteDocumentCache since the last call * to `resetPersistenceStats()`. */ - private void assertRemoteDocumentsRead(int byKey, int byQuery) { + private void assertRemoteDocumentsRead(int byKey, int byCollection) { assertEquals( - "Remote documents read (by query)", byQuery, queryEngine.getDocumentsReadByQuery()); + "Remote documents read (by collection)", + byCollection, + queryEngine.getDocumentsReadByColllection()); assertEquals("Remote documents read (by key)", byKey, queryEngine.getDocumentsReadByKey()); } @@ -973,9 +976,9 @@ public void testReadsAllDocumentsForInitialCollectionQueries() { localStore.executeQuery(query, /* usePreviousResults= */ true); - assertRemoteDocumentsRead(/* byKey= */ 0, /* byQuery= */ 2); + assertRemoteDocumentsRead(/* byKey= */ 0, /* byCollection= */ 2); if (!Persistence.OVERLAY_SUPPORT_ENABLED) { - assertMutationsRead(/* byKey= */ 0, /* byQuery= */ 1); + assertMutationsRead(/* byKey= */ 0, /* byCollection= */ 1); } } @@ -1101,7 +1104,7 @@ public void testUsesTargetMappingToExecuteQueries() { // Execute the query, but note that we read all existing documents from the RemoteDocumentCache // since we do not yet have target mapping. executeQuery(query); - assertRemoteDocumentsRead(/* byKey= */ 0, /* byQuery= */ 2); + assertRemoteDocumentsRead(/* byKey= */ 0, /* byCollection= */ 3); // Issue a RemoteEvent to persist the target mapping. applyRemoteEvent( @@ -1115,7 +1118,7 @@ public void testUsesTargetMappingToExecuteQueries() { // Execute the query again, this time verifying that we only read the two documents that match // the query. executeQuery(query); - assertRemoteDocumentsRead(/* byKey= */ 2, /* byQuery= */ 0); + assertRemoteDocumentsRead(/* byKey= */ 2, /* byCollection= */ 0); assertQueryReturned("foo/a", "foo/b"); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java index dee6fe13ecc..87d6fca358c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java @@ -21,18 +21,16 @@ import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.path; -import static com.google.firebase.firestore.testutil.TestUtil.values; import static com.google.firebase.firestore.testutil.TestUtil.version; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; -import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.auth.User; -import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex.IndexOffset; import com.google.firebase.firestore.model.MutableDocument; +import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.model.SnapshotVersion; import java.util.ArrayList; import java.util.Arrays; @@ -171,79 +169,78 @@ public void testRemoveNonExistentDocument() { } @Test - public void testDocumentsMatchingQuery() { - // TODO: This just verifies that we do a prefix scan against the - // query path. We'll need more tests once we add index support. + public void testGetAllFromCollection() { addTestDocumentAtPath("a/1"); addTestDocumentAtPath("b/1"); addTestDocumentAtPath("b/2"); addTestDocumentAtPath("c/1"); - Query query = Query.atPath(path("b")); - ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query, IndexOffset.NONE); - assertThat(values(results)).containsExactly(doc("b/1", 42, DOC_DATA), doc("b/2", 42, DOC_DATA)); + ResourcePath collection = path("b"); + Map results = + remoteDocumentCache.getAll(collection, IndexOffset.NONE); + assertThat(results.values()) + .containsExactly(doc("b/1", 42, DOC_DATA), doc("b/2", 42, DOC_DATA)); } @Test - public void testDocumentsMatchingQueryExcludesSubcollections() { + public void testGetAllFromExcludesSubcollections() { addTestDocumentAtPath("a/1"); addTestDocumentAtPath("a/1/b/1"); addTestDocumentAtPath("a/2"); - Query query = Query.atPath(path("a")); - ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query, IndexOffset.NONE); - assertThat(values(results)).containsExactly(doc("a/1", 42, DOC_DATA), doc("a/2", 42, DOC_DATA)); + ResourcePath collection = path("a"); + Map results = + remoteDocumentCache.getAll(collection, IndexOffset.NONE); + assertThat(results.values()) + .containsExactly(doc("a/1", 42, DOC_DATA), doc("a/2", 42, DOC_DATA)); } @Test - public void testDocumentsMatchingQuerySinceReadTimeAndSeconds() { + public void testGetAllFromSinceReadTimeAndSeconds() { addTestDocumentAtPath("b/old", /* updateTime= */ 1, /* readTime= */ 11); addTestDocumentAtPath("b/current", /* updateTime= */ 2, /* readTime= = */ 12); addTestDocumentAtPath("b/new", /* updateTime= */ 3, /* readTime= = */ 13); - Query query = Query.atPath(path("b")); - ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query, IndexOffset.create(version(12))); - assertThat(values(results)).containsExactly(doc("b/new", 3, DOC_DATA)); + ResourcePath collection = path("b"); + Map results = + remoteDocumentCache.getAll(collection, IndexOffset.create(version(12))); + assertThat(results.values()).containsExactly(doc("b/new", 3, DOC_DATA)); } @Test - public void testDocumentsMatchingQuerySinceReadTimeAndNanoseconds() { + public void testGetAllFromSinceReadTimeAndNanoseconds() { add(doc("b/old", 1, DOC_DATA), version(1, 1)); add(doc("b/current", 1, DOC_DATA), version(1, 2)); add(doc("b/new", 1, DOC_DATA), version(1, 3)); - Query query = Query.atPath(path("b")); - ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query, IndexOffset.create(version(1, 2))); - assertThat(values(results)).containsExactly(doc("b/new", 1, DOC_DATA)); + ResourcePath collection = path("b"); + Map results = + remoteDocumentCache.getAll(collection, IndexOffset.create(version(1, 2))); + assertThat(results.values()).containsExactly(doc("b/new", 1, DOC_DATA)); } @Test - public void testDocumentsMatchingQuerySinceReadTimeAndDocumentKey() { + public void testGetAllFromSinceReadTimeAndDocumentKey() { addTestDocumentAtPath("b/a", /* updateTime= */ 1, /* readTime= */ 11); addTestDocumentAtPath("b/b", /* updateTime= */ 2, /* readTime= = */ 11); addTestDocumentAtPath("b/c", /* updateTime= */ 3, /* readTime= = */ 11); addTestDocumentAtPath("b/d", /* updateTime= */ 4, /* readTime= = */ 12); - Query query = Query.atPath(path("b")); - ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery( - query, IndexOffset.create(version(11), key("b/b"))); - assertThat(values(results)).containsExactly(doc("b/c", 3, DOC_DATA), doc("b/d", 4, DOC_DATA)); + ResourcePath collection = path("b"); + Map results = + remoteDocumentCache.getAll(collection, IndexOffset.create(version(11), key("b/b"))); + assertThat(results.values()).containsExactly(doc("b/c", 3, DOC_DATA), doc("b/d", 4, DOC_DATA)); } @Test - public void testDocumentsMatchingUsesReadTimeNotUpdateTime() { + public void testGetAllFromUsesReadTimeNotUpdateTime() { addTestDocumentAtPath("b/old", /* updateTime= */ 1, /* readTime= */ 2); addTestDocumentAtPath("b/new", /* updateTime= */ 2, /* readTime= */ 1); - Query query = Query.atPath(path("b")); - ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query, IndexOffset.create(version(1))); - assertThat(values(results)).containsExactly(doc("b/old", 1, DOC_DATA)); + ResourcePath collection = path("b"); + Map results = + remoteDocumentCache.getAll(collection, IndexOffset.create(version(1))); + assertThat(results.values()).containsExactly(doc("b/old", 1, DOC_DATA)); } @Test diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index 810d3649aa8..a7e3b7f97e4 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -14,11 +14,11 @@ package com.google.firebase.firestore.local; +import static com.google.common.truth.Truth.assertThat; import static com.google.firebase.firestore.local.EncodedPath.decodeResourcePath; import static com.google.firebase.firestore.local.EncodedPath.encode; import static com.google.firebase.firestore.local.SQLiteSchema.VERSION; import static com.google.firebase.firestore.testutil.TestUtil.filter; -import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.path; import static com.google.firebase.firestore.testutil.TestUtil.query; @@ -34,7 +34,6 @@ import android.database.Cursor; import android.database.sqlite.SQLiteDatabase; import androidx.test.core.app.ApplicationProvider; -import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DatabaseId; import com.google.firebase.firestore.model.DocumentKey; @@ -45,11 +44,13 @@ import com.google.firebase.firestore.proto.Target; import com.google.firebase.firestore.proto.WriteBatch; import com.google.firebase.firestore.remote.RemoteSerializer; +import com.google.firebase.firestore.testutil.TestUtil; import com.google.firestore.v1.Document; import com.google.firestore.v1.Write; import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.Timestamp; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -426,7 +427,7 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() { new Object[] {encode(path("coll/existing")), createDummyDocument("coll/existing")}); // Run the index-free migration. - schema.runSchemaUpgrades(8, 10); + schema.runSchemaUpgrades(8, 14); db.execSQL( "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)", new Object[] {encode(path("coll/old")), 0, 1000, createDummyDocument("coll/old")}); @@ -443,15 +444,13 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() { // Verify that queries with SnapshotVersion.NONE return all results, regardless of whether the // read time has been set. - ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query("coll"), IndexOffset.NONE); + Map results = + remoteDocumentCache.getAll(path("coll"), IndexOffset.NONE); assertResultsContain(results, "coll/existing", "coll/old", "coll/current", "coll/new"); // Queries that filter by read time only return documents that were written after the index-free // migration. - results = - remoteDocumentCache.getAllDocumentsMatchingQuery( - query("coll"), IndexOffset.create(version(2))); + results = remoteDocumentCache.getAll(path("coll"), IndexOffset.create(version(2))); assertResultsContain(results, "coll/new"); } @@ -733,11 +732,9 @@ private Target createDummyQueryTargetWithLimboFreeVersion(int targetId) { } private void assertResultsContain( - ImmutableSortedMap actualResults, String... docs) { - for (String doc : docs) { - assertTrue("Expected result for " + doc, actualResults.containsKey(key(doc))); - } - assertEquals("Results contain unexpected entries", docs.length, actualResults.size()); + Map actualResults, String... docs) { + assertThat(actualResults.keySet()) + .containsExactly(Arrays.stream(docs).map(TestUtil::key).toArray()); } private void assertTableExists(String tableName) { From acc43e939bf078b8bd5b6433f98de7bbdfce0ed8 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 2 Dec 2021 11:43:25 -0700 Subject: [PATCH 13/20] Format --- .../com/google/firebase/firestore/local/SQLiteIndexManager.java | 1 - 1 file changed, 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java index ac541f30322..ced9950e402 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java @@ -24,7 +24,6 @@ import androidx.annotation.Nullable; import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedMap; -import com.google.firebase.database.collection.LLRBNode; import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.Bound; import com.google.firebase.firestore.core.FieldFilter; From c4963a356a5ce337c5b30386ca58d6bedfd00d08 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 2 Dec 2021 11:54:13 -0700 Subject: [PATCH 14/20] Fill read_time --- .../firebase/firestore/local/SQLiteSchema.java | 12 +++++++++++- .../firebase/firestore/local/SQLiteSchemaTest.java | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index ae10014ebca..93f8651829c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -49,7 +49,7 @@ class SQLiteSchema { * The version of the schema. Increase this by one for each migration added to runMigrations * below. */ - static final int VERSION = 13; + static final int VERSION = 14; static final int OVERLAY_SUPPORT_VERSION = VERSION + 1; @@ -178,6 +178,10 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { ensurePathLength(); } + if (fromVersion < 14 && toVersion >= 14) { + ensureReadTime(); + } + /* * Adding a new migration? READ THIS FIRST! * @@ -658,6 +662,12 @@ private void ensurePathLength() { } while (resultsRemaining[0]); } + /** Initialize the remote_document's read_time column with 0 values if they are not set. */ + private void ensureReadTime() { + db.execSQL( + "UPDATE remote_documents SET read_time_seconds = 0, read_time_nanos = 0 WHERE read_time_seconds IS NULL"); + } + private void createBundleCache() { ifTablesDontExist( new String[] {"bundles", "named_queries"}, diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index 810d3649aa8..2dbd968ff2e 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -426,7 +426,7 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() { new Object[] {encode(path("coll/existing")), createDummyDocument("coll/existing")}); // Run the index-free migration. - schema.runSchemaUpgrades(8, 10); + schema.runSchemaUpgrades(8, 14); db.execSQL( "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)", new Object[] {encode(path("coll/old")), 0, 1000, createDummyDocument("coll/old")}); From 0f56227b116e474e1b724ffa1719ab49db475b6c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 3 Dec 2021 09:16:35 -0700 Subject: [PATCH 15/20] Review --- .../firebase/firestore/local/SQLiteRemoteDocumentCache.java | 5 +++-- .../com/google/firebase/firestore/local/SQLiteSchema.java | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 8fca8e81088..85e3af99239 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -140,7 +140,8 @@ public ImmutableSortedMap getAllDocumentsMatchingQ "SELECT contents, read_time_seconds, read_time_nanos " + "FROM remote_documents WHERE path >= ? AND path < ? AND path_length = ?"); - Object[] bindVars = new Object[3 + (FieldIndex.IndexOffset.NONE.equals(offset) ? 0 : 6)]; + boolean hasOffset = !FieldIndex.IndexOffset.NONE.equals(offset); + Object[] bindVars = new Object[3 + (hasOffset ? 6 : 0)]; String prefix = EncodedPath.encode(query.getPath()); @@ -149,7 +150,7 @@ public ImmutableSortedMap getAllDocumentsMatchingQ bindVars[i++] = EncodedPath.prefixSuccessor(prefix); bindVars[i++] = query.getPath().length() + 1; - if (!FieldIndex.IndexOffset.NONE.equals(offset)) { + if (hasOffset) { Timestamp readTime = offset.getReadTime().getTimestamp(); DocumentKey documentKey = offset.getDocumentKey(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index ae10014ebca..46f7823dd94 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -629,7 +629,7 @@ private void rewriteCanonicalIds() { }); } - /** Fill the remote_document's path_length column. */ + /** Populates the remote_document's path_length column. */ private void ensurePathLength() { SQLitePersistence.Query documentsToMigrate = new SQLitePersistence.Query( From f6f3664599c16d368f64849145c9be6d1646f323 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 3 Dec 2021 09:58:22 -0700 Subject: [PATCH 16/20] Review --- .../firestore/local/RemoteDocumentCache.java | 4 ++-- .../local/SQLiteRemoteDocumentCache.java | 3 +-- .../firestore/local/CountingQueryEngine.java | 24 +++++++++---------- .../firestore/local/LocalStoreTestCase.java | 2 +- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java index 17040a58119..f59ac5c0549 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java @@ -74,7 +74,7 @@ interface RemoteDocumentCache { * @param collectionGroup The collection group to scan. * @param offset The offset to start the scan at. * @param count The number of results to return. - * @return A map with next set of documents. + * @return A newly created map with next set of documents. */ Map getAll(String collectionGroup, IndexOffset offset, int count); @@ -83,7 +83,7 @@ interface RemoteDocumentCache { * * @param collection The collection to read. * @param offset The read time and document key to start scanning at (exclusive). - * @return The set of matching documents. + * @return A newly created map with the set of documents in the collection. */ Map getAll(ResourcePath collection, IndexOffset offset); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 2dcc3130e46..4aa63d8807d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -226,8 +226,7 @@ private Map getAll( } @Override - public Map getAll( - final ResourcePath collection, IndexOffset offset) { + public Map getAll(ResourcePath collection, IndexOffset offset) { return getAll(Collections.singletonList(collection), offset, Integer.MAX_VALUE); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java index 4be8f2b3b99..ae8432545a7 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java @@ -38,9 +38,9 @@ class CountingQueryEngine implements QueryEngine { private final QueryEngine queryEngine; - private final int[] mutationsReadByQuery = new int[] {0}; + private final int[] mutationsReadByCollection = new int[] {0}; private final int[] mutationsReadByKey = new int[] {0}; - private final int[] documentsReadByQuery = new int[] {0}; + private final int[] documentsReadByCollection = new int[] {0}; private final int[] documentsReadByKey = new int[] {0}; CountingQueryEngine(QueryEngine queryEngine) { @@ -48,9 +48,9 @@ class CountingQueryEngine implements QueryEngine { } void resetCounts() { - mutationsReadByQuery[0] = 0; + mutationsReadByCollection[0] = 0; mutationsReadByKey[0] = 0; - documentsReadByQuery[0] = 0; + documentsReadByCollection[0] = 0; documentsReadByKey[0] = 0; } @@ -84,11 +84,11 @@ QueryEngine getSubject() { } /** - * Returns the number of documents returned by the RemoteDocumentCache's - * `getAll()` API (since the last call to `resetCounts()`) + * Returns the number of documents returned by the RemoteDocumentCache's `getAll()` API (since the + * last call to `resetCounts()`) */ - int getDocumentsReadByColllection() { - return documentsReadByQuery[0]; + int getDocumentsReadByCollection() { + return documentsReadByCollection[0]; } /** @@ -104,7 +104,7 @@ int getDocumentsReadByKey() { * `getAllMutationBatchesAffectingQuery()` API (since the last call to `resetCounts()`) */ int getMutationsReadByCollection() { - return mutationsReadByQuery[0]; + return mutationsReadByCollection[0]; } /** @@ -154,14 +154,14 @@ public Map getAll(Iterable documentKe public Map getAll( String collectionGroup, IndexOffset offset, int count) { Map result = subject.getAll(collectionGroup, offset, count); - documentsReadByQuery[0] += result.size(); + documentsReadByCollection[0] += result.size(); return result; } @Override public Map getAll(ResourcePath collection, IndexOffset offset) { Map result = subject.getAll(collection, offset); - documentsReadByQuery[0] += result.size(); + documentsReadByCollection[0] += result.size(); return result; } @@ -249,7 +249,7 @@ public List getAllMutationBatchesAffectingDocumentKeys( @Override public List getAllMutationBatchesAffectingQuery(Query query) { List result = subject.getAllMutationBatchesAffectingQuery(query); - mutationsReadByQuery[0] += result.size(); + mutationsReadByCollection[0] += result.size(); return result; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index 223c18409e9..eff58f6d28a 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -331,7 +331,7 @@ private void assertRemoteDocumentsRead(int byKey, int byCollection) { assertEquals( "Remote documents read (by collection)", byCollection, - queryEngine.getDocumentsReadByColllection()); + queryEngine.getDocumentsReadByCollection()); assertEquals("Remote documents read (by key)", byKey, queryEngine.getDocumentsReadByKey()); } From 473e8f4f67fd520826390547983246dc1619596f Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 7 Dec 2021 11:29:41 -0700 Subject: [PATCH 17/20] Format --- .../firebase/firestore/local/SQLiteRemoteDocumentCache.java | 4 ++-- .../com/google/firebase/firestore/local/SQLiteSchema.java | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index ea84901551a..f9fceee7fb4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -217,8 +217,8 @@ private Map getAll( () -> { MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds[0], readTimeNanos[0]); - synchronized (SQLiteRemoteDocumentCache.this) { - results[0].put(document.getKey(), document); + synchronized (SQLiteRemoteDocumentCache.this) { + results[0].put(document.getKey(), document); } }); }); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index 8006fa7d6d1..8bf3ae6d6be 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -51,7 +51,6 @@ class SQLiteSchema { */ static final int VERSION = 15; - // TODO(indexing): Remove this constant and increment VERSION to enable indexing support static final int INDEXING_SUPPORT_VERSION = VERSION + 1; From 4719d115bee6c1f046931e11d7bdcbd2a13d74d9 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 8 Dec 2021 10:15:58 -0700 Subject: [PATCH 18/20] Review --- .../local/MemoryRemoteDocumentCache.java | 23 ++++++++----------- .../firestore/local/RemoteDocumentCache.java | 9 +++----- .../local/SQLiteRemoteDocumentCache.java | 9 ++++---- .../firebase/firestore/model/FieldIndex.java | 2 ++ .../google/firebase/firestore/util/Util.java | 17 ++++++++++++++ .../firestore/local/CountingQueryEngine.java | 4 ++-- .../firebase/firestore/util/UtilTest.java | 15 ++++++++++++ 7 files changed, 53 insertions(+), 26 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java index 1ebfba4c3db..fb47cd8c8c8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java @@ -16,6 +16,7 @@ import static com.google.firebase.firestore.model.DocumentCollections.emptyMutableDocumentMap; import static com.google.firebase.firestore.util.Assert.hardAssert; +import static com.google.firebase.firestore.util.Util.trimMap; import androidx.annotation.NonNull; import com.google.firebase.database.collection.ImmutableSortedMap; @@ -86,10 +87,11 @@ public Map getAll(Iterable keys) { @Override public Map getAll( - String collectionGroup, IndexOffset offset, int count) { + String collectionGroup, IndexOffset offset, int limit) { + // Note: This method is pretty inefficient, but t is not called since the method is only used + // during index backfill, which is not supported by memory persistence. + List collectionParents = indexManager.getCollectionParents(collectionGroup); - Set allDocuments = - new TreeSet<>((l, r) -> IndexOffset.fromDocument(l).compareTo(IndexOffset.fromDocument(r))); Map matchingDocuments = new HashMap<>(); for (ResourcePath collectionParent : collectionParents) { @@ -99,24 +101,17 @@ public Map getAll( while (iterator.hasNext()) { Map.Entry entry = iterator.next(); DocumentKey key = entry.getKey(); - + MutableDocument document = entry.getValue(); if (!documentParent.isPrefixOf(key.getPath())) { break; } - - if (IndexOffset.fromDocument(entry.getValue()).compareTo(offset) > 0) { - allDocuments.add(entry.getValue()); + if (IndexOffset.fromDocument(document).compareTo(offset) > 0) { + matchingDocuments.put(key, document); } } } - Iterator it = allDocuments.iterator(); - while (it.hasNext() && matchingDocuments.size() < count) { - MutableDocument document = it.next(); - matchingDocuments.put(document.getKey(), document); - } - - return matchingDocuments; + return trimMap(matchingDocuments, limit, IndexOffset.DOCUMENT_COMPARATOR); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java index dd04ccca613..87283229ee2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java @@ -66,18 +66,15 @@ interface RemoteDocumentCache { Map getAll(Iterable documentKeys); /** - * Looks up the next {@code count} documents for a collection group based on the provided offset. + * Looks up the next {@code limit} documents for a collection group based on the provided offset. * The ordering is based on the document's read time and key. * - *

This method may return more results than requested if a collection group scan scans more - * than 100 collections. - * * @param collectionGroup The collection group to scan. * @param offset The offset to start the scan at. - * @param count The number of results to return. + * @param limit The maximum number of results to return. * @return A map with next set of documents. */ - Map getAll(String collectionGroup, IndexOffset offset, int count); + Map getAll(String collectionGroup, IndexOffset offset, int limit); /** * Executes a query against the cached Document entries diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index f9fceee7fb4..70cef66df63 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -17,6 +17,7 @@ import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; import static com.google.firebase.firestore.util.Util.repeatSequence; +import static com.google.firebase.firestore.util.Util.trimMap; import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; @@ -136,7 +137,7 @@ public Map getAll(Iterable documentKe @Override public Map getAll( - String collectionGroup, IndexOffset offset, int count) { + String collectionGroup, IndexOffset offset, int limit) { List collectionParents = indexManager.getCollectionParents(collectionGroup); List collections = new ArrayList<>(collectionParents.size()); for (ResourcePath collectionParent : collectionParents) { @@ -146,7 +147,7 @@ public Map getAll( if (collections.isEmpty()) { return Collections.emptyMap(); } else if (BINDS_PER_STATEMENT * collections.size() < SQLitePersistence.MAX_ARGS) { - return getAll(collections, offset, count); + return getAll(collections, offset, limit); } else { // We need to fan out our collection scan since SQLite only supports 999 binds per statement. Map results = new HashMap<>(); @@ -154,9 +155,9 @@ public Map getAll( for (int i = 0; i < collections.size(); i += pageSize) { results.putAll( getAll( - collections.subList(i, Math.min(collections.size(), i + pageSize)), offset, count)); + collections.subList(i, Math.min(collections.size(), i + pageSize)), offset, limit)); } - return results; + return trimMap(results, limit, IndexOffset.DOCUMENT_COMPARATOR); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java index e3d074cf653..362e0496899 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java @@ -116,6 +116,8 @@ public static IndexState create( /** Stores the latest read time and document that were processed for an index. */ @AutoValue public abstract static class IndexOffset implements Comparable { + public static final Comparator DOCUMENT_COMPARATOR = (l,r)->IndexOffset.fromDocument(l).compareTo(IndexOffset.fromDocument(r)); + public static final IndexOffset NONE = create(SnapshotVersion.NONE, DocumentKey.empty()); /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java index 289e6656840..1a2f3c5fc20 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java @@ -31,8 +31,10 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Random; import java.util.SortedSet; @@ -366,4 +368,19 @@ private static void diffCollections( private static T advanceIterator(Iterator it) { return it.hasNext() ? it.next() : null; } + + /** Returns a map with the first {#code count} elements of {#code data} when sorted by comp. */ + public static Map trimMap(Map data, int count, Comparator comp) { + if (data.size() <= count) { + return data; + } else { + List> sortedVlaues = new ArrayList<>(data.entrySet()); + Collections.sort(sortedVlaues, (l, r) -> comp.compare(l.getValue(), r.getValue())); + Map result = new HashMap<>(); + for (int i = 0; i < count; ++i) { + result.put(sortedVlaues.get(i).getKey(), sortedVlaues.get(i).getValue()); + } + return result; + } + } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java index 3ed87d137ca..a434d2af4ca 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java @@ -151,8 +151,8 @@ public Map getAll(Iterable documentKe @Override public Map getAll( - String collectionGroup, IndexOffset offset, int count) { - Map result = subject.getAll(collectionGroup, offset, count); + String collectionGroup, IndexOffset offset, int limit) { + Map result = subject.getAll(collectionGroup, offset, limit); documentsReadByQuery[0] += result.size(); return result; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java index a28a61ed5b8..4088ff5f945 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java @@ -15,6 +15,7 @@ package com.google.firebase.firestore.util; import static com.google.common.truth.Truth.assertThat; +import static com.google.firebase.firestore.util.Util.trimMap; import static org.junit.Assert.assertEquals; import com.google.firebase.firestore.testutil.TestUtil; @@ -22,7 +23,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; + import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -69,6 +73,17 @@ public void testDiffCollectionsWithEmptyLists() { validateDiffCollection(Collections.emptyList(), Collections.emptyList()); } + + @Test + public void testTrimMap() { + Map data = new HashMap<>(); + data.put(1, 1); + data.put(3, 3); + data.put(2, 2); + data = trimMap(data, 2, Integer::compare); + assertThat(data).containsExactly(1, 1, 2,2); + } + private void validateDiffCollection(List before, List after) { List result = new ArrayList<>(before); Util.diffCollections(before, after, String::compareTo, result::add, result::remove); From a3a03b190d0da4cec8fefe22a45df361e42cdac5 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 8 Dec 2021 12:55:11 -0700 Subject: [PATCH 19/20] Review --- .../local/MemoryRemoteDocumentCache.java | 30 +--------- .../local/SQLiteRemoteDocumentCache.java | 4 +- .../firebase/firestore/model/FieldIndex.java | 3 +- .../google/firebase/firestore/util/Util.java | 8 +-- .../local/RemoteDocumentCacheTestCase.java | 57 +----------------- .../local/SQLiteRemoteDocumentCacheTest.java | 60 +++++++++++++++++++ .../firebase/firestore/util/UtilTest.java | 10 ++-- 7 files changed, 77 insertions(+), 95 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java index fb47cd8c8c8..acd84633979 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java @@ -16,7 +16,6 @@ import static com.google.firebase.firestore.model.DocumentCollections.emptyMutableDocumentMap; import static com.google.firebase.firestore.util.Assert.hardAssert; -import static com.google.firebase.firestore.util.Util.trimMap; import androidx.annotation.NonNull; import com.google.firebase.database.collection.ImmutableSortedMap; @@ -28,10 +27,7 @@ import com.google.firebase.firestore.model.SnapshotVersion; import java.util.HashMap; import java.util.Iterator; -import java.util.List; import java.util.Map; -import java.util.Set; -import java.util.TreeSet; /** In-memory cache of remote documents. */ final class MemoryRemoteDocumentCache implements RemoteDocumentCache { @@ -88,30 +84,8 @@ public Map getAll(Iterable keys) { @Override public Map getAll( String collectionGroup, IndexOffset offset, int limit) { - // Note: This method is pretty inefficient, but t is not called since the method is only used - // during index backfill, which is not supported by memory persistence. - - List collectionParents = indexManager.getCollectionParents(collectionGroup); - Map matchingDocuments = new HashMap<>(); - - for (ResourcePath collectionParent : collectionParents) { - ResourcePath documentParent = collectionParent.append(collectionGroup); - Iterator> iterator = - docs.iteratorFrom(DocumentKey.fromPath(documentParent.append(""))); - while (iterator.hasNext()) { - Map.Entry entry = iterator.next(); - DocumentKey key = entry.getKey(); - MutableDocument document = entry.getValue(); - if (!documentParent.isPrefixOf(key.getPath())) { - break; - } - if (IndexOffset.fromDocument(document).compareTo(offset) > 0) { - matchingDocuments.put(key, document); - } - } - } - - return trimMap(matchingDocuments, limit, IndexOffset.DOCUMENT_COMPARATOR); + // This method should only be called from the IndexBackfiller if SQLite is enabled. + throw new UnsupportedOperationException("getAll(String, IndexOffset, int) is not supported."); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 70cef66df63..fb45c355c46 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -16,8 +16,8 @@ import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; +import static com.google.firebase.firestore.util.Util.firstNEntries; import static com.google.firebase.firestore.util.Util.repeatSequence; -import static com.google.firebase.firestore.util.Util.trimMap; import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; @@ -157,7 +157,7 @@ public Map getAll( getAll( collections.subList(i, Math.min(collections.size(), i + pageSize)), offset, limit)); } - return trimMap(results, limit, IndexOffset.DOCUMENT_COMPARATOR); + return firstNEntries(results, limit, IndexOffset.DOCUMENT_COMPARATOR); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java index 362e0496899..40e465a327b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java @@ -116,7 +116,8 @@ public static IndexState create( /** Stores the latest read time and document that were processed for an index. */ @AutoValue public abstract static class IndexOffset implements Comparable { - public static final Comparator DOCUMENT_COMPARATOR = (l,r)->IndexOffset.fromDocument(l).compareTo(IndexOffset.fromDocument(r)); + public static final Comparator DOCUMENT_COMPARATOR = + (l, r) -> IndexOffset.fromDocument(l).compareTo(IndexOffset.fromDocument(r)); public static final IndexOffset NONE = create(SnapshotVersion.NONE, DocumentKey.empty()); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java index 1a2f3c5fc20..8819511f19c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java @@ -369,15 +369,15 @@ private static T advanceIterator(Iterator it) { return it.hasNext() ? it.next() : null; } - /** Returns a map with the first {#code count} elements of {#code data} when sorted by comp. */ - public static Map trimMap(Map data, int count, Comparator comp) { - if (data.size() <= count) { + /** Returns a map with the first {#code n} elements of {#code data} when sorted by comp. */ + public static Map firstNEntries(Map data, int n, Comparator comp) { + if (data.size() <= n) { return data; } else { List> sortedVlaues = new ArrayList<>(data.entrySet()); Collections.sort(sortedVlaues, (l, r) -> comp.compare(l.getValue(), r.getValue())); Map result = new HashMap<>(); - for (int i = 0; i < count; ++i) { + for (int i = 0; i < n; ++i) { result.put(sortedVlaues.get(i).getKey(), sortedVlaues.get(i).getValue()); } return result; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java index dee6fe13ecc..18eda812756 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java @@ -57,7 +57,7 @@ abstract class RemoteDocumentCacheTestCase { private final Map DOC_DATA = map("data", 2); private Persistence persistence; - private RemoteDocumentCache remoteDocumentCache; + protected RemoteDocumentCache remoteDocumentCache; @Before public void setUp() { @@ -246,57 +246,6 @@ public void testDocumentsMatchingUsesReadTimeNotUpdateTime() { assertThat(values(results)).containsExactly(doc("b/old", 1, DOC_DATA)); } - @Test - public void testNextDocumentsFromCollectionGroup() { - addTestDocumentAtPath("a/1"); - addTestDocumentAtPath("a/2"); - addTestDocumentAtPath("b/3"); - - Map results = - remoteDocumentCache.getAll("a", IndexOffset.NONE, Integer.MAX_VALUE); - assertThat(results.keySet()).containsExactly(key("a/1"), key("a/2")); - } - - @Test - public void testNextDocumentsFromCollectionGroupWithLimit() { - addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11); - addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 1, /* readTime= */ 12); - addTestDocumentAtPath("a/3", /* updateTime= */ 1, /* readTime= */ 13); - - Map results = - remoteDocumentCache.getAll("a", IndexOffset.NONE, 2); - assertThat(results.keySet()).containsExactly(key("a/1"), key("b/2/a/2")); - } - - @Test - public void testNextDocumentsFromCollectionGroupWithOffset() { - addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11); - addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 2, /* readTime= = */ 12); - addTestDocumentAtPath("a/3", /* updateTime= */ 3, /* readTime= = */ 13); - - Map results = - remoteDocumentCache.getAll("a", IndexOffset.create(version(11)), 2); - assertThat(results.keySet()).containsExactly(key("b/2/a/2"), key("a/3")); - } - - @Test - public void testNextDocumentsForNonExistingCollectionGroup() { - Map results = - remoteDocumentCache.getAll("a", IndexOffset.create(version(11)), 2); - assertThat(results).isEmpty(); - } - - @Test - public void testNextDocumentsForLargeCollectionGroup() { - int size = 999 / SQLiteRemoteDocumentCache.BINDS_PER_STATEMENT + 1; - for (int i = 0; i < size; ++i) { - addTestDocumentAtPath("a/" + i + "/b/doc"); - } - Map results = - remoteDocumentCache.getAll("b", IndexOffset.NONE, size); - assertThat(results).hasSize(size); - } - @Test public void testLatestReadTime() { SnapshotVersion latestReadTime = remoteDocumentCache.getLatestReadTime(); @@ -315,11 +264,11 @@ public void testLatestReadTime() { assertEquals(version(3), latestReadTime); } - private MutableDocument addTestDocumentAtPath(String path) { + protected MutableDocument addTestDocumentAtPath(String path) { return addTestDocumentAtPath(path, 42, 42); } - private MutableDocument addTestDocumentAtPath(String path, int updateTime, int readTime) { + protected MutableDocument addTestDocumentAtPath(String path, int updateTime, int readTime) { MutableDocument doc = doc(path, updateTime, map("data", 2)); add(doc, version(readTime)); return doc; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCacheTest.java index fa6c473d444..a5f234e0114 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCacheTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCacheTest.java @@ -14,6 +14,15 @@ package com.google.firebase.firestore.local; +import static com.google.common.truth.Truth.assertThat; +import static com.google.firebase.firestore.testutil.TestUtil.key; +import static com.google.firebase.firestore.testutil.TestUtil.version; + +import com.google.firebase.firestore.model.DocumentKey; +import com.google.firebase.firestore.model.FieldIndex; +import com.google.firebase.firestore.model.MutableDocument; +import java.util.Map; +import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; import org.robolectric.annotation.Config; @@ -26,4 +35,55 @@ public final class SQLiteRemoteDocumentCacheTest extends RemoteDocumentCacheTest Persistence getPersistence() { return PersistenceTestHelpers.createSQLitePersistence(); } + + @Test + public void testNextDocumentsFromCollectionGroup() { + addTestDocumentAtPath("a/1"); + addTestDocumentAtPath("a/2"); + addTestDocumentAtPath("b/3"); + + Map results = + remoteDocumentCache.getAll("a", FieldIndex.IndexOffset.NONE, Integer.MAX_VALUE); + assertThat(results.keySet()).containsExactly(key("a/1"), key("a/2")); + } + + @Test + public void testNextDocumentsFromCollectionGroupWithLimit() { + addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11); + addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 1, /* readTime= */ 12); + addTestDocumentAtPath("a/3", /* updateTime= */ 1, /* readTime= */ 13); + + Map results = + remoteDocumentCache.getAll("a", FieldIndex.IndexOffset.NONE, 2); + assertThat(results.keySet()).containsExactly(key("a/1"), key("b/2/a/2")); + } + + @Test + public void testNextDocumentsFromCollectionGroupWithOffset() { + addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11); + addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 2, /* readTime= = */ 12); + addTestDocumentAtPath("a/3", /* updateTime= */ 3, /* readTime= = */ 13); + + Map results = + remoteDocumentCache.getAll("a", FieldIndex.IndexOffset.create(version(11)), 2); + assertThat(results.keySet()).containsExactly(key("b/2/a/2"), key("a/3")); + } + + @Test + public void testNextDocumentsFromNonExistingCollectionGroup() { + Map results = + remoteDocumentCache.getAll("a", FieldIndex.IndexOffset.create(version(11)), 2); + assertThat(results).isEmpty(); + } + + @Test + public void testNextDocumentsForLargeCollectionGroup() { + int size = 999 / SQLiteRemoteDocumentCache.BINDS_PER_STATEMENT + 1; + for (int i = 0; i < size; ++i) { + addTestDocumentAtPath("a/" + i + "/b/doc"); + } + Map results = + remoteDocumentCache.getAll("b", FieldIndex.IndexOffset.NONE, size); + assertThat(results).hasSize(size); + } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java index 4088ff5f945..6ff424ef994 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java @@ -15,7 +15,7 @@ package com.google.firebase.firestore.util; import static com.google.common.truth.Truth.assertThat; -import static com.google.firebase.firestore.util.Util.trimMap; +import static com.google.firebase.firestore.util.Util.firstNEntries; import static org.junit.Assert.assertEquals; import com.google.firebase.firestore.testutil.TestUtil; @@ -26,7 +26,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; - import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -73,15 +72,14 @@ public void testDiffCollectionsWithEmptyLists() { validateDiffCollection(Collections.emptyList(), Collections.emptyList()); } - @Test - public void testTrimMap() { + public void testFirstNEntries() { Map data = new HashMap<>(); data.put(1, 1); data.put(3, 3); data.put(2, 2); - data = trimMap(data, 2, Integer::compare); - assertThat(data).containsExactly(1, 1, 2,2); + data = firstNEntries(data, 2, Integer::compare); + assertThat(data).containsExactly(1, 1, 2, 2); } private void validateDiffCollection(List before, List after) { From c72063df45470ac7f1caf57599b4aa074d35c9c4 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 8 Dec 2021 13:39:43 -0700 Subject: [PATCH 20/20] Cleanup --- .../firestore/local/LocalDocumentsView.java | 42 ------------------- .../firestore/local/CountingQueryEngine.java | 13 +----- .../firestore/local/LocalStoreTestCase.java | 13 +----- 3 files changed, 2 insertions(+), 66 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java index aed7d1b5eee..8445dfefcd6 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java @@ -281,46 +281,4 @@ private ImmutableSortedMap getDocumentsMatchingCollection return results; } - - /** Queries the remote documents and mutation queue, by doing a full collection scan. */ - private ImmutableSortedMap - getDocumentsMatchingCollectionQueryFromMutationQueue(Query query, IndexOffset offset) { - Map remoteDocuments = - remoteDocumentCache.getAll(query.getPath(), offset); - - // TODO(indexing): We should plumb sinceReadTime through to the mutation queue - List matchingBatches = mutationQueue.getAllMutationBatchesAffectingQuery(query); - - for (MutationBatch batch : matchingBatches) { - for (Mutation mutation : batch.getMutations()) { - // Only process documents belonging to the collection. - if (!query.getPath().isImmediateParentOf(mutation.getKey().getPath())) { - continue; - } - - DocumentKey key = mutation.getKey(); - MutableDocument document = remoteDocuments.get(key); - if (document == null) { - // Create invalid document to apply mutations on top of - document = MutableDocument.newInvalidDocument(key); - remoteDocuments.put(key, document); - } - mutation.applyToLocalView( - document, FieldMask.fromSet(new HashSet<>()), batch.getLocalWriteTime()); - if (!document.isFoundDocument()) { - remoteDocuments.remove(key); - } - } - } - - ImmutableSortedMap results = emptyDocumentMap(); - for (Map.Entry docEntry : remoteDocuments.entrySet()) { - // Finally, insert the documents that still match the query - if (query.matches(docEntry.getValue())) { - results = results.insert(docEntry.getKey(), docEntry.getValue()); - } - } - - return results; - } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java index 9d11813c322..7b80198b890 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/CountingQueryEngine.java @@ -154,26 +154,15 @@ public Map getAll(Iterable documentKe public Map getAll( String collectionGroup, IndexOffset offset, int limit) { Map result = subject.getAll(collectionGroup, offset, limit); -<<<<<<< HEAD documentsReadByCollection[0] += result.size(); -======= - documentsReadByQuery[0] += result.size(); ->>>>>>> master + return result; } @Override -<<<<<<< HEAD public Map getAll(ResourcePath collection, IndexOffset offset) { Map result = subject.getAll(collection, offset); documentsReadByCollection[0] += result.size(); -======= - public ImmutableSortedMap getAllDocumentsMatchingQuery( - Query query, IndexOffset offset) { - ImmutableSortedMap result = - subject.getAllDocumentsMatchingQuery(query, offset); - documentsReadByQuery[0] += result.size(); ->>>>>>> master return result; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index c6c5110723a..e1377030cac 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -979,20 +979,9 @@ public void testReadsAllDocumentsForInitialCollectionQueries() { resetPersistenceStats(); localStore.executeQuery(query, /* usePreviousResults= */ true); -<<<<<<< HEAD assertRemoteDocumentsRead(/* byKey= */ 0, /* byCollection= */ 2); - if (Persistence.OVERLAY_SUPPORT_ENABLED) { - // No mutations are read because only overlay is needed. - assertMutationsRead(/* byKey= */ 0, /* byCollection= */ 0); - } else { - assertMutationsRead(/* byKey= */ 0, /* byCollection= */ 1); - } -======= - - assertRemoteDocumentsRead(/* byKey= */ 0, /* byQuery= */ 2); // No mutations are read because only overlay is needed. - assertMutationsRead(/* byKey= */ 0, /* byQuery= */ 0); ->>>>>>> master + assertMutationsRead(/* byKey= */ 0, /* byCollection= */ 0); } @Test