Use DocumentKey as tie breaker in index backfill #3174

schmidt-sebastian · 2021-11-24T21:57:21Z

This PR changes the index backfill offset to use readTime+documentKey instead of just readTime. This helps there are more than 50 documents per read time.

google-oss-bot · 2021-11-29T20:17:50Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 45.20% (ab6bab4) to 45.28% (e42ab4ff) by +0.08%.

Filename	Base (`ab6bab4`)	Head (e42ab4ff)	Diff
AutoValue_FieldIndex_IndexOffset.java	?	62.50%	?
DefaultQueryEngine.java	97.83%	97.87%	+0.05%
IndexBackfiller.java	77.38%	78.05%	+0.67%
PatchMutation.java	98.39%	100.00%	+1.61%
SQLiteIndexManager.java	99.36%	99.37%	+0.01%
SQLiteRemoteDocumentCache.java	95.83%	96.00%	+0.17%
SetMutation.java	94.29%	97.14%	+2.86%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (e42ab4ff) is created by Prow via merging commits: ab6bab4 72489fc.

google-oss-bot · 2021-11-29T20:26:52Z

Binary Size Report

Affected SDKs

`firebase-crashlytics-ndk`

Type	Base (`ab6bab4`)	Head (e42ab4ff)	Diff
aar	1.68 MB	1.68 MB	-443 B (-0.0%)
apk (aggressive / x86_64)	1.44 MB	1.44 MB	-4.10 kB (-0.3%)
apk (release / x86_64)	2.07 MB	2.07 MB	-4.10 kB (-0.2%)

firebase-firestore

Type Base (ab6bab4) Head (e42ab4ff) Diff

aar 1.23 MB 1.23 MB +2.89 kB (+0.2%)

apk (release) 3.32 MB 3.33 MB +1.17 kB (+0.0%)

Test Logs

Notes

Head commit (e42ab4ff) is created by Prow via merging commits: ab6bab4 72489fc.

schmidt-sebastian · 2021-11-29T20:27:15Z

/test check-changed

thebrianchen

lgtm, with a question about range exclusivity.

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

thebrianchen · 2021-11-29T20:43:48Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

+     */
+    public static IndexOffset create(SnapshotVersion readTime) {
+      long successorSeconds = readTime.getTimestamp().getSeconds();
+      int successorNanos = readTime.getTimestamp().getNanoseconds() + 1;


If we're doing a > comparison in the RemoteDocumentCache when fetching readTime, we don't want to increment here right? It makes sense to increment if we're doing a >= comparison.

Storing the last read time and using > could also remove the nano increment logic below.

Alternatively, you can keep the range-exclusive behavior and remove the increment.

Added a comment:

// We want to create an offset that matches all documents with a read time greater than // the provided read time. To do so, we technically need to create an offset for // `(readTime, MAX_DOCUMENT_KEY)`. While we could use Unicode codepoints to generate // MAX_DOCUMENT_KEY, it is much easier to use `(readTime + 1, DocumentKey.empty())` since // `> DocumentKey.empty()` matches all valid document IDs.

thebrianchen · 2021-11-29T20:53:06Z

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

        documents.isEmpty()
-            ? remoteDocumentCache.getLatestReadTime()
-            : documents.get(documents.size() - 1).getReadTime();
+            ? IndexOffset.NONE


Can we use the latest read time here instead of IndexOffset.NONE? This way we can continue from the current read time on the next backfill iteration.

optional: add test that verifies we store/use the latest read time even if no documents were found.

Hm, now I wonder why I did not do this right away. We should use the latest read time here for sure.

thebrianchen · 2021-11-29T21:16:26Z

...firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java

    List<MutableDocument> expected = asList(doc("b/new", 3, docData));
    assertEquals(expected, values(results));
  }

