Add Index-Free query engine #697

schmidt-sebastian · 2019-08-09T21:29:50Z

This is the last PR with index-free logic. It, however, doesn't turn it on yet (and is still against the indexfree-master branch).

There are a fair bit of comments in the code to explain how it all works. The only other thing worth pointing out is that I updated the LocalStoreTests to run parameterized with different query engine, which changed some of the plumbing during the test setup.

I verified that the Spec tests pass with the IndexFreeQueryEngine, but also left those running against the SimpleQueryEngine to match the code in production.

schmidt-sebastian · 2019-08-14T22:10:41Z

PTAL :)

wilhuff · 2019-08-15T16:38:27Z

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

+   * Returns true if this query does not specify any query constraints that could remove results.
+   */
+  public boolean matchesAllDocuments() {
+    return filters.isEmpty() && limit == NO_LIMIT && startAt == null && endAt == null;


Order bys also remove documents because they implicitly restrict the result set to those documents containing the field on which to order.

This is my often suggested workaround for "not null" queries. Good catch.

The fix isn't exactly right because users can explicitly orderBy("__name__") which is the one case where we don't lose any entries. I think it's:

FieldPath firstField = getFirstOrderByField(); firstField == null || (firstField.isKeyField() && getExplicitOrderBy().size() == 1);

I didn't feel like there was much to gain from special-casing this, but since this is now a generalized API in Query itself, it probably makes sense to address this case.

wilhuff · 2019-08-15T16:57:48Z

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

+    // Limit queries are not eligible for index-free query execution if any part of the result was
+    // modified after we received the last query snapshot. This makes sure that we re-populate the
+    // view with older documents that may sort before the modified document.
+    if (query.hasLimit()


This is tragic if there's nothing we can do about it. Users use limits all the time and refine views by applying additional filters. If two different queries run against the same data each will invalidate the other's ability to run index free.

We could probably make this less bad if we change this line

firebase-android-sdk/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

Line 398 in 72e972c

|| doc.getVersion().compareTo(existingDoc.getVersion()) >= 0) {

to not update documents that haven't changed (provided they have no local commits).

That seems pretty reasonable.

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

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

...se-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java

wilhuff · 2019-08-15T17:26:44Z

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

+  public void testUsesTargetMappingToExecuteQueries() {
+    // This test verifies that once a target mapping has been written, only documents that match
+    // the query are read from the RemoteDocumentCache.
+    assumeTrue(queryEngine instanceof IndexFreeQueryEngine);


Nit: none of the other platforms have an equivalent to assumeTrue. Is this really better than just if/return?

It seems like !garbageCollectorIsEager() is a similar kind of assumption. Handle them the same way?

JavaScript allows us to conditionally invoke it.skip() (https://github.com/firebase/firebase-js-sdk/blob/80d0846598d5a155390701c3b9a93ac160ee12b0/packages/firestore/test/integration/api/database.test.ts#L126). I prefer using the test runners this way because it doesn't give us the illusion that we ran a large number of tests when in reality we didn't

I know that we handle garbageCollectorIsEager differently, but I would much prefer to also use assume for this. Either way, I don't feel strongly.

I don't have a strong preference either but it seems like we should at least handle query engine and GC the same way. assumeFalse(garbageCollectorIsEager) or assumeTrue(garbageCollectorNonEager())?

There are only a few tests that skip test execution completely based on garbageCollectorIsEager, so I updated them to use Assume.

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

schmidt-sebastian

I addressed most of the feedback, and left some questions unanswered for follow-up.

schmidt-sebastian · 2019-08-15T18:23:08Z

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

+   * Returns true if this query does not specify any query constraints that could remove results.
+   */
+  public boolean matchesAllDocuments() {
+    return filters.isEmpty() && limit == NO_LIMIT && startAt == null && endAt == null;


This is my often suggested workaround for "not null" queries. Good catch.

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

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

firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java

...se-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java

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

...se-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java

…e/firebase-android-sdk into mrschmidt/indexfree-query-engine

wilhuff · 2019-08-16T15:02:40Z

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

+   * Returns true if this query does not specify any query constraints that could remove results.
+   */
+  public boolean matchesAllDocuments() {
+    return filters.isEmpty() && limit == NO_LIMIT && startAt == null && endAt == null;


The fix isn't exactly right because users can explicitly orderBy("__name__") which is the one case where we don't lose any entries. I think it's:

FieldPath firstField = getFirstOrderByField(); firstField == null || (firstField.isKeyField() && getExplicitOrderBy().size() == 1);

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

wilhuff · 2019-08-16T15:43:41Z

...se-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java

+
+  /** Add a document to local cache but not the remote key mapping. */
+  private void addDocumentToLocalResult(Document doc) {
+    remoteDocumentCache.add(doc, doc.getVersion());


Should this be in a transaction?

Updated for consistency. Note that I wrapped addDocumentToRemoteResult in a transaction since the LRU accounting throws a NPE without it.

wilhuff · 2019-08-16T15:48:11Z

...se-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java

+    addDocumentToRemoteResult(MATCHING_DOC_A);
+    addDocumentToRemoteResult(MATCHING_DOC_B);
+
+    DocumentSet docs = runQuery(query, queryData, /* expectIndexFree= */ true);


Since you're already spelling out /* expectIndexFree= */ at each call site and just assigning to a member here you might consider doing this separately, like so:

IndexFreeQueryEngineTest expectIndexFree() { expectIndexFreeExecution = true; return this; }

Then your callers can be

DocumentSet docs = expectIndexFree().runQuery(query, runData);

This is a totally optional minor thing, but eliminates passing a boolean literal, which always seems suspect in my book.

I updated it to use Lambdas. It now looks like

expectIndexFreeQuery(() -> runQuery(query, queryData));

or

expectFullCollectionQuery(() -> runQuery(query, queryData));

wilhuff · 2019-08-16T16:24:17Z

...se-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java

+    QueryData originalQueryData = queryData(query, /* hasLimboFreeSnapshot= */ true);
+
+    addDocumentToRemoteResult(MATCHING_DOC_A);
+    addDocumentToRemoteResult(NON_MATCHING_DOC_B);


Similar to my earlier question: I don't understand how this happens. Is this shorthand for the query result initially matching B, but then an update changed B to not match?

Also, all versions of document B are at version 2 and changing UPDATED_MATCHING_DOC_B doesn't change that. I guess this happens to work because queryData forces the initial query to version 1, but that means that we never find B except because it's been updated.

Is this the premise of the test? If so it'd be worth spelling this out in comments.

Also: "remote" and "local" don't really capture the difference between these operations that well since both cases are updating the remote document cache. One case is setting which documents match the query and the other is performing an out-of-band update I'd suggest renaming these initiallyMatched (or matchesQuery or addToQueryResult) vs otherQueryUpdates (or updateSeparately, updateByOtherQuery, updateOutOfBand, or something similar).

For this test case, I can just use two matching documents in the beginning and overwrite one to be non-matching. There is a similar test case where that does quite achieve the same test coverage, I added a comment here (doesNotUseInitialResultsForLimitQueryWithDocumentRemoval()).

Furthermore, I split the addDocumentToRemoteResult() into two calls. We now have addDocument() and persistQueryMapping() which should hopefully make it much clearer what is going on.

wilhuff · 2019-08-16T16:26:35Z

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

+  public void testUsesTargetMappingToExecuteQueries() {
+    // This test verifies that once a target mapping has been written, only documents that match
+    // the query are read from the RemoteDocumentCache.
+    assumeTrue(queryEngine instanceof IndexFreeQueryEngine);


I don't have a strong preference either but it seems like we should at least handle query engine and GC the same way. assumeFalse(garbageCollectorIsEager) or assumeTrue(garbageCollectorNonEager())?

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

wilhuff · 2019-08-16T16:59:34Z

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

+    // Limit queries are not eligible for index-free query execution if any part of the result was
+    // modified after we received the last query snapshot. This makes sure that we re-populate the
+    // view with older documents that may sort before the modified document.
+    if (query.hasLimit()


That seems pretty reasonable.

…e/firebase-android-sdk into mrschmidt/indexfree-query-engine

schmidt-sebastian · 2019-08-16T23:10:15Z

/test check-changed

schmidt-sebastian · 2019-08-18T03:37:11Z

/test check-changed

…ery-engine

google-oss-bot · 2019-08-18T04:35:21Z

@schmidt-sebastian: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
device-check-changed	`10f0c8e`	link	`/test device-check-changed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wilhuff

LGTM

schmidt-sebastian · 2019-08-21T16:18:28Z

/retest device-check-changed

schmidt-sebastian · 2019-08-21T17:17:42Z

Test failure is for a RTDB test (persistedServerValuesAreResolved).

Add Index-Free query engine

72e972c

googlebot added the cla: yes Override cla label Aug 9, 2019

google-oss-bot added the size/XL label Aug 9, 2019

schmidt-sebastian requested a review from wilhuff August 9, 2019 21:30

schmidt-sebastian assigned wilhuff Aug 9, 2019

schmidt-sebastian added the api: firestore label Aug 9, 2019

Formatting firebase-database-collection

db9a9a7

schmidt-sebastian force-pushed the mrschmidt/indexfree-query-engine branch from 36891f7 to db9a9a7 Compare August 9, 2019 22:25

Use androidX Nullable

d65656b

schmidt-sebastian force-pushed the mrschmidt/indexfree-query-engine branch from 98a562f to d65656b Compare August 9, 2019 22:29

Re-wrap comment

335b3e4

wilhuff reviewed Aug 15, 2019

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Aug 15, 2019

Addressed feedback

664f6e3

schmidt-sebastian commented Aug 16, 2019

View reviewed changes

Merge branch 'mrschmidt/indexfree-query-engine' of github.com:firebas…

4b6ac3f

…e/firebase-android-sdk into mrschmidt/indexfree-query-engine

schmidt-sebastian removed their assignment Aug 16, 2019

wilhuff reviewed Aug 16, 2019

View reviewed changes

schmidt-sebastian added 4 commits August 16, 2019 15:16

Re-wrap comment

62e4f7c

(Mostly) test cleanup

165b40b

Merge branch 'mrschmidt/indexfree-query-engine' of github.com:firebas…

407b7d8

…e/firebase-android-sdk into mrschmidt/indexfree-query-engine

Java Format

477ba0d

schmidt-sebastian added 2 commits August 17, 2019 21:03

Merge branch 'mrschmidt/indexfree-master' into mrschmidt/indexfree-qu…

805f41e

…ery-engine

Fix unit tests

10f0c8e

schmidt-sebastian assigned wilhuff Aug 20, 2019

wilhuff approved these changes Aug 21, 2019

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Aug 21, 2019

schmidt-sebastian merged commit 715f3f0 into mrschmidt/indexfree-master Aug 21, 2019

schmidt-sebastian deleted the mrschmidt/indexfree-query-engine branch August 27, 2019 23:45

firebase locked and limited conversation to collaborators Oct 8, 2019

Add Index-Free query engine #697

Add Index-Free query engine #697

Uh oh!

Conversation

schmidt-sebastian commented Aug 9, 2019

Uh oh!

schmidt-sebastian commented Aug 14, 2019

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

wilhuff Aug 16, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Aug 16, 2019

Uh oh!

schmidt-sebastian commented Aug 18, 2019

Uh oh!

google-oss-bot commented Aug 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wilhuff Aug 16, 2019 •

edited

Loading

google-oss-bot commented Aug 18, 2019 •

edited

Loading