Remove SimpleQueryEngine

Author: schmidt-sebastian

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java

@@ -21,10 +21,10 @@
 import com.google.firebase.database.collection.ImmutableSortedSet;
 import com.google.firebase.firestore.auth.User;
 import com.google.firebase.firestore.core.OnlineState;
+import com.google.firebase.firestore.local.DefaultQueryEngine;
 import com.google.firebase.firestore.local.LocalStore;
 import com.google.firebase.firestore.local.MemoryPersistence;
 import com.google.firebase.firestore.local.Persistence;
-import com.google.firebase.firestore.local.SimpleQueryEngine;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.mutation.MutationBatchResult;
 import com.google.firebase.firestore.testutil.IntegrationTestUtil;
@@ -74,7 +74,7 @@ public ImmutableSortedSet<DocumentKey> getRemoteKeysForTarget(int targetId) {
         };
 
     FakeConnectivityMonitor connectivityMonitor = new FakeConnectivityMonitor();
-    SimpleQueryEngine queryEngine = new SimpleQueryEngine();
+    DefaultQueryEngine queryEngine = new DefaultQueryEngine();
     Persistence persistence = MemoryPersistence.createEagerGcMemoryPersistence();
     persistence.start();
     LocalStore localStore = new LocalStore(persistence, queryEngine, User.UNAUTHENTICATED);

firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java

@@ -16,8 +16,8 @@
 
 import androidx.annotation.Nullable;
 import com.google.firebase.database.collection.ImmutableSortedSet;
+import com.google.firebase.firestore.local.DefaultQueryEngine;
 import com.google.firebase.firestore.local.GarbageCollectionScheduler;
-import com.google.firebase.firestore.local.IndexFreeQueryEngine;
 import com.google.firebase.firestore.local.LocalStore;
 import com.google.firebase.firestore.local.MemoryPersistence;
 import com.google.firebase.firestore.local.Persistence;
@@ -49,7 +49,7 @@ protected EventManager createEventManager(Configuration configuration) {
   @Override
   protected LocalStore createLocalStore(Configuration configuration) {
     return new LocalStore(
-        getPersistence(), new IndexFreeQueryEngine(), configuration.getInitialUser());
+        getPersistence(), new DefaultQueryEngine(), configuration.getInitialUser());
   }
 
   @Override

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java renamed to firebase-firestore/src/main/java/com/google/firebase/firestore/local/DefaultQueryEngine.java

@@ -27,26 +27,25 @@
 import java.util.Collections;
 import java.util.Map;
 
-// TOOD(b/140938512): Drop SimpleQueryEngine and rename IndexFreeQueryEngine.
-
 /**
- * A query engine that takes advantage of the target document mapping in the TargetCache. The
- * IndexFreeQueryEngine optimizes query execution by only reading the documents that previously
- * matched a query plus any documents that were edited after the query was last listened to.
+ * A query engine that takes advantage of the target document mapping in the TargetCache. Query
+ * execution is optimized by only reading the documents that previously matched a query plus any
+ * documents that were edited after the query was last listened to.
  *
- * <p>There are some cases where Index-Free queries are not guaranteed to produce the same results
- * as full collection scans. In these cases, the IndexFreeQueryEngine falls back to full query
- * processing. These cases are:
+ * <p>There are some cases where this optimization is not guaranteed to produce the same results as
+ * full collection scans. In these cases, query processing falls back to full scans. These cases
+ * are:
  *
  * <ol>
- *   <il>Limit queries where a document that matched the query previously no longer matches the
- *   query. <il> Limit queries where a document edit may cause the document to sort below another
- *   document that is in the local cache. <il>Queries that have never been CURRENT or free of Limbo
- *   documents.
+ *   <li>Limit queries where a document that matched the query previously no longer matches the
+ *       query.
+ *   <li>Limit queries where a document edit may cause the document to sort below another document
+ *       that is in the local cache.
+ *   <li>Queries that have never been CURRENT or free of limbo documents.
  * </ol>
  */
