More feedback

Author: schmidt-sebastian

packages/firestore/src/local/index_free_query_engine.ts

@@ -22,7 +22,11 @@ import { PersistencePromise } from './persistence_promise';
 import { QueryData } from './query_data';
 import { Query } from '../core/query';
 import { SnapshotVersion } from '../core/snapshot_version';
-import { DocumentKeySet, DocumentMap } from '../model/collections';
+import {
+  DocumentKeySet,
+  DocumentMap,
+  MaybeDocumentMap
+} from '../model/collections';
 import { Document } from '../model/document';
 import { assert } from '../util/assert';
 import { debug, getLogLevel, LogLevel } from '../util/log';
@@ -82,8 +86,10 @@ export class IndexFreeQueryEngine implements QueryEngine {
       return this.executeFullCollectionScan(transaction, query);
     }
 
-    return this.getSortedDocuments(transaction, query, remoteKeys).next(
-      previousResults => {
+    return this.localDocumentsView!.getDocuments(transaction, remoteKeys).next(
+      documents => {
+        const previousResults = this.applyQuery(query, documents);
+
         if (
           query.hasLimit() &&
           this.needsRefill(
@@ -111,6 +117,9 @@ export class IndexFreeQueryEngine implements QueryEngine {
           query,
           queryData.lastLimboFreeSnapshotVersion
         ).next(updatedResults => {
+          // We merge `previousResults` into `updateResults`, since
+          // `updateResults` is already a DocumentMap. If a document is
+          // contained in both lists, then its contents are the same.
           previousResults.forEach(doc => {
             updatedResults = updatedResults.insert(doc.key, doc);
           });
@@ -120,31 +129,22 @@ 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.
-   */
-  private getSortedDocuments(
-    transaction: PersistenceTransaction,
-    sortByQuery: Query,
-    remoteKeys: DocumentKeySet
-  ): PersistencePromise<SortedSet<Document>> {
-    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) =>
-          sortByQuery.docComparator(d1, d2)
-        );
-        previousResults.forEach((_, maybeDoc) => {
-          if (maybeDoc instanceof Document && sortByQuery.matches(maybeDoc)) {
-            results = results.add(maybeDoc);
-          }
-        });
-
-        return results;
-      }
+  /** Applies the query filter and sorting to the provided documents.  */
+  private applyQuery(
+    query: Query,
+    documents: MaybeDocumentMap
+  ): SortedSet<Document> {
+    // Sort the documents and re-apply the query filter since previously
+    // matching documents do not necessarily still match the query.
+    let queyrResults = new SortedSet<Document>((d1, d2) =>
+      query.docComparator(d1, d2)
     );
+    documents.forEach((_, maybeDoc) => {
+      if (maybeDoc instanceof Document && query.matches(maybeDoc)) {
+        queyrResults = queyrResults.add(maybeDoc);
+      }
+    });
+    return queyrResults;
   }
 
   /**

packages/firestore/test/unit/local/index_free_query_engine.test.ts

@@ -38,14 +38,8 @@ import { DocumentKey } from '../../../src/model/document_key';
 import { DocumentSet } from '../../../src/model/document_set';
 import { assert } from '../../../src/util/assert';
 import { testMemoryEagerPersistence } from './persistence_test_helpers';
-import {
-  doc,
-  filter,
-  key,
-  orderBy,
-  path,
-  queryData as testQueryData
-} from '../../util/helpers';
+import { doc, filter, key, orderBy, path, version } from '../../util/helpers';
+import { ListenSequence } from '../../../src/core/listen_sequence';
 
 const TEST_TARGET_ID = 1;
 
@@ -183,13 +177,7 @@ describe('IndexFreeQueryEngine', () => {
     const query = Query.atPath(path('coll')).addFilter(
       filter('matches', '==', true)
     );
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ true
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ true);
 
     await addDocument(MATCHING_DOC_A, MATCHING_DOC_B);
     await persistQueryMapping(MATCHING_DOC_A.key, MATCHING_DOC_B.key);
@@ -203,13 +191,7 @@ describe('IndexFreeQueryEngine', () => {
     const query = Query.atPath(path('coll')).addFilter(
       filter('matches', '==', true)
     );
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ true
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ true);
 
     await addDocument(MATCHING_DOC_A, MATCHING_DOC_B);
     await persistQueryMapping(MATCHING_DOC_A.key, MATCHING_DOC_B.key);
@@ -226,13 +208,7 @@ describe('IndexFreeQueryEngine', () => {
     const query = Query.atPath(path('coll')).addFilter(
       filter('matches', '==', true)
     );
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ true
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ true);
 
     await addDocument(MATCHING_DOC_A, MATCHING_DOC_B);
     await persistQueryMapping(MATCHING_DOC_A.key, MATCHING_DOC_B.key);
@@ -251,13 +227,7 @@ describe('IndexFreeQueryEngine', () => {
     const query = Query.atPath(path('coll')).addFilter(
       filter('matches', '==', true)
     );
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ false
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ false);
 
     const docs = await expectFullCollectionQuery(() =>
       runQuery(query, queryData)
@@ -267,13 +237,7 @@ describe('IndexFreeQueryEngine', () => {
 
   it('does not use initial results for unfiltered collection query', async () => {
     const query = Query.atPath(path('coll'));
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ true
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ true);
 
     const docs = await expectFullCollectionQuery(() =>
       runQuery(query, queryData)
@@ -294,13 +258,7 @@ describe('IndexFreeQueryEngine', () => {
 
     await addDocument(MATCHING_DOC_B);
 
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ true
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ true);
     const docs = await expectFullCollectionQuery(() =>
       runQuery(query, queryData)
     );
@@ -319,13 +277,7 @@ describe('IndexFreeQueryEngine', () => {
     await addDocument(PENDING_MATCHING_DOC_A);
     await persistQueryMapping(PENDING_MATCHING_DOC_A.key);
 
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ true
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ true);
 
     await addDocument(MATCHING_DOC_B);
 
@@ -347,13 +299,7 @@ describe('IndexFreeQueryEngine', () => {
     await addDocument(UPDATED_DOC_A);
     await persistQueryMapping(UPDATED_DOC_A.key);
 
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ true
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ true);
 
     await addDocument(MATCHING_DOC_B);
 
@@ -372,13 +318,7 @@ describe('IndexFreeQueryEngine', () => {
     await addDocument(doc('coll/b', 1, { order: 3 }));
     await persistQueryMapping(key('coll/a'), key('coll/b'));
 
-    const queryData = testQueryData(
-      2,
-      QueryPurpose.Listen,
-      'coll',
-      10,
-      /* hasLimboFreeSnapshot= */ true
-    );
+    const queryData = testQueryData(query, /* hasLimboFreeSnapshot= */ true);
 
     // Update "coll/a" but make sure it still sorts before "coll/b"
     await addDocument(
@@ -412,3 +352,14 @@ function verifyResult(
     'Result count does not match'
   );
 }
+
+function testQueryData(query: Query, hasLimboFreeSnapshot: boolean): QueryData {
+  return new QueryData(
+    query,
+    TEST_TARGET_ID,
+    QueryPurpose.Listen,
+    ListenSequence.INVALID,
+    version(10),
+    hasLimboFreeSnapshot ? version(10) : SnapshotVersion.MIN
+  );
+}