Only fetch 50 documents per backfill #3193

schmidt-sebastian · 2021-11-30T23:06:40Z

This PR adds a dedicated API to the RemoteDocumentCache to fetch the next 50 documents per collection group. This is done with a giant UNION statement.

Changes to be committed:

google-oss-bot · 2021-11-30T23:17:39Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 45.20% (110b867) to 45.24% (4b5d63f4) by +0.04%.

Filename	Base (`110b867`)	Head (4b5d63f4)	Diff
DeleteMutation.java	95.00%	90.00%	-5.00%
FieldIndex.java	100.00%	98.08%	-1.92%
IndexBackfiller.java	78.05%	76.00%	-2.05%
MemoryRemoteDocumentCache.java	100.00%	98.25%	-1.75%
PatchMutation.java	100.00%	98.39%	-1.61%
SQLiteIndexManager.java	99.37%	99.37%	-0.00%
SQLiteRemoteDocumentCache.java	95.88%	96.72%	+0.85%
SQLiteSchema.java	96.62%	96.67%	+0.05%
SetMutation.java	97.14%	94.29%	-2.86%
Util.java	66.14%	63.70%	-2.44%

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 (4b5d63f4) is created by Prow via merging commits: 110b867 a3a03b1.

google-oss-bot · 2021-11-30T23:19:09Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (110b867) Head (4b5d63f4) Diff

aar 1.23 MB 1.23 MB +1.72 kB (+0.1%)

apk (release) 3.32 MB 3.33 MB +556 B (+0.0%)

Test Logs

Notes

Head commit (4b5d63f4) is created by Prow via merging commits: 110b867 a3a03b1.

schmidt-sebastian · 2021-11-30T23:41:24Z

cc @thebrianchen

schmidt-sebastian · 2021-12-02T16:48:59Z

/retest

schmidt-sebastian · 2021-12-07T16:47:36Z

FYI

EXPLAIN QUERY PLAN SELECT contents, read_time_seconds, read_time_nanos, path FROM remote_documents WHERE path >= 'a' AND path < 'b' AND path_length = 2 AND (read_time_seconds > 1 OR (read_time_seconds = 2 AND read_time_nanos > 3) OR ( read_time_seconds = 4 AND read_time_nanos = 5 and path > 'b')) UNION SELECT contents, read_time_seconds, read_time_nanos, path FROM remote_documents WHERE path >= 'a1' AND path < 'b1' AND path_length = 2 AND (read_time_seconds > 1 OR (read_time_seconds = 2 AND read_time_nanos > 3) OR ( read_time_seconds = 4 AND read_time_nanos = 5 and path > 'b1')) ORDER BY read_time_seconds, read_time_nanos, path LIMIT 2;
QUERY PLAN
`--MERGE (UNION)
   |--LEFT
   |  |--SEARCH remote_documents USING INDEX sqlite_autoindex_remote_documents_1 (path>? AND path<?)
   |  `--USE TEMP B-TREE FOR ORDER BY
   `--RIGHT
      |--SEARCH remote_documents USING INDEX sqlite_autoindex_remote_documents_1 (path>? AND path<?)
      `--USE TEMP B-TREE FOR ORDER BY

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

wu-hui · 2021-12-06T16:16:51Z

...e-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java