-public class IndexFreeQueryEngine implements QueryEngine {
-  private static final String LOG_TAG = "IndexFreeQueryEngine";
+public class DefaultQueryEngine implements QueryEngine {
+  private static final String LOG_TAG = "DefaultQueryEngine";
 
   private LocalDocumentsView localDocumentsView;
 
@@ -62,7 +61,7 @@ public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery(
       ImmutableSortedSet<DocumentKey> remoteKeys) {
     hardAssert(localDocumentsView != null, "setLocalDocumentsView() not called");
 
-    // Queries that match all documents don't benefit from using IndexFreeQueries. It is more
+    // Queries that match all documents don't benefit from using key-based lookups. It is more
     // efficient to scan all documents in a collection, rather than to perform individual lookups.
     if (query.matchesAllDocuments()) {
       return executeFullCollectionScan(query);

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

@@ -142,9 +142,9 @@ ImmutableSortedMap<DocumentKey, MaybeDocument> getLocalViewOfDocuments(
     return results;
   }
 
-  // TODO: The Querying implementation here should move 100% to SimpleQueryEngine.
+  // 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 SimpleQueryEngine can do that and then filter in
+  // documents in a given collection so that query engine can do that and then filter in
   // memory.
 
   /**

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

@@ -99,8 +99,6 @@ public abstract class LocalStoreTestCase {
   private @Nullable QueryResult lastQueryResult;
   private int lastTargetId;
 
-  abstract QueryEngine getQueryEngine();
-
   abstract Persistence getPersistence();
 
   abstract boolean garbageCollectorIsEager();
@@ -113,7 +111,7 @@ public void setUp() {
     lastTargetId = 0;
 
     localStorePersistence = getPersistence();
-    queryEngine = new CountingQueryEngine(getQueryEngine());
+    queryEngine = new CountingQueryEngine(new DefaultQueryEngine());
     localStore = new LocalStore(localStorePersistence, queryEngine, User.UNAUTHENTICATED);
     localStore.start();
   }
@@ -1023,7 +1021,7 @@ public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransfo
   @Test
   public void testUsesTargetMappingToExecuteQueries() {
     assumeFalse(garbageCollectorIsEager());
-    assumeTrue(queryEngine.getSubject() instanceof IndexFreeQueryEngine);
+    assumeTrue(queryEngine.getSubject() instanceof DefaultQueryEngine);
 
     // This test verifies that once a target mapping has been written, only documents that match
     // the query are read from the RemoteDocumentCache.

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

@@ -14,38 +14,19 @@
 
 package com.google.firebase.firestore.local;
 
-import java.util.Arrays;
-import java.util.Collection;
 import org.junit.runner.RunWith;
-import org.robolectric.ParameterizedRobolectricTestRunner;
+import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
 
-@RunWith(ParameterizedRobolectricTestRunner.class)
+@RunWith(RobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
 public class MemoryLocalStoreTest extends LocalStoreTestCase {
 
-  private QueryEngine queryEngine;
-
-  @ParameterizedRobolectricTestRunner.Parameters(name = "QueryEngine = {0}")
-  public static Collection<Object[]> data() {
-    return Arrays.asList(
-        new Object[] {new SimpleQueryEngine()}, new Object[] {new IndexFreeQueryEngine()});
-  }
-
-  public MemoryLocalStoreTest(QueryEngine queryEngine) {
-    this.queryEngine = queryEngine;
-  }
-
   @Override
   Persistence getPersistence() {
     return PersistenceTestHelpers.createEagerGCMemoryPersistence();
   }
 
-  @Override
-  QueryEngine getQueryEngine() {
-    return this.queryEngine;
-  }
-
   @Override
   boolean garbageCollectorIsEager() {
     return true;

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

@@ -45,7 +45,7 @@
 
 @RunWith(RobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
-public class IndexFreeQueryEngineTest {
+public class QueryEngineTest {
 
   private static final int TEST_TARGET_ID = 1;
 
@@ -72,15 +72,15 @@ public class IndexFreeQueryEngineTest {
   private TargetCache targetCache;
   private QueryEngine queryEngine;
 
-  private @Nullable Boolean expectIndexFreeExecution;
+  private @Nullable Boolean expectFullCollectionScan;
 
   @Before
   public void setUp() {
-    expectIndexFreeExecution = null;
+    expectFullCollectionScan = null;
 
     persistence = MemoryPersistence.createEagerGcMemoryPersistence();
     targetCache = new MemoryTargetCache(persistence);
-    queryEngine = new IndexFreeQueryEngine();
+    queryEngine = new DefaultQueryEngine();
 
     remoteDocumentCache = persistence.getRemoteDocumentCache();
 
@@ -94,8 +94,8 @@ public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery(
               Query query, SnapshotVersion sinceReadTime) {
             assertEquals(
                 "Observed query execution mode did not match expectation",
-                expectIndexFreeExecution,
-                !SnapshotVersion.NONE.equals(sinceReadTime));
+                expectFullCollectionScan,
+                SnapshotVersion.NONE.equals(sinceReadTime));
             return super.getDocumentsMatchingQuery(query, sinceReadTime);
           }
         };
@@ -126,28 +126,28 @@ private void addDocument(Document... docs) {
         });
   }
 
-  private <T> T expectIndexFreeQuery(Callable<T> c) throws Exception {
+  private <T> T expectOptimizedCollectionQuery(Callable<T> c) throws Exception {
     try {
-      expectIndexFreeExecution = true;
+      expectFullCollectionScan = false;
       return c.call();
     } finally {
-      expectIndexFreeExecution = null;
+      expectFullCollectionScan = null;
     }
   }
 
   private <T> T expectFullCollectionQuery(Callable<T> c) throws Exception {
     try {
-      expectIndexFreeExecution = false;
+      expectFullCollectionScan = true;
       return c.call();
     } finally {
-      expectIndexFreeExecution = null;
+      expectFullCollectionScan = null;
     }
   }
 
   private DocumentSet runQuery(Query query, SnapshotVersion lastLimboFreeSnapshotVersion) {
     Preconditions.checkNotNull(
-        expectIndexFreeExecution,
-        "Encountered runQuery() call not wrapped in expectIndexFreeQuery()/expectFullCollectionQuery()");
+        expectFullCollectionScan,
+        "Encountered runQuery() call not wrapped in expectOptimizedCollectionQuery()/expectFullCollectionQuery()");
     ImmutableSortedMap<DocumentKey, Document> docs =
         queryEngine.getDocumentsMatchingQuery(
             query,
@@ -166,7 +166,8 @@ public void usesTargetMappingForInitialView() throws Exception {
     addDocument(MATCHING_DOC_A, MATCHING_DOC_B);
     persistQueryMapping(MATCHING_DOC_A.getKey(), MATCHING_DOC_B.getKey());
 
-    DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
+    DocumentSet docs =
+        expectOptimizedCollectionQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
     assertEquals(docSet(query.comparator(), MATCHING_DOC_A, MATCHING_DOC_B), docs);
   }
 
@@ -180,7 +181,8 @@ public void filtersNonMatchingInitialResults() throws Exception {
     // Add a mutated document that is not yet part of query's set of remote keys.
     addDocument(PENDING_NON_MATCHING_DOC_A);
 
-    DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
+    DocumentSet docs =
+        expectOptimizedCollectionQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
     assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs);
   }
 
@@ -191,12 +193,13 @@ public void includesChangesSinceInitialResults() throws Exception {
     addDocument(MATCHING_DOC_A, MATCHING_DOC_B);
     persistQueryMapping(MATCHING_DOC_A.getKey(), MATCHING_DOC_B.getKey());
 
-    DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
+    DocumentSet docs =
+        expectOptimizedCollectionQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
     assertEquals(docSet(query.comparator(), MATCHING_DOC_A, MATCHING_DOC_B), docs);
 
     addDocument(UPDATED_MATCHING_DOC_B);
 
-    docs = expectIndexFreeQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
+    docs = expectOptimizedCollectionQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
     assertEquals(docSet(query.comparator(), MATCHING_DOC_A, UPDATED_MATCHING_DOC_B), docs);
   }
 
@@ -343,7 +346,8 @@ public void limitQueriesUseInitialResultsIfLastDocumentInLimitIsUnchanged() thro
     // Since the last document in the limit didn't change (and hence we know that all documents
     // written prior to query execution still sort after "coll/b"), we should use an Index-Free
     // query.
-    DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
+    DocumentSet docs =
+        expectOptimizedCollectionQuery(() -> runQuery(query, LAST_LIMBO_FREE_SNAPSHOT));
     assertEquals(
         docSet(
             query.comparator(),

firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java renamed to firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryEngineTest.java

@@ -14,33 +14,14 @@
 
 package com.google.firebase.firestore.local;
 
-import java.util.Arrays;
-import java.util.Collection;
 import org.junit.runner.RunWith;
-import org.robolectric.ParameterizedRobolectricTestRunner;
+import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
 
-@RunWith(ParameterizedRobolectricTestRunner.class)
+@RunWith(RobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
 public class SQLiteLocalStoreTest extends LocalStoreTestCase {
 
-  private QueryEngine queryEngine;
-
-  @ParameterizedRobolectricTestRunner.Parameters(name = "QueryEngine = {0}")
-  public static Collection<Object[]> data() {
-    return Arrays.asList(
-        new Object[] {new SimpleQueryEngine()}, new Object[] {new IndexFreeQueryEngine()});
-  }
-
-  public SQLiteLocalStoreTest(QueryEngine queryEngine) {
-    this.queryEngine = queryEngine;
-  }
-
-  @Override
-  QueryEngine getQueryEngine() {
-    return this.queryEngine;
-  }
-
   @Override
   Persistence getPersistence() {
     return PersistenceTestHelpers.createSQLitePersistence();