Add IndexFreeQueryEngine #2169

schmidt-sebastian · 2019-09-11T16:26:05Z

This PR ports the current state of https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java and https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java, as well as the updated plumbing for executeQuery() (which allows us to pass in the remoteKeys if we already have them, since we don't want to fetch them twice).

This is the last PR for Index-Free queries that adds logic to the client. There will be one more PR that adds new tests to the LocalStore.

mikelehen

I failed to find any bugs, but I did not let that stop me from complaining!

mikelehen · 2019-09-11T18:28:25Z

packages/firestore/src/core/sync_engine.ts

-              return queryView.view.computeDocChanges(docs, viewDocChanges);
-            });
+            return this.localStore
+              .executeQuery(queryView.query, null, documentKeySet())


It looks like queryData / remoteKeys are optional, so you can just leave them off here?

Passing null makes sure we don't look up any QueryData, essentially disabling Index-Free execution. I agree that this is pretty subtle here. On Android it is slightly more obvious since we have two overloads, but in general I agree that we should probably be more explicit (more on that below).

mikelehen · 2019-09-11T18:30:40Z

packages/firestore/src/local/index_free_query_engine.ts

+/**
+ * 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


"documents previously" => "documents that previously"

mikelehen · 2019-09-11T18:31:04Z

packages/firestore/src/local/index_free_query_engine.ts

+ * 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


"produce to the same" => "produce the same"
"these case" => "these cases" or "this case"

mikelehen · 2019-09-11T18:31:27Z

packages/firestore/src/local/index_free_query_engine.ts

+ *
+ * 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:


"to a full query processing" => "to full query processing" (or something)

Fixed. Sorry :/

mikelehen · 2019-09-11T18:32:48Z

packages/firestore/src/local/index_free_query_engine.ts

+ * - 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.


"a document that was sent to us as part of a different query result" should really just be "a cached document", I think... It could have been cached in a variety of ways (e.g. it used to match this query)

That's true. I used "another cached document".

mikelehen · 2019-09-11T22:13:55Z

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

+const UPDATED_MATCHING_DOC_B = doc('coll/b', 11, { matches: true, order: 2 });
+
+/**
+ * A LocalDocumentsView that inspects the arguments to


Maybe add "wrapper" to this description.

mikelehen · 2019-09-11T22:17:39Z

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

+    verifyResult(docs, [MATCHING_DOC_A, MATCHING_DOC_B]);
+  });
+
+  it('filters non-matching initial results', async () => {


I'm not sure what "initial results" refers to... should this be "filters non-matching changes since initial results" by chance?

Yes. Renamed.

mikelehen · 2019-09-11T22:29:27Z

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

+      .withLimit(1);
+
+    // Add a query mapping for a document that matches, but that sorts below
+    // another document based due to an update that the SDK received after the


"based due"?

mikelehen · 2019-09-11T22:31:50Z

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

+    version(10),
+    hasLimboFreeSnapshot ? version(10) : SnapshotVersion.MIN
+  );
+}


Why are these helpers at the bottom instead of the top?

They don't need any state from the test and I thought this would be a good place to hide them.

~~I did end up removing testQueryData and am using the queryData from helpers.ts instead.~~ Update: The test no longer needs to use QueryData since the API only requires the lastLimboFreeSnapshotVersion.

mikelehen · 2019-09-11T22:35:24Z

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

@@ -73,7 +73,7 @@ import {
 } from '../../util/helpers';

 import { FieldValue, IntegerValue } from '../../../src/model/field_value';
-import { CountingQueryEngine } from './forwarding_counting_query_engine';
+import { CountingQueryEngine } from './counting_query_engine';


schmidt-sebastian

Thanks for your thorough review. Looks like I have to do some comment cleanup in Android.

schmidt-sebastian · 2019-09-12T17:32:44Z

packages/firestore/src/core/sync_engine.ts

-              return queryView.view.computeDocChanges(docs, viewDocChanges);
-            });
+            return this.localStore
+              .executeQuery(queryView.query, null, documentKeySet())


Passing null makes sure we don't look up any QueryData, essentially disabling Index-Free execution. I agree that this is pretty subtle here. On Android it is slightly more obvious since we have two overloads, but in general I agree that we should probably be more explicit (more on that below).

schmidt-sebastian · 2019-09-12T17:42:19Z

packages/firestore/src/local/index_free_query_engine.ts

+
+/**
+ * A query engine that takes advantage of the target document mapping in the
+ * QueryCache. The IndexFreeQueryEngine optimizes query execution by only


What do you think of "ThreadLocalFreeQueryEngine"? It sounds even more advanced and the implementation is actually free of them. :)

In all honesty, I can't come up with a better name right now. I filed b/140938512 and will tackle this once we remove SimpleQueryEngine.

schmidt-sebastian · 2019-09-12T17:42:31Z

packages/firestore/src/local/index_free_query_engine.ts

+/**
+ * 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


schmidt-sebastian · 2019-09-12T17:42:57Z

packages/firestore/src/local/index_free_query_engine.ts

+ * 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


schmidt-sebastian · 2019-09-12T17:43:15Z

packages/firestore/src/local/index_free_query_engine.ts

+ *
+ * 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:


Fixed. Sorry :/

schmidt-sebastian · 2019-09-12T18:38:51Z

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

+    verifyResult(docs, [MATCHING_DOC_A, MATCHING_DOC_B]);
+  });
+
+  it('filters non-matching initial results', async () => {


Yes. Renamed.

schmidt-sebastian · 2019-09-12T18:39:20Z

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

+      .withLimit(1);
+
+    // Add a query mapping for a document that matches, but that sorts below
+    // another document based due to an update that the SDK received after the


schmidt-sebastian · 2019-09-12T18:58:48Z

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

+    version(10),
+    hasLimboFreeSnapshot ? version(10) : SnapshotVersion.MIN
+  );
+}


They don't need any state from the test and I thought this would be a good place to hide them.

~~I did end up removing testQueryData and am using the queryData from helpers.ts instead.~~ Update: The test no longer needs to use QueryData since the API only requires the lastLimboFreeSnapshotVersion.

schmidt-sebastian · 2019-09-12T21:56:16Z

packages/firestore/src/local/local_store.ts

+  executeQuery(
+    query: Query,
+    queryData: QueryData | null,
+    remoteKeys: DocumentKeySet


Let me discuss the least important issue here first:

One of my goals for this API was to be relatively implementation agnostic. That's why I opted to pass in QueryData rather than just what is needed.

As for the rest of the API, there is indeed a bit too much magic here. I tried to come up with a better interaction model when I worked on the Android implementation. The other ideas that I played with at the time did not win me over (having a second refillQuery() method or explicitly changing the query data in a secondary call to remove the last limbo free snapshot version). That's how I ended up with this magic.

The "magic" provides with the following functionality:

A way to refill the query.

A way to let FirestoreClient use Index-Free, even though it does not have access to any state from LocalStore.

We don't need to perform two separate lookups for remoteKeys.

For now, I updated the PR to be more explicit. I now have three separate signatures:

/** * 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, options: { queryData: QueryData; remoteKeys: DocumentKeySet } ): Promise<DocumentMap>;

schmidt-sebastian · 2019-09-12T21:58:26Z

packages/firestore/src/local/local_store.ts

+      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.


Done (as shown above).

mikelehen

A few nits in addition to my main concern about the executeQuery() signature. Feel free to ping me if you want to chat through alternatives or something.

mikelehen · 2019-09-13T18:37:41Z

packages/firestore/src/local/index_free_query_engine.ts

+   * Returns the documents for the specified remote keys if they still match the
+   * query, sorted by the query's comparator.
+   */
+  getSortedPreviousResults(


I agree that the sorting is crucial, but I also think it's crucial for executeQuery...() methods to return sorted results in general, so I'm not completely convinced it's necessary to call out the sorting in the method name... especially since the filtering is also crucial but is not called out (and is in fact deemphasized by renaming query to sortByQuery when it's also being used for filtering...)

