Review comments

Author: schmidt-sebastian

packages/firestore/src/core/sync_engine.ts

@@ -246,11 +246,10 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
     const remoteKeys = await this.localStore.remoteDocumentKeys(
       queryData.targetId
     );
-    const docs = await this.localStore.executeQuery(
-      query,
+    const docs = await this.localStore.executeQuery(query, {
       queryData,
       remoteKeys
-    );
+    });
 
     const view = new View(query, remoteKeys);
     const viewDocChanges = view.computeDocChanges(docs);
@@ -290,11 +289,10 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
     const remoteKeys = await this.localStore.remoteDocumentKeys(
       queryData.targetId
     );
-    const docs = await this.localStore.executeQuery(
-      queryData.query,
+    const docs = await this.localStore.executeQuery(queryData.query, {
       queryData,
       remoteKeys
-    );
+    });
     const viewSnapshot = view.synchronizeWithPersistedState(docs, remoteKeys);
     if (this.isPrimary) {
       this.updateTrackedLimbos(queryData.targetId, viewSnapshot.limboChanges);
@@ -793,7 +791,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
             // to re-run the query against the local store to make sure we
             // didn't lose any good docs that had been past the limit.
             return this.localStore
-              .executeQuery(queryView.query, null, documentKeySet())
+              .executeQuery(queryView.query, { needsRefill: true })
               .then(docs => {
                 return queryView.view.computeDocChanges(docs, viewDocChanges);
               });

packages/firestore/src/local/index_free_query_engine.ts

@@ -28,32 +28,25 @@ import { assert } from '../util/assert';
 import { debug, getLogLevel, LogLevel } from '../util/log';
 import { SortedSet } from '../util/sorted_set';
 
+// TOOD(b/140938512): Drop SimpleQueryEngine and rename IndexFreeQueryEngine.
+
 /**
  * A query engine that takes advantage of the target document mapping in the
  * QueryCache. The IndexFreeQueryEngine optimizes query execution by only
- * reading the documents previously matched a query plus any documents that were
+ * reading the documents that previously matched a query plus any documents that were
  * edited after the query was last listened to.
  *
  * There are some cases where Index-Free queries are not guaranteed to produce
- * to the same results as full collection scans. In these case, the
- * IndexFreeQueryEngine falls back to a full query processing. These cases are:
+ * the same results as full collection scans. In these cases, the
+ * IndexFreeQueryEngine falls back to full query processing. These cases are:
  *
  * - Limit queries where a document that matched the query previously no longer
- * matches the query. In this case, we have to scan all local documents since a
- * document that was sent to us as part of a different query result may now fall
- * into the limit.
+ *   matches the query.
  *
- * - Limit queries that include edits that occurred after the last remote
- * snapshot (both latency-compensated or committed). Even if an edited document
- * continues to match the query, an edit may cause a document to sort below
- * another document that is in the local cache.
+ * - Limit queries where a document edit may cause the document to sort below
+ *   another document that is in the local cache.
  *
- * - Queries where the last snapshot contained Limbo documents. Even though a
- * Limbo document is not part of the backend result set, we need to include
- * Limbo documents in local views to ensure consistency between different Query
- * views. If there exists a previous query snapshot that contained no limbo
- * documents, we can instead use the older snapshot version for Index-Free
- * processing.
+ * - Queries that have never been CURRENT or free of Limbo documents.
  */
 export class IndexFreeQueryEngine implements QueryEngine {
   private localDocumentsView: LocalDocumentsView | undefined;
@@ -89,7 +82,7 @@ export class IndexFreeQueryEngine implements QueryEngine {
       return this.executeFullCollectionScan(transaction, query);
     }
 
-    return this.getSortedPreviousResults(transaction, query, remoteKeys).next(
+    return this.getSortedDocuments(transaction, query, remoteKeys).next(
       previousResults => {
         if (
           query.hasLimit() &&
@@ -131,21 +124,20 @@ export class IndexFreeQueryEngine implements QueryEngine {
    * Returns the documents for the specified remote keys if they still match the
    * query, sorted by the query's comparator.
    */
-  getSortedPreviousResults(
+  private getSortedDocuments(
     transaction: PersistenceTransaction,
-    query: Query,
+    sortByQuery: Query,
     remoteKeys: DocumentKeySet
   ): PersistencePromise<SortedSet<Document>> {
-    // Fetch the documents that matched the query at the last snapshot.
     return this.localDocumentsView!.getDocuments(transaction, remoteKeys).next(
       previousResults => {
         // Sort the documents and re-apply the query filter since previously
         // matching documents do not necessarily still match the query.
         let results = new SortedSet<Document>((d1, d2) =>
-          query.docComparator(d1, d2)
+          sortByQuery.docComparator(d1, d2)
         );
         previousResults.forEach((_, maybeDoc) => {
-          if (maybeDoc instanceof Document && query.matches(maybeDoc)) {
+          if (maybeDoc instanceof Document && sortByQuery.matches(maybeDoc)) {
             results = results.add(maybeDoc);
           }
         });
@@ -166,7 +158,7 @@ export class IndexFreeQueryEngine implements QueryEngine {
    * @param limboFreeSnapshotVersion The version of the snapshot when the query
    * was last synchronized.
    */
-  needsRefill(
+  private needsRefill(
     sortedPreviousResults: SortedSet<Document>,
     remoteKeys: DocumentKeySet,
     limboFreeSnapshotVersion: SnapshotVersion
@@ -177,12 +169,6 @@ export class IndexFreeQueryEngine implements QueryEngine {
       return true;
     }
 
-    // We don't need to find a better match from cache if no documents matched
-    // the query.
-    if (sortedPreviousResults.isEmpty()) {
-      return false;
-    }
-
     // Limit queries are not eligible for index-free query execution if there is
     // a potential that an older document from cache now sorts before a document
     // that was previously part of the limit. This, however, can only happen if
@@ -192,14 +178,18 @@ export class IndexFreeQueryEngine implements QueryEngine {
     // documents from cache will continue to be "rejected" by this boundary.
     // Therefore, we can ignore any modifications that don't affect the last
     // document.
-    const lastDocumentInLimit = sortedPreviousResults.last()!;
+    const lastDocumentInLimit = sortedPreviousResults.last();
+    if (!lastDocumentInLimit) {
+      // We don't need to refill the query if there were already no documents.
+      return false;
+    }
     return (
       lastDocumentInLimit.hasPendingWrites ||
       lastDocumentInLimit.version.compareTo(limboFreeSnapshotVersion) > 0
     );
   }
 
-  executeFullCollectionScan(
+  private executeFullCollectionScan(
     transaction: PersistenceTransaction,
     query: Query
   ): PersistencePromise<DocumentMap> {

packages/firestore/src/local/local_store.ts

@@ -830,54 +830,67 @@ export class LocalStore {
   }
 
   /**
-   * Runs the specified query against all the documents in the local store and
-   * returns the results.
+   * Runs the specified query against the local store and returns the results,
+   * potentially taking advantage of query data from previous executions (such
+   * as the set of remote keys).
    */
   executeQuery(query: Query): Promise<DocumentMap>;
+  /**
+   * Performs a full collection scan for the provided query and returns the
+   * results. Does not take into account metadata from prior executions.
+   */
+  executeQuery(
+    query: Query,
+    options: { needsRefill: true }
+  ): Promise<DocumentMap>;
   /**
    * Runs the specified query against the local store and returns the results,
    * potentially taking advantage of the provided query data and the set of
    * remote document keys.
    */
   executeQuery(
     query: Query,
-    queryData: QueryData | null,
-    remoteKeys: DocumentKeySet
+    options: { queryData: QueryData; remoteKeys: DocumentKeySet }
   ): Promise<DocumentMap>;
   executeQuery(
     query: Query,
-    queryData?: QueryData | null,
-    remoteKeys?: DocumentKeySet
+    options: {
+      needsRefill?: boolean;
+      queryData?: QueryData;
+      remoteKeys?: DocumentKeySet;
+    } = {}
   ): Promise<DocumentMap> {
-    let cachedQueryData = queryData;
-    let cachedRemoteKeys = remoteKeys;
+    const allowLookup = options.needsRefill !== false;
+
+    let queryData = options.queryData;
+    let remoteKeys = options.remoteKeys;
 
     return this.persistence.runTransaction('Execute query', 'readonly', txn => {
       return PersistencePromise.resolve()
         .next(() => {
-          // If QueryData is not provided (and not explicitly set to `null`), we
-          // retrieve the query data and the remote keys from cache.
-          if (cachedQueryData === undefined) {
-            return this.getQueryData(txn, query).next(queryData => {
-              if (queryData) {
-                return this.queryCache
-                  .getMatchingKeysForTargetId(txn, queryData.targetId)
-                  .next(remoteKeys => {
-                    cachedQueryData = queryData;
-                    cachedRemoteKeys = remoteKeys;
-                  });
-              }
+          if (allowLookup && !queryData) {
+            return this.getQueryData(txn, query).next(result => {
+              queryData = result || undefined;
             });
           }
         })
-        .next(() =>
-          this.queryEngine.getDocumentsMatchingQuery(
+        .next(() => {
+          if (allowLookup && queryData && !remoteKeys) {
+            return this.queryCache
+              .getMatchingKeysForTargetId(txn, queryData.targetId)
+              .next(result => {
+                remoteKeys = result;
+              });
+          }
+        })
+        .next(() => {
+          return this.queryEngine.getDocumentsMatchingQuery(
             txn,
             query,
-            cachedQueryData || null,
-            cachedRemoteKeys || documentKeySet()
-          )
-        );
+            queryData || null,
+            remoteKeys || documentKeySet()
+          );
+        });
     });
   }