From 7df90c048837b809ae353ab0524a5c766ece062b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 26 Aug 2025 19:24:09 -0400 Subject: [PATCH 01/18] SQLiteRemoteDocumentCache.java: fix slow queries when a collection has many NO_DOCUMENT tombstones. This is an attempt to fix https://github.com/firebase/firebase-android-sdk/issues/7295 --- .../local/SQLiteRemoteDocumentCache.java | 41 +++++++++++++++++-- .../firestore/local/SQLiteSchema.java | 12 +++++- 2 files changed, 48 insertions(+), 5 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 b26f9601a81..724aabb44ee 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 @@ -65,6 +65,32 @@ public void setIndexManager(IndexManager indexManager) { this.indexManager = indexManager; } + private enum DocumentType { + NO_DOCUMENT(1), + FOUND_DOCUMENT(2), + UNKNOWN_DOCUMENT(3), + INVALID_DOCUMENT(4); + + final int dbValue; + + DocumentType(int dbValue) { + this.dbValue = dbValue; + } + + static DocumentType forMutableDocument(MutableDocument document) { + if (document.isNoDocument()) { + return NO_DOCUMENT; + } else if (document.isFoundDocument()) { + return FOUND_DOCUMENT; + } else if (document.isUnknownDocument()) { + return UNKNOWN_DOCUMENT; + } else { + hardAssert(!document.isValidDocument(), "MutableDocument has an unknown type"); + return INVALID_DOCUMENT; + } + } + } + @Override public void add(MutableDocument document, SnapshotVersion readTime) { hardAssert( @@ -77,12 +103,13 @@ public void add(MutableDocument document, SnapshotVersion readTime) { db.execute( "INSERT OR REPLACE INTO remote_documents " - + "(path, path_length, read_time_seconds, read_time_nanos, contents) " - + "VALUES (?, ?, ?, ?, ?)", + + "(path, path_length, read_time_seconds, read_time_nanos, document_type, contents) " + + "VALUES (?, ?, ?, ?, ?, ?)", EncodedPath.encode(documentKey.getPath()), documentKey.getPath().length(), timestamp.getSeconds(), timestamp.getNanoseconds(), + DocumentType.forMutableDocument(document).dbValue, message.toByteArray()); indexManager.addToCollectionParentIndex(document.getKey().getCollectionPath()); @@ -182,6 +209,7 @@ private Map getAll( List collections, IndexOffset offset, int count, + @Nullable DocumentType filterDocumentType, @Nullable Function filter, @Nullable QueryContext context) { Timestamp readTime = offset.getReadTime().getTimestamp(); @@ -192,6 +220,7 @@ private Map getAll( "SELECT contents, read_time_seconds, read_time_nanos, path " + "FROM remote_documents " + "WHERE path >= ? AND path < ? AND path_length = ? " + + (filterDocumentType == null ? "" : " AND document_type = ? ") + "AND (read_time_seconds > ? OR ( " + "read_time_seconds = ? AND read_time_nanos > ?) OR ( " + "read_time_seconds = ? AND read_time_nanos = ? and path > ?)) ", @@ -199,13 +228,16 @@ private Map getAll( " UNION "); sql.append("ORDER BY read_time_seconds, read_time_nanos, path LIMIT ?"); - Object[] bindVars = new Object[BINDS_PER_STATEMENT * collections.size() + 1]; + Object[] bindVars = new Object[BINDS_PER_STATEMENT * collections.size() + 1 + (filterDocumentType != null ? 1 : 0)]; int i = 0; for (ResourcePath collection : collections) { String prefixPath = EncodedPath.encode(collection); bindVars[i++] = prefixPath; bindVars[i++] = EncodedPath.prefixSuccessor(prefixPath); bindVars[i++] = collection.length() + 1; + if (filterDocumentType != null) { + bindVars[i++] = filterDocumentType.dbValue; + } bindVars[i++] = readTime.getSeconds(); bindVars[i++] = readTime.getSeconds(); bindVars[i++] = readTime.getNanoseconds(); @@ -235,7 +267,7 @@ private Map getAll( IndexOffset offset, int count, @Nullable Function filter) { - return getAll(collections, offset, count, filter, /*context*/ null); + return getAll(collections, offset, count, /*filterDocumentType*/ null, filter, /*context*/ null); } private void processRowInBackground( @@ -278,6 +310,7 @@ public Map getDocumentsMatchingQuery( Collections.singletonList(query.getPath()), offset, Integer.MAX_VALUE, + DocumentType.FOUND_DOCUMENT, (MutableDocument doc) -> query.matches(doc) || mutatedKeys.contains(doc.getKey()), context); } 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 ce758195446..adf48f82290 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 @@ -48,7 +48,7 @@ class SQLiteSchema { * The version of the schema. Increase this by one for each migration added to runMigrations * below. */ - static final int VERSION = 17; + static final int VERSION = 18; /** * The batch size for data migrations. @@ -183,6 +183,10 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { createGlobalsTable(); } + if (fromVersion < 18 && toVersion >= 18) { + addDocumentType(); + } + /* * Adding a new schema upgrade? READ THIS FIRST! * @@ -448,6 +452,12 @@ private void addPathLength() { } } + private void addDocumentType() { + if (!tableContainsColumn("remote_documents", "document_type")) { + db.execSQL("ALTER TABLE remote_documents ADD COLUMN document_type INTEGER"); + } + } + private boolean hasReadTime() { boolean hasReadTimeSeconds = tableContainsColumn("remote_documents", "read_time_seconds"); boolean hasReadTimeNanos = tableContainsColumn("remote_documents", "read_time_nanos"); From 80a65cc3adcb576c9f9716210dfae135a5c69e83 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 26 Aug 2025 19:34:53 -0400 Subject: [PATCH 02/18] spotlessApply --- .../firestore/local/SQLiteRemoteDocumentCache.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 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 724aabb44ee..ce865450aff 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 @@ -228,7 +228,9 @@ private Map getAll( " UNION "); sql.append("ORDER BY read_time_seconds, read_time_nanos, path LIMIT ?"); - Object[] bindVars = new Object[BINDS_PER_STATEMENT * collections.size() + 1 + (filterDocumentType != null ? 1 : 0)]; + Object[] bindVars = + new Object + [BINDS_PER_STATEMENT * collections.size() + 1 + (filterDocumentType != null ? 1 : 0)]; int i = 0; for (ResourcePath collection : collections) { String prefixPath = EncodedPath.encode(collection); @@ -267,7 +269,8 @@ private Map getAll( IndexOffset offset, int count, @Nullable Function filter) { - return getAll(collections, offset, count, /*filterDocumentType*/ null, filter, /*context*/ null); + return getAll( + collections, offset, count, /*filterDocumentType*/ null, filter, /*context*/ null); } private void processRowInBackground( From dcc0332bc1997811d3a0423a5991ce609e7f49b7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 27 Aug 2025 04:25:12 +0000 Subject: [PATCH 03/18] SQLiteRemoteDocumentCache.java: make sure to include rows where document_type is null --- .../firebase/firestore/local/SQLiteRemoteDocumentCache.java | 4 +++- 1 file changed, 3 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 ce865450aff..357aeda4f97 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 @@ -220,7 +220,9 @@ private Map getAll( "SELECT contents, read_time_seconds, read_time_nanos, path " + "FROM remote_documents " + "WHERE path >= ? AND path < ? AND path_length = ? " - + (filterDocumentType == null ? "" : " AND document_type = ? ") + + (filterDocumentType == null + ? "" + : " AND (document_type IS NULL OR document_type = ?) ") + "AND (read_time_seconds > ? OR ( " + "read_time_seconds = ? AND read_time_nanos > ?) OR ( " + "read_time_seconds = ? AND read_time_nanos = ? and path > ?)) ", From ee6844130a524f92166091530acd6d14ff3a23ce Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 27 Aug 2025 04:26:03 +0000 Subject: [PATCH 04/18] SQLiteRemoteDocumentCache.java: fix size of bindVars in the case that the length of the given collections is greater than zero. --- .../firebase/firestore/local/SQLiteRemoteDocumentCache.java | 2 +- 1 file changed, 1 insertion(+), 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 357aeda4f97..f9db5616770 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 @@ -232,7 +232,7 @@ private Map getAll( Object[] bindVars = new Object - [BINDS_PER_STATEMENT * collections.size() + 1 + (filterDocumentType != null ? 1 : 0)]; + [(BINDS_PER_STATEMENT + (filterDocumentType != null ? 1 : 0)) * collections.size() + 1]; int i = 0; for (ResourcePath collection : collections) { String prefixPath = EncodedPath.encode(collection); From 6ddff9cf04a87e94f98765ce927f6ca05acdf506 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 27 Aug 2025 04:32:02 +0000 Subject: [PATCH 05/18] SQLiteSchemaTest.java: add a test --- .../firestore/local/SQLiteSchemaTest.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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 6e250332143..eaa807d9b67 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 @@ -702,6 +702,27 @@ public void createsOverlaysAndMigrationTable() { assertEquals(Persistence.DATA_MIGRATION_BUILD_OVERLAYS, migrationName); } + @Test + public void existingDocumentsMatchAfterRemoteDocumentsDocumentTypeColumnAdded() { + schema.runSchemaUpgrades(0, 1); + for (int i = 0; i < 3; i++) { + db.execSQL( + "INSERT INTO remote_documents (path, contents) VALUES (?, ?)", + new Object[] {encode(path("coll/doc" + i)), createDummyDocument("coll/doc" + i)}); + } + + // The migration of interest is 18, but go to the latest migration to ensure compatibility with + // the SQLiteRemoteDocumentCache implementation. + schema.runSchemaUpgrades(2, VERSION); + + SQLiteRemoteDocumentCache remoteDocumentCache = createRemoteDocumentCache(); + + Map results = + remoteDocumentCache.getDocumentsMatchingQuery( + query("coll"), IndexOffset.NONE, new HashSet()); + assertResultsContain(results, "coll/doc0", "coll/doc1", "coll/doc2"); + } + private SQLiteRemoteDocumentCache createRemoteDocumentCache() { SQLitePersistence persistence = new SQLitePersistence(serializer, LruGarbageCollector.Params.Default(), opener); From a466c0bc12f9b2012325df45785320e05eff219d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 27 Aug 2025 00:37:48 -0400 Subject: [PATCH 06/18] CHANGELOG.md entry added --- firebase-firestore/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 861ab4c0d9b..a25dc0c5af7 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,4 +1,6 @@ # Unreleased +* [changed] Improve the performance of queries in collections that contain many deleted documents. + [#7295](//github.com/firebase/firebase-android-sdk/issues/7295) # 26.0.0 From cbdfd5fc31e6948faf1974320a62aaebab28b9f9 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 27 Aug 2025 00:43:40 -0400 Subject: [PATCH 07/18] add a comment about _why_ filterDocumentType=FOUND_DOCUMENT is specified to getAll() --- .../firebase/firestore/local/SQLiteRemoteDocumentCache.java | 3 +++ 1 file changed, 3 insertions(+) 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 f9db5616770..f39edace1ee 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 @@ -315,6 +315,9 @@ public Map getDocumentsMatchingQuery( Collections.singletonList(query.getPath()), offset, Integer.MAX_VALUE, + // Specify filterDocumentType=FOUND_DOCUMENT to getAll() as an optimization, because + // query.matches(doc) will return false for all non-"found" document types anyways. + // See https://github.com/firebase/firebase-android-sdk/issues/7295 DocumentType.FOUND_DOCUMENT, (MutableDocument doc) -> query.matches(doc) || mutatedKeys.contains(doc.getKey()), context); From 6004cdf3a47b677b0f8deb1c3dd9f6f2da44c23b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 27 Aug 2025 00:55:05 -0400 Subject: [PATCH 08/18] Rename `filterDocumentType` to `tryFilterDocumentType` to emphasize that the filtering is a "best effort" and not guaranteed. --- .../local/SQLiteRemoteDocumentCache.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 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 f39edace1ee..4e77ea65f1b 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 @@ -209,7 +209,7 @@ private Map getAll( List collections, IndexOffset offset, int count, - @Nullable DocumentType filterDocumentType, + @Nullable DocumentType tryFilterDocumentType, @Nullable Function filter, @Nullable QueryContext context) { Timestamp readTime = offset.getReadTime().getTimestamp(); @@ -220,7 +220,7 @@ private Map getAll( "SELECT contents, read_time_seconds, read_time_nanos, path " + "FROM remote_documents " + "WHERE path >= ? AND path < ? AND path_length = ? " - + (filterDocumentType == null + + (tryFilterDocumentType == null ? "" : " AND (document_type IS NULL OR document_type = ?) ") + "AND (read_time_seconds > ? OR ( " @@ -232,15 +232,16 @@ private Map getAll( Object[] bindVars = new Object - [(BINDS_PER_STATEMENT + (filterDocumentType != null ? 1 : 0)) * collections.size() + 1]; + [(BINDS_PER_STATEMENT + (tryFilterDocumentType != null ? 1 : 0)) * collections.size() + + 1]; int i = 0; for (ResourcePath collection : collections) { String prefixPath = EncodedPath.encode(collection); bindVars[i++] = prefixPath; bindVars[i++] = EncodedPath.prefixSuccessor(prefixPath); bindVars[i++] = collection.length() + 1; - if (filterDocumentType != null) { - bindVars[i++] = filterDocumentType.dbValue; + if (tryFilterDocumentType != null) { + bindVars[i++] = tryFilterDocumentType.dbValue; } bindVars[i++] = readTime.getSeconds(); bindVars[i++] = readTime.getSeconds(); @@ -272,7 +273,7 @@ private Map getAll( int count, @Nullable Function filter) { return getAll( - collections, offset, count, /*filterDocumentType*/ null, filter, /*context*/ null); + collections, offset, count, /*tryFilterDocumentType*/ null, filter, /*context*/ null); } private void processRowInBackground( @@ -315,7 +316,7 @@ public Map getDocumentsMatchingQuery( Collections.singletonList(query.getPath()), offset, Integer.MAX_VALUE, - // Specify filterDocumentType=FOUND_DOCUMENT to getAll() as an optimization, because + // Specify tryFilterDocumentType=FOUND_DOCUMENT to getAll() as an optimization, because // query.matches(doc) will return false for all non-"found" document types anyways. // See https://github.com/firebase/firebase-android-sdk/issues/7295 DocumentType.FOUND_DOCUMENT, From cb0f0d982cf1a33eac512b12ad00ef56773f352b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 27 Aug 2025 09:35:19 -0400 Subject: [PATCH 09/18] add comments explaining the document_type column --- .../com/google/firebase/firestore/local/SQLiteSchema.java | 6 ++++++ 1 file changed, 6 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 adf48f82290..88af8a53292 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 @@ -453,6 +453,12 @@ private void addPathLength() { } private void addDocumentType() { + // The new "document_type" column is a copy of the document type encoded in the "contents" blob. + // Its range of values are defined in the `SQLiteRemoteDocumentCache.DocumentType` enum. + // The "document_type" value for a given row must be equal to the document type encoded in the + // "contents" column for that row. The purpose of the "document_type" column is to enable + // efficient filtering on document type. But when using it as a filter, a null value must also + // be considered as their document type must be determined by parsing the the "contents" column. if (!tableContainsColumn("remote_documents", "document_type")) { db.execSQL("ALTER TABLE remote_documents ADD COLUMN document_type INTEGER"); } From 46b6943443e560d95ec243f3e5ec0b82f95d4702 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 28 Aug 2025 10:39:16 -0400 Subject: [PATCH 10/18] SQLiteRemoteDocumentCache.java: prepare for backfilling document type --- .../local/SQLiteRemoteDocumentCache.java | 80 ++++++++++++++++--- 1 file changed, 69 insertions(+), 11 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 4e77ea65f1b..c9ce7e9f843 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 @@ -21,6 +21,7 @@ import static com.google.firebase.firestore.util.Util.repeatSequence; import android.database.Cursor; +import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedMap; @@ -147,30 +148,40 @@ public MutableDocument get(DocumentKey documentKey) { public Map getAll(Iterable documentKeys) { Map results = new HashMap<>(); List bindVars = new ArrayList<>(); - for (DocumentKey key : documentKeys) { - bindVars.add(EncodedPath.encode(key.getPath())); + synchronized (results) { + for (DocumentKey key : documentKeys) { + 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)); + // 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 " + "SELECT contents, read_time_seconds, read_time_nanos, document_type, path " + + "FROM remote_documents " + "WHERE path IN (", bindVars, ") ORDER BY path"); BackgroundQueue backgroundQueue = new BackgroundQueue(); + ArrayList documentTypeBackfills = new ArrayList<>(); while (longQuery.hasMoreSubqueries()) { longQuery .performNextSubquery() - .forEach(row -> processRowInBackground(backgroundQueue, results, row, /*filter*/ null)); + .forEach( + row -> + processRowInBackground( + backgroundQueue, results, row, documentTypeBackfills, /*filter*/ null)); } backgroundQueue.drain(); - return results; + + synchronized (results) { + return results; + } } @Override @@ -217,7 +228,7 @@ private Map getAll( StringBuilder sql = repeatSequence( - "SELECT contents, read_time_seconds, read_time_nanos, path " + "SELECT contents, read_time_seconds, read_time_nanos, document_type, path " + "FROM remote_documents " + "WHERE path >= ? AND path < ? AND path_length = ? " + (tryFilterDocumentType == null @@ -254,17 +265,21 @@ private Map getAll( BackgroundQueue backgroundQueue = new BackgroundQueue(); Map results = new HashMap<>(); + ArrayList documentTypeBackfills = new ArrayList<>(); db.query(sql.toString()) .binding(bindVars) .forEach( row -> { - processRowInBackground(backgroundQueue, results, row, filter); + processRowInBackground(backgroundQueue, results, row, documentTypeBackfills, filter); if (context != null) { context.incrementDocumentReadCount(); } }); backgroundQueue.drain(); - return results; + + synchronized (results) { + return results; + } } private Map getAll( @@ -280,10 +295,13 @@ private void processRowInBackground( BackgroundQueue backgroundQueue, Map results, Cursor row, + List documentTypeBackfills, @Nullable Function filter) { byte[] rawDocument = row.getBlob(0); int readTimeSeconds = row.getInt(1); int readTimeNanos = row.getInt(2); + boolean documentTypeIsNull = row.isNull(3); + String path = row.getString(4); // Since scheduling background tasks incurs overhead, we only dispatch to a // background thread if there are still some documents remaining. @@ -292,6 +310,17 @@ private void processRowInBackground( () -> { MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds, readTimeNanos); + if (documentTypeIsNull) { + DocumentTypeUpdateInfo updateInfo = + new DocumentTypeUpdateInfo( + path, + readTimeSeconds, + readTimeNanos, + DocumentType.forMutableDocument(document)); + synchronized (documentTypeBackfills) { + documentTypeBackfills.add(updateInfo); + } + } if (filter == null || filter.apply(document)) { synchronized (results) { results.put(document.getKey(), document); @@ -334,4 +363,33 @@ private MutableDocument decodeMaybeDocument( throw fail("MaybeDocument failed to parse: %s", e); } } + + private static class DocumentTypeUpdateInfo { + final String path; + final int readTimeSeconds; + final int readTimeNanos; + final DocumentType documentType; + + DocumentTypeUpdateInfo( + String path, int readTimeSeconds, int readTimeNanos, DocumentType documentType) { + this.path = path; + this.readTimeSeconds = readTimeSeconds; + this.readTimeNanos = readTimeNanos; + this.documentType = documentType; + } + + @NonNull + @Override + public String toString() { + return "DocumentTypeUpdateInfo(path=" + + path + + ", readTimeSeconds=" + + readTimeSeconds + + ", readTimeNanos=" + + readTimeNanos + + ", documentType=" + + documentType + + ")"; + } + } } From 21580d100ac0a62ded1bcb4b1804e4f96da821ec Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 28 Aug 2025 12:15:58 -0400 Subject: [PATCH 11/18] run document type backfills on query --- .../local/SQLiteRemoteDocumentCache.java | 151 +++++++++++++----- 1 file changed, 110 insertions(+), 41 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 c9ce7e9f843..f1393de58c9 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 @@ -21,6 +21,7 @@ import static com.google.firebase.firestore.util.Util.repeatSequence; import android.database.Cursor; +import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; @@ -41,9 +42,11 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -56,6 +59,8 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache { private final LocalSerializer serializer; private IndexManager indexManager; + private final DocumentTypeBackfills documentTypeBackfills = new DocumentTypeBackfills(); + SQLiteRemoteDocumentCache(SQLitePersistence persistence, LocalSerializer serializer) { this.db = persistence; this.serializer = serializer; @@ -168,17 +173,15 @@ public Map getAll(Iterable documentKe ") ORDER BY path"); BackgroundQueue backgroundQueue = new BackgroundQueue(); - ArrayList documentTypeBackfills = new ArrayList<>(); while (longQuery.hasMoreSubqueries()) { longQuery .performNextSubquery() - .forEach( - row -> - processRowInBackground( - backgroundQueue, results, row, documentTypeBackfills, /*filter*/ null)); + .forEach(row -> processRowInBackground(backgroundQueue, results, row, /*filter*/ null)); } backgroundQueue.drain(); + documentTypeBackfills.backfill(db); + synchronized (results) { return results; } @@ -265,18 +268,19 @@ private Map getAll( BackgroundQueue backgroundQueue = new BackgroundQueue(); Map results = new HashMap<>(); - ArrayList documentTypeBackfills = new ArrayList<>(); db.query(sql.toString()) .binding(bindVars) .forEach( row -> { - processRowInBackground(backgroundQueue, results, row, documentTypeBackfills, filter); + processRowInBackground(backgroundQueue, results, row, filter); if (context != null) { context.incrementDocumentReadCount(); } }); backgroundQueue.drain(); + documentTypeBackfills.backfill(db); + synchronized (results) { return results; } @@ -295,7 +299,6 @@ private void processRowInBackground( BackgroundQueue backgroundQueue, Map results, Cursor row, - List documentTypeBackfills, @Nullable Function filter) { byte[] rawDocument = row.getBlob(0); int readTimeSeconds = row.getInt(1); @@ -311,15 +314,7 @@ private void processRowInBackground( MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds, readTimeNanos); if (documentTypeIsNull) { - DocumentTypeUpdateInfo updateInfo = - new DocumentTypeUpdateInfo( - path, - readTimeSeconds, - readTimeNanos, - DocumentType.forMutableDocument(document)); - synchronized (documentTypeBackfills) { - documentTypeBackfills.add(updateInfo); - } + documentTypeBackfills.add(path, readTimeSeconds, readTimeNanos, document); } if (filter == null || filter.apply(document)) { synchronized (results) { @@ -364,32 +359,106 @@ private MutableDocument decodeMaybeDocument( } } - private static class DocumentTypeUpdateInfo { - final String path; - final int readTimeSeconds; - final int readTimeNanos; - final DocumentType documentType; - - DocumentTypeUpdateInfo( - String path, int readTimeSeconds, int readTimeNanos, DocumentType documentType) { - this.path = path; - this.readTimeSeconds = readTimeSeconds; - this.readTimeNanos = readTimeNanos; - this.documentType = documentType; + // This class is thread safe and all public methods may be safely called concurrently from + // multiple threads. This makes it safe to use instances of this class from BackgroundQueue. + private static class DocumentTypeBackfills { + + private final ConcurrentHashMap documentTypeByBackfillKey = + new ConcurrentHashMap<>(); + + enum BackfillResult { + NO_PENDING_BACKFILLS, + HAS_PENDING_BACKFILLS, + } + + void add(String path, int readTimeSeconds, int readTimeNanos, MutableDocument document) { + BackfillKey backfillKey = new BackfillKey(path, readTimeSeconds, readTimeNanos); + DocumentType documentType = DocumentType.forMutableDocument(document); + documentTypeByBackfillKey.putIfAbsent(backfillKey, documentType); + } + + BackfillResult backfill(SQLitePersistence db) { + ArrayList caseClauses = new ArrayList<>(); + ArrayList caseClauseBindings = new ArrayList<>(); + ArrayList whereClauses = new ArrayList<>(); + ArrayList whereClauseBindings = new ArrayList<>(); + + Iterator backfillKeys = documentTypeByBackfillKey.keySet().iterator(); + while (backfillKeys.hasNext() + && caseClauseBindings.size() + whereClauseBindings.size() < 900) { + BackfillKey backfillKey = backfillKeys.next(); + DocumentType documentType = documentTypeByBackfillKey.remove(backfillKey); + if (documentType == null) { + continue; + } + + caseClauses.add("WHEN path=? AND read_time_seconds=? AND read_time_nanos=? THEN ?"); + caseClauseBindings.add(backfillKey.path); + caseClauseBindings.add(backfillKey.readTimeSeconds); + caseClauseBindings.add(backfillKey.readTimeNanos); + caseClauseBindings.add(documentType.dbValue); + + whereClauses.add("(path=? AND read_time_seconds=? AND read_time_nanos=?)"); + whereClauseBindings.add(backfillKey.path); + whereClauseBindings.add(backfillKey.readTimeSeconds); + whereClauseBindings.add(backfillKey.readTimeNanos); + } + + if (!caseClauseBindings.isEmpty()) { + String sql; + { + StringBuilder sb = new StringBuilder("UPDATE remote_documents SET document_type = CASE"); + for (String caseClause : caseClauses) { + sb.append(' ').append(caseClause); + } + sb.append(" ELSE NULL END WHERE "); + boolean isFirstWhereClause = true; + for (String whereClause : whereClauses) { + if (isFirstWhereClause) { + isFirstWhereClause = false; + } else { + sb.append(" OR "); + } + sb.append(whereClause); + } + sql = sb.toString(); + } + + caseClauseBindings.addAll(whereClauseBindings); + Object[] bindings = caseClauseBindings.toArray(); + + Log.i("zzyzx", "sql=sql"); + + db.execute(sql, bindings); + } + + return documentTypeByBackfillKey.isEmpty() + ? BackfillResult.NO_PENDING_BACKFILLS + : BackfillResult.HAS_PENDING_BACKFILLS; } - @NonNull - @Override - public String toString() { - return "DocumentTypeUpdateInfo(path=" - + path - + ", readTimeSeconds=" - + readTimeSeconds - + ", readTimeNanos=" - + readTimeNanos - + ", documentType=" - + documentType - + ")"; + private static class BackfillKey { + final String path; + final int readTimeSeconds; + final int readTimeNanos; + + BackfillKey(String path, int readTimeSeconds, int readTimeNanos) { + this.path = path; + this.readTimeSeconds = readTimeSeconds; + this.readTimeNanos = readTimeNanos; + } + + @NonNull + @Override + public String toString() { + return "DocumentTypeBackfills.BackfillKey(path=" + + path + + ", readTimeSeconds=" + + readTimeSeconds + + ", readTimeNanos=" + + readTimeNanos + + ")"; + } } } } From 96fe2e7d8f39e1ef1da887b0b18c042295b39174 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 28 Aug 2025 13:25:44 -0400 Subject: [PATCH 12/18] use numbered sqlite parameters so we can fit more updates into a single statement --- .../firestore/local/SQLitePersistence.java | 2 +- .../local/SQLiteRemoteDocumentCache.java | 85 +++++++++---------- 2 files changed, 43 insertions(+), 44 deletions(-) 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 b9c89d3cb8b..a7e353cd6e8 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 @@ -616,7 +616,7 @@ static class LongQuery { // attempt to check for placeholders in the query {@link head}; if it only relied on the number // of placeholders it itself generates, in that situation it would still exceed the SQLite // limit. - private static final int LIMIT = 900; + static final int LIMIT = 900; /** * Creates a new {@code LongQuery} with parameters that describe a template for creating each 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 f1393de58c9..29d8e72fb8f 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 @@ -21,7 +21,6 @@ import static com.google.firebase.firestore.util.Util.repeatSequence; import android.database.Cursor; -import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; @@ -378,58 +377,58 @@ void add(String path, int readTimeSeconds, int readTimeNanos, MutableDocument do } BackfillResult backfill(SQLitePersistence db) { - ArrayList caseClauses = new ArrayList<>(); - ArrayList caseClauseBindings = new ArrayList<>(); - ArrayList whereClauses = new ArrayList<>(); - ArrayList whereClauseBindings = new ArrayList<>(); + StringBuilder caseClauses = new StringBuilder(); + StringBuilder whereClauses = new StringBuilder(); + ArrayList bindings = new ArrayList<>(); Iterator backfillKeys = documentTypeByBackfillKey.keySet().iterator(); - while (backfillKeys.hasNext() - && caseClauseBindings.size() + whereClauseBindings.size() < 900) { + while (backfillKeys.hasNext() && bindings.size() < SQLitePersistence.LongQuery.LIMIT) { BackfillKey backfillKey = backfillKeys.next(); DocumentType documentType = documentTypeByBackfillKey.remove(backfillKey); if (documentType == null) { continue; } - caseClauses.add("WHEN path=? AND read_time_seconds=? AND read_time_nanos=? THEN ?"); - caseClauseBindings.add(backfillKey.path); - caseClauseBindings.add(backfillKey.readTimeSeconds); - caseClauseBindings.add(backfillKey.readTimeNanos); - caseClauseBindings.add(documentType.dbValue); - - whereClauses.add("(path=? AND read_time_seconds=? AND read_time_nanos=?)"); - whereClauseBindings.add(backfillKey.path); - whereClauseBindings.add(backfillKey.readTimeSeconds); - whereClauseBindings.add(backfillKey.readTimeNanos); - } - - if (!caseClauseBindings.isEmpty()) { - String sql; - { - StringBuilder sb = new StringBuilder("UPDATE remote_documents SET document_type = CASE"); - for (String caseClause : caseClauses) { - sb.append(' ').append(caseClause); - } - sb.append(" ELSE NULL END WHERE "); - boolean isFirstWhereClause = true; - for (String whereClause : whereClauses) { - if (isFirstWhereClause) { - isFirstWhereClause = false; - } else { - sb.append(" OR "); - } - sb.append(whereClause); - } - sql = sb.toString(); + bindings.add(backfillKey.path); + int pathBindingNumber = bindings.size(); + bindings.add(backfillKey.readTimeSeconds); + int readTimeSecondsBindingNumber = bindings.size(); + bindings.add(backfillKey.readTimeNanos); + int readTimeNanosBindingNumber = bindings.size(); + bindings.add(documentType.dbValue); + int dbValueBindingNumber = bindings.size(); + + caseClauses + .append(" WHEN path=?") + .append(pathBindingNumber) + .append(" AND read_time_seconds=?") + .append(readTimeSecondsBindingNumber) + .append(" AND read_time_nanos=?") + .append(readTimeNanosBindingNumber) + .append(" THEN ?") + .append(dbValueBindingNumber); + + if (whereClauses.length() > 0) { + whereClauses.append(" OR"); } + whereClauses + .append(" (path=?") + .append(pathBindingNumber) + .append(" AND read_time_seconds=?") + .append(readTimeSecondsBindingNumber) + .append(" AND read_time_nanos=?") + .append(readTimeNanosBindingNumber) + .append(')'); + } - caseClauseBindings.addAll(whereClauseBindings); - Object[] bindings = caseClauseBindings.toArray(); - - Log.i("zzyzx", "sql=sql"); - - db.execute(sql, bindings); + if (!bindings.isEmpty()) { + String sql = + "UPDATE remote_documents SET document_type = CASE" + + caseClauses + + " ELSE NULL END WHERE" + + whereClauses; + android.util.Log.i("zzyzx", sql); + db.execute(sql, bindings.toArray()); } return documentTypeByBackfillKey.isEmpty() From df9bf438488cf47a21f4b66aeefd561739ff6b44 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 28 Aug 2025 13:41:04 -0400 Subject: [PATCH 13/18] SQLiteRemoteDocumentCache.java: factor out sql statement string building --- .../local/SQLiteRemoteDocumentCache.java | 40 +++++++++++++------ 1 file changed, 28 insertions(+), 12 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 29d8e72fb8f..21aa5a6c2f3 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 @@ -377,11 +377,31 @@ void add(String path, int readTimeSeconds, int readTimeNanos, MutableDocument do } BackfillResult backfill(SQLitePersistence db) { + // Just return immediately if there are no pending backfills, as a performance optimization. + // This just elides a few allocations (e.g. the ArrayList below) that would otherwise + // needlessly occur. + if (documentTypeByBackfillKey.isEmpty()) { + return BackfillResult.NO_PENDING_BACKFILLS; + } + + ArrayList sqlBindings = new ArrayList<>(); + String sql = calculateBackfillSql(sqlBindings); + if (sql != null) { + db.execute(sql, sqlBindings.toArray()); + } + + return documentTypeByBackfillKey.isEmpty() + ? BackfillResult.NO_PENDING_BACKFILLS + : BackfillResult.HAS_PENDING_BACKFILLS; + } + + @Nullable + String calculateBackfillSql(ArrayList bindings) { StringBuilder caseClauses = new StringBuilder(); StringBuilder whereClauses = new StringBuilder(); - ArrayList bindings = new ArrayList<>(); Iterator backfillKeys = documentTypeByBackfillKey.keySet().iterator(); + boolean backfillsFound = false; while (backfillKeys.hasNext() && bindings.size() < SQLitePersistence.LongQuery.LIMIT) { BackfillKey backfillKey = backfillKeys.next(); DocumentType documentType = documentTypeByBackfillKey.remove(backfillKey); @@ -389,6 +409,7 @@ BackfillResult backfill(SQLitePersistence db) { continue; } + backfillsFound = true; bindings.add(backfillKey.path); int pathBindingNumber = bindings.size(); bindings.add(backfillKey.readTimeSeconds); @@ -421,19 +442,14 @@ BackfillResult backfill(SQLitePersistence db) { .append(')'); } - if (!bindings.isEmpty()) { - String sql = - "UPDATE remote_documents SET document_type = CASE" - + caseClauses - + " ELSE NULL END WHERE" - + whereClauses; - android.util.Log.i("zzyzx", sql); - db.execute(sql, bindings.toArray()); + if (!backfillsFound) { + return null; } - return documentTypeByBackfillKey.isEmpty() - ? BackfillResult.NO_PENDING_BACKFILLS - : BackfillResult.HAS_PENDING_BACKFILLS; + return "UPDATE remote_documents SET document_type = CASE" + + caseClauses + + " ELSE NULL END WHERE" + + whereClauses; } private static class BackfillKey { From f2a149f52afeda67db1ac0f13dedd38869bb340d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 28 Aug 2025 13:58:39 -0400 Subject: [PATCH 14/18] code and comment cleanup/improvement --- .../local/SQLiteRemoteDocumentCache.java | 35 ++++++++++++++----- 1 file changed, 26 insertions(+), 9 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 21aa5a6c2f3..4539128b32b 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 @@ -152,14 +152,12 @@ public MutableDocument get(DocumentKey documentKey) { public Map getAll(Iterable documentKeys) { Map results = new HashMap<>(); List bindVars = new ArrayList<>(); - synchronized (results) { - for (DocumentKey key : documentKeys) { - bindVars.add(EncodedPath.encode(key.getPath())); + for (DocumentKey key : documentKeys) { + 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)); - } + // 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 = @@ -181,6 +179,7 @@ public Map getAll(Iterable documentKe documentTypeBackfills.backfill(db); + // Synchronize on `results` to avoid a data race with the background queue. synchronized (results) { return results; } @@ -280,6 +279,7 @@ private Map getAll( documentTypeBackfills.backfill(db); + // Synchronize on `results` to avoid a data race with the background queue. synchronized (results) { return results; } @@ -358,8 +358,25 @@ private MutableDocument decodeMaybeDocument( } } - // This class is thread safe and all public methods may be safely called concurrently from - // multiple threads. This makes it safe to use instances of this class from BackgroundQueue. + /** + * Helper class to backfill the `document_type` column in the `remote_documents` table. + *