I think executeQueryAgainstDocuments() is my preference at this point. It's not a huge deal though, so if you're not won over, you can leave as-is.

mikelehen · 2019-09-13T18:46:39Z

packages/firestore/src/local/index_free_query_engine.ts

+          previousResults.forEach(doc => {
+            updatedResults = updatedResults.insert(doc.key, doc);
+          });
+          return updatedResults;


Oh! I see. Probably not a big deal, but a comment would still save me some head-scratching. Or you could add an "optimization" that also removes the concern I had:

if (updatedResults.get(doc.key) === null) { updatedResults = updatedResults.insert(doc.key, doc); }

I also don't care too strongly if you want to leave it as-is though.

mikelehen · 2019-09-13T18:52:47Z

packages/firestore/src/local/index_free_query_engine.ts

+
+/**
+ * A query engine that takes advantage of the target document mapping in the
+ * QueryCache. The IndexFreeQueryEngine optimizes query execution by only


The better name I am hoping for eventually is "QueryEngine" 😄

mikelehen · 2019-09-13T18:54:18Z

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

+    });
+  }
+
+  beforeEach(async () => {


... or just ignore the feedback. That's fine too. 😛

mikelehen · 2019-09-13T18:58:17Z

packages/firestore/src/local/index_free_query_engine.ts

    remoteKeys: DocumentKeySet
  ): PersistencePromise<SortedSet<Document>> {
-    // Fetch the documents that matched the query at the last snapshot.
    return this.localDocumentsView!.getDocuments(transaction, remoteKeys).next(
      previousResults => {


rename previousResults since the method name / args no longer refer to "previous"

I left it as is since the name seems significant in the context of this method.

mikelehen · 2019-09-13T19:03:01Z

packages/firestore/src/local/local_store.ts

+  executeQuery(
+    query: Query,
+    queryData: QueryData | null,
+    remoteKeys: DocumentKeySet


FWIW (you're still free to push back / suggest alternatives) - Fleshing out my proposal a bit more, I think I'd maybe change CachedResultsInfo.lookup(targetId) to CachedResultsInfo.unknown(targetId) and add CachedResultsInfo.None, which would be the default behavior if no CachedResultsInfo was passed.

mikelehen · 2019-09-13T19:05:46Z

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

-    version(10),
-    hasLimboFreeSnapshot ? version(10) : SnapshotVersion.MIN
-  );
-}


I don't care super strongly, but as a general rule I hate test boilerplate and so I preferred having the specialized helper. Was there a reason to remove it?

Updated: Helper no longer needed.

schmidt-sebastian · 2019-09-13T19:16:37Z

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

+    });
+  }
+
+  beforeEach(async () => {


See below: "They don't need any state from the test and I thought this would be a good place to hide them." :)

schmidt-sebastian · 2019-09-13T20:53:06Z

packages/firestore/src/local/index_free_query_engine.ts

+          previousResults.forEach(doc => {
+            updatedResults = updatedResults.insert(doc.key, doc);
+          });
+          return updatedResults;


I added a comment, but left the code as is.

schmidt-sebastian · 2019-09-13T21:28:47Z

packages/firestore/src/local/index_free_query_engine.ts

+   * Returns the documents for the specified remote keys if they still match the
+   * query, sorted by the query's comparator.
+   */
+  getSortedPreviousResults(


The main executeQuery() does not return sorted results, but rather returns a DocumentMap keyed by document name. Because I spent a ton of time thinking about executeQuery() recently, to me the sorting aspect is not as obvious.

Either way, I think I managed to address both of our concerns. I removed the offending method and am not fetching the results in getDocumentsMatchingQuery(). The sorting and filtering now takes place in applyQuery.

schmidt-sebastian · 2019-09-13T21:29:33Z

packages/firestore/src/local/index_free_query_engine.ts

    remoteKeys: DocumentKeySet
  ): PersistencePromise<SortedSet<Document>> {
-    // Fetch the documents that matched the query at the last snapshot.
    return this.localDocumentsView!.getDocuments(transaction, remoteKeys).next(
      previousResults => {


I left it as is since the name seems significant in the context of this method.

schmidt-sebastian · 2019-09-13T21:33:18Z

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

-    version(10),
-    hasLimboFreeSnapshot ? version(10) : SnapshotVersion.MIN
-  );
-}


Updated: Helper no longer needed.

schmidt-sebastian · 2019-09-17T00:27:30Z

packages/firestore/src/core/firestore_client.ts

-        ).snapshot!;
-      });
+    return this.asyncQueue.enqueue(async () => {
+      const queryResult = await this.localStore.executeQuery(


Updated this to all run on the async queue.

schmidt-sebastian · 2019-09-17T00:28:08Z

packages/firestore/src/core/firestore_client.ts

+        query,
+        /* usePreviousResults= */ true
+      );
+      const view = new View(query, queryResult.remoteKeys);


queryResult.remoteKeys is used now instead of null, which seems more correct, but doesn't change the state of the fromCache flag since it is set to true either way.

schmidt-sebastian · 2019-09-17T00:34:08Z

packages/firestore/src/local/local_store.ts

+  executeQuery(
+    query: Query,
+    queryData: QueryData | null,
+    remoteKeys: DocumentKeySet


I updated this based on our offline discussion:

executeQuery() now returns the remoteKeys, which means that there are now only two cases this method needs to address (Index-Free: yes or no). We don't need to pass remote keys since the method returns them. As discussed, this adds slight overhead to limit query refills.

This change also solves a potential multi-tab race since the set of remote keys and the set of remote documents are now fetched in a single transaction.

I also changed the QueryEngine to only take the lastLimbFreeSnapshotVersion instead of QueryData.

schmidt-sebastian · 2019-09-17T00:36:26Z

packages/firestore/src/core/sync_engine.ts

-            });
+            return this.localStore
+              .executeQuery(queryView.query, /* usePreviousResults= */ false)
+              .then(({ documents }) => {


Note: I verified that the transpiler unpacks this (in cjs and esm mode).

mikelehen

LGTM with minor request...

mikelehen · 2019-09-17T18:49:51Z

packages/firestore/src/local/local_store.ts

-      const cachedQueryData = this.queryDataByTarget[queryData!.targetId];
-      return cachedQueryData || queryData;
-    });
+    );


