Remove SimpleQueryEngine (#7038)

Author: schmidt-sebastian

Firestore/Example/Firestore.xcodeproj/project.pbxproj

@@ -32,7 +32,7 @@
 #include "Firestore/core/src/local/local_documents_view.h"
 #include "Firestore/core/src/local/local_store.h"
 #include "Firestore/core/src/local/memory_persistence.h"
-#include "Firestore/core/src/local/simple_query_engine.h"
+#include "Firestore/core/src/local/query_engine.h"
 #include "Firestore/core/src/local/target_data.h"
 #include "Firestore/core/src/model/database_id.h"
 #include "Firestore/core/src/model/document_key.h"
@@ -64,7 +64,7 @@
 using firebase::firestore::local::LocalStore;
 using firebase::firestore::local::MemoryPersistence;
 using firebase::firestore::local::Persistence;
-using firebase::firestore::local::SimpleQueryEngine;
+using firebase::firestore::local::QueryEngine;
 using firebase::firestore::local::TargetData;
 using firebase::firestore::model::BatchId;
 using firebase::firestore::model::DatabaseId;
@@ -226,7 +226,7 @@ @implementation FSTDatastoreTests {
   std::unique_ptr<Persistence> _persistence;
 
   DatabaseInfo _databaseInfo;
-  SimpleQueryEngine _queryEngine;
+  QueryEngine _queryEngine;
 
   std::unique_ptr<ConnectivityMonitor> _connectivityMonitor;
   std::unique_ptr<FirebaseMetadataProvider> _firebaseMetadataProvider;

Firestore/Example/Tests/Integration/FSTDatastoreTests.mm

@@ -36,9 +36,9 @@
 #include "Firestore/core/src/core/listen_options.h"
 #include "Firestore/core/src/core/query_listener.h"
 #include "Firestore/core/src/core/sync_engine.h"
-#include "Firestore/core/src/local/index_free_query_engine.h"
 #include "Firestore/core/src/local/local_store.h"
 #include "Firestore/core/src/local/persistence.h"
+#include "Firestore/core/src/local/query_engine.h"
 #include "Firestore/core/src/model/database_id.h"
 #include "Firestore/core/src/model/document_key.h"
 #include "Firestore/core/src/remote/firebase_metadata_provider.h"
@@ -72,7 +72,7 @@
 using firebase::firestore::core::QueryListener;
 using firebase::firestore::core::SyncEngine;
 using firebase::firestore::core::ViewSnapshot;
-using firebase::firestore::local::IndexFreeQueryEngine;
+using firebase::firestore::local::QueryEngine;
 using firebase::firestore::local::LocalStore;
 using firebase::firestore::local::Persistence;
 using firebase::firestore::local::TargetData;
@@ -198,7 +198,7 @@ @implementation FSTSyncEngineTestDriver {
   std::vector<std::shared_ptr<EventListener<Empty>>> _snapshotsInSyncListeners;
   std::shared_ptr<MockDatastore> _datastore;
 
-  IndexFreeQueryEngine _queryEngine;
+  QueryEngine _queryEngine;
 
   int _snapshotsInSyncEvents;
   int _waitForPendingWritesEvents;

Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm

@@ -31,13 +31,13 @@
 #include "Firestore/core/src/core/query_listener.h"
 #include "Firestore/core/src/core/sync_engine.h"
 #include "Firestore/core/src/core/view.h"
-#include "Firestore/core/src/local/index_free_query_engine.h"
 #include "Firestore/core/src/local/leveldb_opener.h"
 #include "Firestore/core/src/local/leveldb_persistence.h"
 #include "Firestore/core/src/local/local_documents_view.h"
 #include "Firestore/core/src/local/local_serializer.h"
 #include "Firestore/core/src/local/local_store.h"
 #include "Firestore/core/src/local/memory_persistence.h"
+#include "Firestore/core/src/local/query_engine.h"
 #include "Firestore/core/src/local/query_result.h"
 #include "Firestore/core/src/model/database_id.h"
 #include "Firestore/core/src/model/document_set.h"
@@ -72,12 +72,12 @@ using api::SnapshotMetadata;
 using auth::CredentialsProvider;
 using auth::User;
 using firestore::Error;
-using local::IndexFreeQueryEngine;
 using local::LevelDbOpener;
 using local::LocalSerializer;
 using local::LocalStore;
 using local::LruParams;
 using local::MemoryPersistence;
+using local::QueryEngine;
 using local::QueryResult;
 using model::DatabaseId;
 using model::Document;
@@ -197,7 +197,7 @@ void FirestoreClient::Initialize(const User& user, const Settings& settings) {
     persistence_ = MemoryPersistence::WithEagerGarbageCollector();
   }
 
-  query_engine_ = absl::make_unique<IndexFreeQueryEngine>();
+  query_engine_ = absl::make_unique<QueryEngine>();
   local_store_ = absl::make_unique<LocalStore>(persistence_.get(),
                                                query_engine_.get(), user);
   connectivity_monitor_ = ConnectivityMonitor::Create(worker_queue_);

Firestore/core/src/core/firestore_client.cc

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "Firestore/core/src/local/index_free_query_engine.h"
+#include "Firestore/core/src/local/query_engine.h"
 
 #include <utility>
 
@@ -42,15 +42,15 @@ using model::MaybeDocument;
 using model::MaybeDocumentMap;
 using model::SnapshotVersion;
 
-DocumentMap IndexFreeQueryEngine::GetDocumentsMatchingQuery(
+DocumentMap QueryEngine::GetDocumentsMatchingQuery(
     const Query& query,
     const SnapshotVersion& last_limbo_free_snapshot_version,
     const DocumentKeySet& remote_keys) {
   HARD_ASSERT(local_documents_view_, "SetLocalDocumentsView() not called");
 
-  // Queries that match all documents don't benefit from using IndexFreeQueries.
-  // It is more efficient to scan all documents in a collection, rather than to
-  // perform individual lookups.
+  // 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);
   }
@@ -89,8 +89,8 @@ DocumentMap IndexFreeQueryEngine::GetDocumentsMatchingQuery(
   return updated_results;
 }
 
-DocumentSet IndexFreeQueryEngine::ApplyQuery(
-    const Query& query, const MaybeDocumentMap& documents) const {
+DocumentSet QueryEngine::ApplyQuery(const Query& query,
+                                    const MaybeDocumentMap& documents) const {
   // Sort the documents and re-apply the query filter since previously matching
   // documents do not necessarily still match the query.
   DocumentSet query_results(query.Comparator());
@@ -107,7 +107,7 @@ DocumentSet IndexFreeQueryEngine::ApplyQuery(
   return query_results;
 }
 
-bool IndexFreeQueryEngine::NeedsRefill(
+bool QueryEngine::NeedsRefill(
     LimitType limit_type,
     const DocumentSet& sorted_previous_results,
     const DocumentKeySet& remote_keys,
@@ -138,8 +138,7 @@ bool IndexFreeQueryEngine::NeedsRefill(
          document_at_limit_edge->version() > limbo_free_snapshot_version;
 }
 
-DocumentMap IndexFreeQueryEngine::ExecuteFullCollectionScan(
-    const Query& query) {
+DocumentMap QueryEngine::ExecuteFullCollectionScan(const Query& query) {
   LOG_DEBUG("Using full collection scan to execute query: %s",
             query.ToString());
   return local_documents_view_->GetDocumentsMatchingQuery(

Firestore/core/src/local/index_free_query_engine.h

@@ -24,20 +24,31 @@ namespace firestore {
 
 namespace core {
 class Query;
+enum class LimitType;
 }  // namespace core
 
 namespace local {
 
 class LocalDocumentsView;
 
 /**
- * Represents a query engine capable of performing queries over the local
- * document cache. You must call `SetLocalDocumentsView()` before using.
+ * A query engine that takes advantage of the target document mapping in the
+ * TargetCache. Query execution is optimizes by only 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 the same results as full collection scans. In these cases, the
+ * query processing falls back to full scans. These cases are:
+ *
+ * - Limit queries where a document that matched the query previously no
+ *   longer matches the query.
+ * - Limit queries where a document edit may cause the document to sort below
+ *   another document that is in the local cache.
+ * - Queries that have never been CURRENT or free of limbo documents.
  */
 class QueryEngine {
  public:
-  enum Type { Simple, IndexFree };
-
   virtual ~QueryEngine() = default;
 
   /**
@@ -46,16 +57,42 @@ class QueryEngine {
    * The caller owns the LocalDocumentView and must ensure that it outlives the
    * QueryEngine.
    */
-  virtual void SetLocalDocumentsView(LocalDocumentsView* local_documents) = 0;
+  virtual void SetLocalDocumentsView(LocalDocumentsView* local_documents) {
+    local_documents_view_ = local_documents;
+  }
 
   /** Returns all local documents matching the specified query. */
-  virtual model::DocumentMap GetDocumentsMatchingQuery(
+  model::DocumentMap GetDocumentsMatchingQuery(
       const core::Query& query,
       const model::SnapshotVersion& last_limbo_free_snapshot_version,
-      const model::DocumentKeySet& remote_keys) = 0;
+      const model::DocumentKeySet& remote_keys);
+
+ private:
+  /** Applies the query filter and sorting to the provided documents. */
+  model::DocumentSet ApplyQuery(const core::Query& query,
+                                const model::MaybeDocumentMap& documents) const;
+
+  /**
+   * Determines if a limit query needs to be refilled from cache, making it
+   * ineligible for index-free execution.
+   *
+   * @param limit_type The type of limit query for refill calculation.
+   * @param sorted_previous_results The documents that matched the query when it
+   *     was last synchronized, sorted by the query's comparator.
+   * @param remote_keys The document keys that matched the query at the last
+   *     snapshot.
+   * @param limbo_free_snapshot_version The version of the snapshot when the
+   *     query was last synchronized.
+   */
+  bool NeedsRefill(
+      core::LimitType limit_type,
+      const model::DocumentSet& sorted_previous_results,
+      const model::DocumentKeySet& remote_keys,
+      const model::SnapshotVersion& limbo_free_snapshot_version) const;
+
+  model::DocumentMap ExecuteFullCollectionScan(const core::Query& query);
 
-  /** Returns the underlying algorithm used by the query engine. */
-  virtual Type type() const = 0;
+  LocalDocumentsView* local_documents_view_ = nullptr;
 };
 
 }  // namespace local