+      for (int i = 0; i < collections.size(); i += pageSize) {
+        results.putAll(
+            getAll(
+                collections.subList(i, Math.min(collections.size(), i + pageSize)), offset, count));


Each fan out call to getAll is called with count, then added directly to the result. So the result could be #collections * count?

Only if collections > 100. The maximum number of documents returned is (Math.max(collections / 100) * 50). Do you think we should post-process this list?

Actually, let me just add this code. The list should only have 50 elements so there is very little overhead.

I see. I still think we should explain what pageSize is here, and I also feel there is a better name..

...e-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java

wu-hui

LGTM.

wu-hui · 2021-12-08T19:25:52Z

...e-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java

+  @Override
+  public Map<DocumentKey, MutableDocument> getAll(
+      String collectionGroup, IndexOffset offset, int limit) {
+    // Note: This method is pretty inefficient, but t is not called since the method is only used


Also, if this is the case then maybe just throw an exception here?

Changed to throw exception.

...e-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java

wu-hui · 2021-12-08T19:37:02Z

...e-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java

+      for (int i = 0; i < collections.size(); i += pageSize) {
+        results.putAll(
+            getAll(
+                collections.subList(i, Math.min(collections.size(), i + pageSize)), offset, count));


I see. I still think we should explain what pageSize is here, and I also feel there is a better name..

wu-hui · 2021-12-08T19:42:05Z

firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java

@@ -366,4 +368,19 @@ public static StringBuilder repeatSequence(
  private static <T> T advanceIterator(Iterator<T> it) {
    return it.hasNext() ? it.next() : null;
  }
+
+  /** Returns a map with the first {#code count} elements of {#code data} when sorted by comp. */
+  public static <K, V> Map<K, V> trimMap(Map<K, V> data, int count, Comparator<V> comp) {


What about firstNEntries?

schmidt-sebastian added 11 commits November 24, 2021 15:54

Migrate remote_documents table to use dedicated collection_path column

cf225f9

Add timer

7f16305

Changes to be committed:

Add DocumentId helpers

ba26b87

Add one more test

60a4240

Rename

9a12530

Merge branch 'master' into mrschmidt/newrdc

dade039

Format

07500fc

Don't delete column

7671330

Changelog

b25191f

Merge

0f93b23

Use PathLength

723defc

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

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

schmidt-sebastian requested a review from wu-hui November 30, 2021 23:41

schmidt-sebastian assigned wu-hui Nov 30, 2021

schmidt-sebastian force-pushed the mrschmidt/getnext branch 2 times, most recently from 778a22d to 2fed47b Compare December 1, 2021 17:47

google-oss-bot added size/XXL and removed size/L labels Dec 1, 2021

schmidt-sebastian changed the base branch from master to mrschmidt/pathlength December 1, 2021 17:48

google-oss-bot added size/XL and removed size/XXL labels Dec 1, 2021

schmidt-sebastian force-pushed the mrschmidt/getnext branch from 2fed47b to 2aa194a Compare December 1, 2021 17:49

google-oss-bot added size/L and removed size/XL labels Dec 1, 2021

schmidt-sebastian added 2 commits December 1, 2021 10:50

Merge branch 'master' into mrschmidt/pathlength

fb15500

Fetch 50 documents

fc00ff6

schmidt-sebastian force-pushed the mrschmidt/getnext branch from 2aa194a to fc00ff6 Compare December 1, 2021 17:56

schmidt-sebastian added 4 commits December 2, 2021 10:25

Simplify

759a50b

Format

acc43e9

Fill read_time

c4963a3

Review

0f56227

Base automatically changed from mrschmidt/pathlength to master December 3, 2021 16:46

google-oss-bot added size/XL and removed size/L labels Dec 3, 2021

Merge

cf107bd

Merge

fa3483b

google-oss-bot added size/L and removed size/XL labels Dec 7, 2021

Format

473e8f4

wu-hui reviewed Dec 8, 2021

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Dec 8, 2021

Review

4719d11

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Dec 8, 2021

wu-hui approved these changes Dec 8, 2021

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Dec 8, 2021

Review

a3a03b1

schmidt-sebastian merged commit 1986969 into master Dec 8, 2021

schmidt-sebastian deleted the mrschmidt/getnext branch December 8, 2021 20:19

firebase locked and limited conversation to collaborators Jan 8, 2022

Only fetch 50 documents per backfill #3193

Only fetch 50 documents per backfill #3193

Uh oh!

Conversation

schmidt-sebastian commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Nov 30, 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 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

schmidt-sebastian commented Nov 30, 2021

Uh oh!

schmidt-sebastian commented Dec 2, 2021

Uh oh!

schmidt-sebastian commented Dec 7, 2021

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!

Uh oh!

wu-hui 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!

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 commented Nov 30, 2021 •

edited

Loading

google-oss-bot commented Nov 30, 2021 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Nov 30, 2021 •

edited

Loading

`firebase-firestore`