Hrm... We are trading a "slow" O(1) operation [or probably O(log(n) I guess] for a "fast" O(n) operation. I know this is only an in-memory O(n) scan and n probably won't be huge most of the time... but it means adding n listeners will do n^2 operations and performance will degrade as you add each additional listener, which is not how the SDK is meant to scale (doing 1000 listeners should be ~1000 times more work than 1 listener, not ~1,000,000 times).

I don't know for sure how big n needs to be to matter, but in general I would prefer to just use the right data structures so that we get the computational complexity that we expect. So I think the right thing to do would be to add a "reverse" queryDataByQuery (or targetIdByQuery) ObjectMap so that we can do a fast lookup.

WDYT? At minimum, I think this would deserve a TODO(perf) comment to flag that we're doing an unnecessary O(n) operation here that could be optimized away.

I'll merge this as is and will send you a follow-up PR soonish. My gut feeling is that n has to be fairly large to be slower than an IndexedDb lookup. This is mostly based on how much faster our Memory persistence layer is in compared to our IndexedDb-backed implementation.

mikelehen · 2019-09-17T18:49:59Z

packages/firestore/src/local/local_store.ts

-        return null;
+    let queryData: QueryData | null = null;
+
+    // Look up the query data in our local map first, as this avoid a potential


avoid=>avoids

schmidt-sebastian added 2 commits September 11, 2019 09:23

Add IndexFreeQueryEngine

43e954e

Sort imports

d9879af

schmidt-sebastian requested a review from mikelehen September 11, 2019 16:29

schmidt-sebastian added the api: firestore label Sep 11, 2019

Rewrap comments

34bbd31

schmidt-sebastian assigned mikelehen Sep 11, 2019

mikelehen suggested changes Sep 11, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Sep 11, 2019

Review comments

4dda69e

schmidt-sebastian commented Sep 12, 2019

View reviewed changes

schmidt-sebastian removed their assignment Sep 12, 2019

schmidt-sebastian added 2 commits September 12, 2019 15:21

[AUTOMATED]: Prettier Code Styling

1e8d007

Merge branch 'mrschmidt/indexfree-master' into mrschmidt/engine

5e65206

schmidt-sebastian assigned mikelehen Sep 12, 2019

Fix lint

ac38cbe

mikelehen suggested changes Sep 13, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Sep 13, 2019

More feedback

2f1ce77

schmidt-sebastian force-pushed the mrschmidt/engine branch from ce31829 to bec52ae Compare September 17, 2019 00:24

Re-work executeQuery()

fdff9cd

schmidt-sebastian force-pushed the mrschmidt/engine branch from bec52ae to fdff9cd Compare September 17, 2019 00:30

schmidt-sebastian commented Sep 17, 2019

View reviewed changes

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Sep 17, 2019

schmidt-sebastian force-pushed the mrschmidt/engine branch from b628c88 to 9db213e Compare September 17, 2019 00:49

Reduce diff

1aa22ac

schmidt-sebastian force-pushed the mrschmidt/engine branch from 6fc8eba to 1aa22ac Compare September 17, 2019 00:59

mikelehen approved these changes Sep 17, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Sep 17, 2019

Typo

42fb166

schmidt-sebastian merged commit 539ab0d into mrschmidt/indexfree-master Sep 17, 2019

schmidt-sebastian mentioned this pull request Sep 18, 2019

Backport executeQuery() API changes firebase/firebase-android-sdk#822

Merged

schmidt-sebastian deleted the mrschmidt/engine branch October 10, 2019 22:55

firebase locked and limited conversation to collaborators Oct 18, 2019

Add IndexFreeQueryEngine #2169

Add IndexFreeQueryEngine #2169

Uh oh!

Conversation

schmidt-sebastian commented Sep 11, 2019

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

schmidt-sebastian Sep 12, 2019 •

edited

Loading

schmidt-sebastian Sep 12, 2019 •

edited

Loading

schmidt-sebastian Sep 13, 2019 •

edited

Loading