+ * The `document_type` column was added as an optimization to skip deleted document tombstones + * when running queries. Any time a new row is added to the `remote_documents` table it _should_ + * have its `document_type` column set to the value that matches the `contents` field. However, + * when upgrading from an older schema version the column value for existing rows will be null + * and this backfiller is intended to replace those null values to improve the future performance + * of queries. + *

+ * When traversing the `remote_documents` table call `add()` upon finding a row whose + * `document_type` is null. Then, call `backfill()` later on to efficiently update the added + * rows in batches. + *

+ * This class is thread safe and all public methods may be safely called concurrently from + * multiple threads. This makes it safe to use instances of this class from BackgroundQueue. + * + * @see #7295 + */ private static class DocumentTypeBackfills { private final ConcurrentHashMap documentTypeByBackfillKey = From d5b96d928cbd6b22f380ed0062d50c52ffdc903f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 28 Aug 2025 13:59:41 -0400 Subject: [PATCH 15/18] add todo comment --- .../firebase/firestore/local/SQLiteRemoteDocumentCache.java | 2 ++ 1 file changed, 2 insertions(+) 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 4539128b32b..c9154a1b073 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 @@ -177,6 +177,7 @@ public Map getAll(Iterable documentKe } backgroundQueue.drain(); + // TODO(dconeybe): schedule the backfill asynchronously. documentTypeBackfills.backfill(db); // Synchronize on `results` to avoid a data race with the background queue. @@ -277,6 +278,7 @@ private Map getAll( }); backgroundQueue.drain(); + // TODO(dconeybe): schedule the backfill asynchronously. documentTypeBackfills.backfill(db); // Synchronize on `results` to avoid a data race with the background queue. From 8baaf6b413506bb6e44c96373623956cae687140 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 29 Aug 2025 00:54:30 -0400 Subject: [PATCH 16/18] do backfill during query --- .../local/SQLiteRemoteDocumentCache.java | 81 ++++++++++--------- 1 file changed, 45 insertions(+), 36 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 c9154a1b073..76542f6ea8e 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 @@ -58,7 +58,7 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache { private final LocalSerializer serializer; private IndexManager indexManager; - private final DocumentTypeBackfills documentTypeBackfills = new DocumentTypeBackfills(); + private final DocumentTypeBackfiller documentTypeBackfiller = new DocumentTypeBackfiller(); SQLiteRemoteDocumentCache(SQLitePersistence persistence, LocalSerializer serializer) { this.db = persistence; @@ -177,8 +177,8 @@ public Map getAll(Iterable documentKe } backgroundQueue.drain(); - // TODO(dconeybe): schedule the backfill asynchronously. - documentTypeBackfills.backfill(db); + // Backfill any rows with null "document_type" discovered by processRowInBackground(). + documentTypeBackfiller.backfill(db); // Synchronize on `results` to avoid a data race with the background queue. synchronized (results) { @@ -278,8 +278,8 @@ private Map getAll( }); backgroundQueue.drain(); - // TODO(dconeybe): schedule the backfill asynchronously. - documentTypeBackfills.backfill(db); + // Backfill any null "document_type" columns discovered by processRowInBackground(). + documentTypeBackfiller.backfill(db); // Synchronize on `results` to avoid a data race with the background queue. synchronized (results) { @@ -315,7 +315,7 @@ private void processRowInBackground( MutableDocument document = decodeMaybeDocument(rawDocument, readTimeSeconds, readTimeNanos); if (documentTypeIsNull) { - documentTypeBackfills.add(path, readTimeSeconds, readTimeNanos, document); + documentTypeBackfiller.enqueue(path, readTimeSeconds, readTimeNanos, document); } if (filter == null || filter.apply(document)) { synchronized (results) { @@ -379,48 +379,54 @@ private MutableDocument decodeMaybeDocument( * * @see #7295 */ - private static class DocumentTypeBackfills { + private static class DocumentTypeBackfiller { private final ConcurrentHashMap documentTypeByBackfillKey = new ConcurrentHashMap<>(); - enum BackfillResult { - NO_PENDING_BACKFILLS, - HAS_PENDING_BACKFILLS, - } - - void add(String path, int readTimeSeconds, int readTimeNanos, MutableDocument document) { + void enqueue(String path, int readTimeSeconds, int readTimeNanos, MutableDocument document) { BackfillKey backfillKey = new BackfillKey(path, readTimeSeconds, readTimeNanos); DocumentType documentType = DocumentType.forMutableDocument(document); documentTypeByBackfillKey.putIfAbsent(backfillKey, documentType); } - BackfillResult backfill(SQLitePersistence db) { - // Just return immediately if there are no pending backfills, as a performance optimization. - // This just elides a few allocations (e.g. the ArrayList below) that would otherwise - // needlessly occur. - if (documentTypeByBackfillKey.isEmpty()) { - return BackfillResult.NO_PENDING_BACKFILLS; + void backfill(SQLitePersistence db) { + while (true) { + BackfillSqlInfo backfillSqlInfo = calculateBackfillSql(); + if (backfillSqlInfo == null) { + break; + } + android.util.Log.i( + "zzyzx", + "Backfilling document_type for " + backfillSqlInfo.numDocumentsAffected + " documents"); + db.execute(backfillSqlInfo.sql, backfillSqlInfo.bindings); } + } - ArrayList sqlBindings = new ArrayList<>(); - String sql = calculateBackfillSql(sqlBindings); - if (sql != null) { - db.execute(sql, sqlBindings.toArray()); - } + private static class BackfillSqlInfo { + final String sql; + final Object[] bindings; + final int numDocumentsAffected; - return documentTypeByBackfillKey.isEmpty() - ? BackfillResult.NO_PENDING_BACKFILLS - : BackfillResult.HAS_PENDING_BACKFILLS; + BackfillSqlInfo(String sql, Object[] bindings, int numDocumentsAffected) { + this.sql = sql; + this.bindings = bindings; + this.numDocumentsAffected = numDocumentsAffected; + } } @Nullable - String calculateBackfillSql(ArrayList bindings) { + BackfillSqlInfo calculateBackfillSql() { + if (documentTypeByBackfillKey.isEmpty()) { + return null; // short circuit + } + + ArrayList bindings = new ArrayList<>(); StringBuilder caseClauses = new StringBuilder(); StringBuilder whereClauses = new StringBuilder(); Iterator backfillKeys = documentTypeByBackfillKey.keySet().iterator(); - boolean backfillsFound = false; + int numDocumentsAffected = 0; while (backfillKeys.hasNext() && bindings.size() < SQLitePersistence.LongQuery.LIMIT) { BackfillKey backfillKey = backfillKeys.next(); DocumentType documentType = documentTypeByBackfillKey.remove(backfillKey); @@ -428,7 +434,7 @@ String calculateBackfillSql(ArrayList bindings) { continue; } - backfillsFound = true; + numDocumentsAffected++; bindings.add(backfillKey.path); int pathBindingNumber = bindings.size(); bindings.add(backfillKey.readTimeSeconds); @@ -461,14 +467,17 @@ String calculateBackfillSql(ArrayList bindings) { .append(')'); } - if (!backfillsFound) { + if (numDocumentsAffected == 0) { return null; } - return "UPDATE remote_documents SET document_type = CASE" - + caseClauses - + " ELSE NULL END WHERE" - + whereClauses; + String sql = + "UPDATE remote_documents SET document_type = CASE" + + caseClauses + + " ELSE NULL END WHERE" + + whereClauses; + + return new BackfillSqlInfo(sql, bindings.toArray(), numDocumentsAffected); } private static class BackfillKey { @@ -485,7 +494,7 @@ private static class BackfillKey { @NonNull @Override public String toString() { - return "DocumentTypeBackfills.BackfillKey(path=" + return "DocumentTypeBackfiller.BackfillKey(path=" + path + ", readTimeSeconds=" + readTimeSeconds From 4a222340d5ea3045fba4b6ae58fcc8fb3095aae3 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 29 Aug 2025 01:00:43 -0400 Subject: [PATCH 17/18] clean up --- .../firebase/firestore/local/SQLiteRemoteDocumentCache.java | 3 --- 1 file changed, 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 76542f6ea8e..5591da14b7b 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 @@ -396,9 +396,6 @@ void backfill(SQLitePersistence db) { if (backfillSqlInfo == null) { break; } - android.util.Log.i( - "zzyzx", - "Backfilling document_type for " + backfillSqlInfo.numDocumentsAffected + " documents"); db.execute(backfillSqlInfo.sql, backfillSqlInfo.bindings); } } From df8aefd311b6c2f9ca9bf3c40d6183f59e869a85 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 29 Aug 2025 10:42:19 -0400 Subject: [PATCH 18/18] spotlessApply --- firebase-firestore/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 98a0db29e85..6fdff5182c4 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased -* [changed] Improve the performance of queries in collections that contain many deleted documents. + +- [changed] Improve the performance of queries in collections that contain many deleted documents. [#7295](//github.com/firebase/firebase-android-sdk/issues/7295) # 26.0.0