Index-free: Use different SQL for non-index free queries (#726)

Author: schmidt-sebastian

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

@@ -155,48 +155,59 @@ public ImmutableSortedMap<DocumentKey, Document> getAllDocumentsMatchingQuery(
         (ImmutableSortedMap<DocumentKey, Document>[])
             new ImmutableSortedMap[] {DocumentCollections.emptyDocumentMap()};
 
+    SQLitePersistence.Query sqlQuery;
+    if (sinceReadTime.equals(SnapshotVersion.NONE)) {
+      sqlQuery =
+          db.query("SELECT path, contents FROM remote_documents WHERE path >= ? AND path < ?")
+              .binding(prefixPath, prefixSuccessorPath);
+    } 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 FROM remote_documents WHERE path >= ? AND path < ?"
+                      + "AND (read_time_seconds > ? OR (read_time_seconds = ? AND read_time_nanos > ?))")
+              .binding(
+                  prefixPath,
+                  prefixSuccessorPath,
+                  readTime.getSeconds(),
+                  readTime.getSeconds(),
+                  readTime.getNanoseconds());
+    }
+
     int rowsProcessed =
-        db.query(
-                "SELECT path, contents FROM remote_documents WHERE path >= ? AND path < ? "
-                    + "AND (read_time_seconds IS NULL OR read_time_seconds > ? "
-                    + "OR (read_time_seconds = ? AND read_time_nanos > ?))")
-            .binding(
-                prefixPath,
-                prefixSuccessorPath,
-                readTime.getSeconds(),
-                readTime.getSeconds(),
-                readTime.getNanoseconds())
-            .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;
-                  }
-
-                  byte[] rawDocument = row.getBlob(1);
-
-                  // 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(
-                      () -> {
-                        MaybeDocument maybeDoc = decodeMaybeDocument(rawDocument);
-
-                        if (maybeDoc instanceof Document && query.matches((Document) maybeDoc)) {
-                          synchronized (SQLiteRemoteDocumentCache.this) {
-                            matchingDocuments[0] =
-                                matchingDocuments[0].insert(maybeDoc.getKey(), (Document) maybeDoc);
-                          }
-                        }
-                      });
-                });
+        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;
+              }
+
+              byte[] rawDocument = row.getBlob(1);
+
+              // 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(
+                  () -> {
+                    MaybeDocument maybeDoc = decodeMaybeDocument(rawDocument);
+
+                    if (maybeDoc instanceof Document && query.matches((Document) maybeDoc)) {
+                      synchronized (SQLiteRemoteDocumentCache.this) {
+                        matchingDocuments[0] =
+                            matchingDocuments[0].insert(maybeDoc.getKey(), (Document) maybeDoc);
+                      }
+                    }
+                  });
+            });
 
     statsCollector.recordRowsRead(STATS_TAG, rowsProcessed);
 

firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java

@@ -423,12 +423,17 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() {
         new Object[] {encode(path("coll/new")), 0, 3000, createDummyDocument("coll/new")});
 
     SQLiteRemoteDocumentCache remoteDocumentCache = createRemoteDocumentCache();
+
+    // Verify that queries with SnapshotVersion.NONE return all results, regardless of whether the
+    // read time has been set.
     ImmutableSortedMap<DocumentKey, com.google.firebase.firestore.model.Document> results =
-        remoteDocumentCache.getAllDocumentsMatchingQuery(query("coll"), version(2));
+        remoteDocumentCache.getAllDocumentsMatchingQuery(query("coll"), version(0));
+    assertResultsContain(results, "coll/existing", "coll/old", "coll/current", "coll/new");
 
-    // Verify that queries return the documents that existed prior to the index-free migration, as
-    // well as any documents with a newer read time than the one passed in.
-    assertResultsContain(results, "coll/existing", "coll/new");
+    // Queries that filter by read time only return documents that were written after the index-free
+    // migration.
+    results = remoteDocumentCache.getAllDocumentsMatchingQuery(query("coll"), version(2));
+    assertResultsContain(results, "coll/new");
   }
 
   private SQLiteRemoteDocumentCache createRemoteDocumentCache() {