+  @Test
+  public void testDocumentsMatchingQuerySinceReadTimeAndDocumentKey() {


optional: add test for nanoseconds range-exclusivity

Would this be different from testDocumentsMatchingQuerySinceReadTime()? If so, can you provide a rough sketch?

I was thinking you could modify or add the test to check for the nanosecond tiebreaker:

@Test public void testDocumentsMatchingQuerySinceReadTimeAndDocumentKey() { Map<String, Object> docData = map("data", 2); addTestDocumentAtPath("b/a", /* updateTime= */ 1, /* readTime= */ version(11,1)); addTestDocumentAtPath("b/b", /* updateTime= */ 2, /* readTime= = */ version(11,2)); addTestDocumentAtPath("b/c", /* updateTime= */ 3, /* readTime= = */ version(11,3)); addTestDocumentAtPath("b/d", /* updateTime= */ 4, /* readTime= = */ version(12,2)); Query query = Query.atPath(path("b")); ImmutableSortedMap<DocumentKey, MutableDocument> results = remoteDocumentCache.getAllDocumentsMatchingQuery( query, IndexOffset.create(version(11,2))); List<MutableDocument> expected = asList(doc("b/c", 3, docData), doc("b/d", 4, docData)); assertEquals(expected, values(results));

Got it, that makes a lot of sense. Added one more test.

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

schmidt-sebastian

Thanks for the review! Couple follow up questions.

schmidt-sebastian · 2021-11-29T21:40:41Z

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

        documents.isEmpty()
-            ? remoteDocumentCache.getLatestReadTime()
-            : documents.get(documents.size() - 1).getReadTime();
+            ? IndexOffset.NONE


Hm, now I wonder why I did not do this right away. We should use the latest read time here for sure.

schmidt-sebastian · 2021-11-29T22:03:00Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

+     */
+    public static IndexOffset create(SnapshotVersion readTime) {
+      long successorSeconds = readTime.getTimestamp().getSeconds();
+      int successorNanos = readTime.getTimestamp().getNanoseconds() + 1;


Added a comment:

// We want to create an offset that matches all documents with a read time greater than // the provided read time. To do so, we technically need to create an offset for // `(readTime, MAX_DOCUMENT_KEY)`. While we could use Unicode codepoints to generate // MAX_DOCUMENT_KEY, it is much easier to use `(readTime + 1, DocumentKey.empty())` since // `> DocumentKey.empty()` matches all valid document IDs.

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

schmidt-sebastian · 2021-11-29T22:18:34Z

...firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java

    List<MutableDocument> expected = asList(doc("b/new", 3, docData));
    assertEquals(expected, values(results));
  }

+  @Test
+  public void testDocumentsMatchingQuerySinceReadTimeAndDocumentKey() {


Would this be different from testDocumentsMatchingQuerySinceReadTime()? If so, can you provide a rough sketch?

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

thebrianchen · 2021-11-29T22:54:47Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/FieldIndexTest.java

+  @Test
+  public void indexOffsetAdvancesSeconds() {
+    IndexOffset actualSuccessor = IndexOffset.create(version(1, (int) 1e9 - 1));
+    IndexOffset expectedSuccessor = IndexOffset.create(version(2, 0), DocumentKey.empty());


ultranit: no need to specify DocumentKey.empty()

We do need to specify DocumentKey.empty() or else we create a key with version(3,0), DocumentKey.empty().

Use DocumentKey as tie breaker in index backfill

c7a3d42

google-cla bot added the cla: yes Override cla label Nov 24, 2021

google-oss-bot added the size/L label Nov 24, 2021

Nits

a5fcb80

schmidt-sebastian requested a review from thebrianchen November 24, 2021 22:01

schmidt-sebastian assigned thebrianchen Nov 24, 2021

schmidt-sebastian added the api: firestore label Nov 24, 2021

Merge branch 'master' into mrschmidt/offset

fdda906

fix test

bde9f06

thebrianchen approved these changes Nov 29, 2021

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Nov 29, 2021

schmidt-sebastian commented Nov 29, 2021

View reviewed changes

Review

505f72e

schmidt-sebastian removed their assignment Nov 29, 2021

Merge

9738794

thebrianchen reviewed Nov 29, 2021

View reviewed changes

thebrianchen assigned schmidt-sebastian Nov 29, 2021

Add test

72489fc

schmidt-sebastian merged commit d0048fd into master Nov 30, 2021

schmidt-sebastian deleted the mrschmidt/offset branch November 30, 2021 00:12

manny-jimenez pushed a commit that referenced this pull request Nov 30, 2021

Use DocumentKey as tie breaker in index backfill (#3174)

9c02ffe

firebase locked and limited conversation to collaborators Dec 30, 2021

Use DocumentKey as tie breaker in index backfill #3174

Use DocumentKey as tie breaker in index backfill #3174

Uh oh!

Conversation

schmidt-sebastian commented Nov 24, 2021

Uh oh!

google-oss-bot commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

google-oss-bot commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-crashlytics-ndk

firebase-firestore

Test Logs

Notes

Uh oh!

schmidt-sebastian commented Nov 29, 2021

Uh oh!

thebrianchen left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebrianchen Nov 29, 2021 • 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

google-oss-bot commented Nov 29, 2021 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Nov 29, 2021 •

edited

Loading

`firebase-crashlytics-ndk`

`firebase-firestore`

thebrianchen Nov 29, 2021 •

edited

Loading