Add mutation batch id to index backfill #3202

thebrianchen · 2021-12-02T22:37:10Z

This PR adds mutation batch id support to the index backfiller. All documents in the same mutation batch are processed, even if it goes over the limit (the alternative is to add an offset for mutations, but batches are limited to 500 writes).

Still todo:

Current implementation performs a collection group query with documents + overlay, but local mutations are not written to the collection_groups table that support collection group queries. This means that the current implementation only supports local mutations that already have a remote document in the same collection.

thebrianchen · 2021-12-02T22:42:02Z

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

+  }
+
+  /** Updates the provided remote documents to include documents in the overlay. */
+  private ImmutableSortedMap<DocumentKey, MutableDocument>


I tried refactoring out shared code between getDocumentsMatchingCollectionQueryFromOverlayCache and getDocumentsCollectionQueryWithBatchId, but I think it can probably be cleaner.

We are probably at a point where we should just remove the non-overlay code.

@wu-hui

google-oss-bot · 2021-12-02T22:55:29Z

Coverage Report ¹

Affected Products

No changes between base commit (46b3dfd) and merge commit (b025f3a).

Test Logs

Notes

Commit (b025f3a) is created by Prow via merging PR base commit (46b3dfd) and head commit (4b10ea0).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/vUOXETjUI6.html

google-oss-bot · 2021-12-02T22:56:53Z

Size Report ¹

Affected Products

firebase-firestore
Type Base (46b3dfd) Merge (b025f3a) Diff
aar 1.23 MB 1.23 MB +3.69 kB (+0.3%)
apk (release) 3.33 MB 3.33 MB +1.87 kB (+0.1%)

Test Logs

Notes

Commit (b025f3a) is created by Prow via merging PR base commit (46b3dfd) and head commit (4b10ea0).

https://storage.googleapis.com/firebase-sdk-metric-reports/Hwitdog1OW.html

schmidt-sebastian

Some early feedback (did not look at test). Looks pretty good, but we may want to think about how we can remove the need to read documents multiple times.

See also for some changes that may affect this: #3193

firebase-firestore/src/main/java/com/google/firebase/firestore/index/IndexEntry.java

schmidt-sebastian · 2021-12-02T23:13:55Z

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

-   */
-  public static <T extends Comparable<T>> void diffCollections(
-      SortedSet<T> before, SortedSet<T> after, Consumer<T> onAdd, Consumer<T> onRemove) {
-    diffCollections(before.iterator(), after.iterator(), before.comparator(), onAdd, onRemove);


This overload is actually different from the one we now default to. This method takes sorted data - the method you are calling now reorders the data (and copies it) even when already sorted. I think you solve the problem you encountered - that SortedSet<T>.comparator() can return null - by creating a default comparator based on T.compareTo() if the set's comparator is null.

Good catch, thanks. Added the code back and added a null check.

schmidt-sebastian · 2021-12-02T23:19:04Z

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

+   *     change past `sinceBatchId` are returned.
+   */
+  Map<DocumentKey, Pair<Integer, Mutation>> getOverlaysWithBatchId(
+      ResourcePath collection, int sinceBatchId);


I am surprised we don't have a high-level class that contains both the mutation and the batch ID for Overlays. We should probably create one and combine this method with the existing method just above. (cc @wu-hui)

Renamed this method into the original getOverlays() and changed logic to support Pair.

schmidt-sebastian · 2021-12-02T23:20:12Z

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

+
+    // Start indexing mutations only after catching up for read time. This must be done separately
+    // since the backfill's first pass is in read time order rather than mutation batch id order.
+    int documentsRemaining = documentsRemainingUnderCap - oldestDocuments.size();


Can you move this to a helper method for better encapsulation? We might also want to do the same for the RemoteDocumentCache code.

Moved into LocalDocumentsView

schmidt-sebastian · 2021-12-02T23:20:32Z

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

+        // order by batch id.
+        int newLargestBatchId =
+            documentToBatchId.get(documentsByBatchId.get(documentsByBatchId.size() - 1));
+        newOffset =


Can you move this into getNewOffset? (as a final call at the end of the method)

N/A after moving into LocalDocumentsView.

schmidt-sebastian · 2021-12-02T23:33:23Z

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

+   * corresponding batch id.
+   */
+  // TODO(indexing): Support adding local mutations to collection group table.
+  public Map<Document, Integer> getDocumentsBatchIdsMatchingCollectionGroupQuery(


Your code would be cleaner if this method returned Map<BatchId, List<Document>>.

The name is also quite a mouthful - and the method should probably just accept a String collectionGroup rather than a query as it ignores all other query constraints.

Done, but cannot think of a cleaner name.

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

schmidt-sebastian · 2021-12-02T23:42:00Z

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

+  }
+
+  /** Updates the provided remote documents to include documents in the overlay. */
+  private ImmutableSortedMap<DocumentKey, MutableDocument>


We are probably at a point where we should just remove the non-overlay code.

@wu-hui

schmidt-sebastian · 2021-12-02T23:42:40Z

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

@@ -105,4 +105,32 @@ public void removeOverlaysForBatchId(int batchId) {

    return result;
  }
+
+  @Override
+  public Map<DocumentKey, Pair<Integer, Mutation>> getOverlaysWithBatchId(


Please find a way to remove this duplication - even if we do not add an Overlay class.

Combined the two methods into a single one.

schmidt-sebastian · 2021-12-02T23:43:04Z

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

@@ -111,4 +112,34 @@ public void removeOverlaysForBatchId(int batchId) {

    return result;
  }
+
+  @Override
+  public Map<DocumentKey, Pair<Integer, Mutation>> getOverlaysWithBatchId(


Same comment as above.

…/index/IndexEntry.java Co-authored-by: Sebastian Schmidt <[email protected]>

…-sdk into bc/mutation-id

schmidt-sebastian · 2021-12-03T21:03:02Z

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

+    documentsProcessed = backfiller.backfill();
+    assertEquals(1, documentsProcessed);
+    SortedSet<IndexEntry> after = indexManager.getExistingIndexEntries(docA.getKey(), fieldIndex);
+    assertNotEquals(before.first().getDirectionalValue(), after.first().getDirectionalValue());


Instead of relying on getExistingIndexEntries, can you just the public API and invoke getDocumentsMatchingTarget with two different targets (foo > 10 and foo > 20 for example). Before the update, you can verify that one of them matches and then after the update two match.

Same comment applies below.

schmidt-sebastian

Tests look great - but we probably do not need to use the private API.

…r merge

…nto Overlay

thebrianchen · 2021-12-09T22:39:53Z

Main changes:

Documents are no longer memoized.
LDV still calculates the new offset and returns the result as pair since it's cleaner than passing in the set of document keys in 2 separate calls.
Overlay class added.
Mutations now have the largest batch id

/test check-changed
/test device-check-changed

schmidt-sebastian

This is a huge step in the right direction. We should talk about the refactor to add (and use) the BatchId in the Document class. I see that you added the code to populate it - maybe it will now be easy enough to also use it? It would allow you to stop passing Pair<integer, document> types.

schmidt-sebastian · 2021-12-10T19:55:37Z

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

@@ -47,11 +48,14 @@
  void removeOverlaysForBatchId(int batchId);

  /**
-   * Returns all saved overlays for the given collection.
+   * Returns a mapping of overlays containing the largest batch id for each overlay mutation in the


You can probably revert this documentation change now.

schmidt-sebastian · 2021-12-10T19:55:52Z

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

   *
   * @param collection The collection path to get the overlays for.
   * @param sinceBatchId The minimum batch ID to filter by (exclusive). Only overlays that contain a
   *     change past `sinceBatchId` are returned.
+   * @return Mapping of each document key in the collection to its largest batch id and overlay
+   *     mutation.


Please update.

schmidt-sebastian · 2021-12-10T19:56:56Z

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

      processedCollectionGroups.add(collectionGroup);
    }
    return maxDocumentsToProcess - documentsRemaining;
  }

-  /** Writes entries for the fetched field indexes. */
+  /**
+   * Writes entries for the corresponding field indexes in the provided collection group. Returns


Please update as their are no "corresponding" indexes (at least none are passed to the method)

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

schmidt-sebastian · 2021-12-10T20:00:50Z

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

+    lowestOffset = lowestOffset == null ? IndexOffset.NONE : lowestOffset;
+
+    // Add earliest batch id to offset
+    int earliestBatchId = getEarliestBatchId(fieldIndexes);


Can you inline this call? You are iterating over the list of field indexes twice.

schmidt-sebastian · 2021-12-10T20:19:38Z

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

+
+    // Apply overlays to empty mutable document map to generate base.
+    for (Map.Entry<DocumentKey, Overlay> entry : overlays.entrySet()) {
+      remoteDocuments.put(entry.getKey(), MutableDocument.newInvalidDocument(entry.getKey()));


Do we not need to fetch the documents from the RDC?

Since the document overlay cache contains the most up to date version of the documents, we don't need to.

schmidt-sebastian · 2021-12-10T20:21:13Z

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

+        mutation.applyToLocalView(docEntry.getValue(), null, batchId, Timestamp.now());
+      }
+      // Finally, insert the documents that still match the query.
+      if (query.matches(docEntry.getValue())) {


I think we should do this once at the end of getNextDocumentsAndOffset.

FWIW, we might not need to do this at all if we pass a collection group here instead of the query. None of the IndexBackfill methods need a query.

schmidt-sebastian · 2021-12-10T20:22:32Z

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

-      return getDocumentKey().compareTo(other.getDocumentKey());
+      cmp = getDocumentKey().compareTo(other.getDocumentKey());
+      if (cmp != 0) return cmp;
+      return Integer.signum(getLargestBatchId() - other.getLargestBatchId());


Integer.compare? (for noobs like me)

schmidt-sebastian · 2021-12-10T20:23:04Z

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

@@ -182,6 +184,10 @@ public SnapshotVersion getReadTime() {
    return readTime;
  }

+  public int getMutationBatchID() {


This doesn't seem used but I wish it was :)

schmidt-sebastian · 2021-12-10T20:24:43Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Overlay.java

+ * <p>Holds information about a mutation and the largest batch id in Firestore when the mutation was
+ * created.
+ */
+public class Overlay {


You probably want to do one of the following:

Make this an AutoValue

Make this a Kotlin value class

Add equals/hashCode

Turned it into an AutoValue.

schmidt-sebastian · 2021-12-10T20:27:36Z

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

+  /** Returns the offset for the index based on the newly indexed documents. */
+  private IndexOffset getNewOffset(
+      ImmutableSortedMap<DocumentKey, Document> documents, IndexOffset currentOffset) {
+    if (documents.isEmpty()) {


FWIW This should eventually live in the IndexBackfiller.

Discussed offline, this probably does not and we need to leave it here.

google-oss-bot · 2022-01-14T01:35:55Z

@thebrianchen: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
check-changed	`4b10ea0`	link	`/test 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.

thebrianchen

Some comments are now outdated + I don't really remember their original context. Please review this as a rough draft. Will clean up the comments later.

thebrianchen · 2021-12-10T20:44:47Z

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

   *
   * @param collection The collection path to get the overlays for.
   * @param sinceBatchId The minimum batch ID to filter by (exclusive). Only overlays that contain a
   *     change past `sinceBatchId` are returned.
+   * @return Mapping of each document key in the collection to its largest batch id and overlay
+   *     mutation.


thebrianchen · 2021-12-10T20:44:52Z

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

@@ -47,11 +48,14 @@
  void removeOverlaysForBatchId(int batchId);

  /**
-   * Returns all saved overlays for the given collection.
+   * Returns a mapping of overlays containing the largest batch id for each overlay mutation in the


thebrianchen · 2021-12-10T20:45:39Z

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

      processedCollectionGroups.add(collectionGroup);
    }
    return maxDocumentsToProcess - documentsRemaining;
  }

-  /** Writes entries for the fetched field indexes. */
+  /**
+   * Writes entries for the corresponding field indexes in the provided collection group. Returns


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

thebrianchen · 2021-12-10T20:47:32Z

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

+    lowestOffset = lowestOffset == null ? IndexOffset.NONE : lowestOffset;
+
+    // Add earliest batch id to offset
+    int earliestBatchId = getEarliestBatchId(fieldIndexes);


thebrianchen · 2021-12-10T21:06:14Z

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

+   */
+  public Map<Document, Integer> getDocumentsInOverlayCacheByBatchId(
+      Query query, IndexOffset offset) {
+    Map<DocumentKey, MutableDocument> remoteDocuments = new HashMap<>();


thebrianchen · 2021-12-10T21:07:52Z

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

+
+    // Apply overlays to empty mutable document map to generate base.
+    for (Map.Entry<DocumentKey, Overlay> entry : overlays.entrySet()) {
+      remoteDocuments.put(entry.getKey(), MutableDocument.newInvalidDocument(entry.getKey()));


Since the document overlay cache contains the most up to date version of the documents, we don't need to.

thebrianchen · 2021-12-10T21:10:26Z

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

-        overlay.applyToLocalView(docEntry.getValue(), null, Timestamp.now());
+      Overlay overlay = overlays.get(docEntry.getKey());
+      Mutation mutation = overlay != null ? overlay.getMutation() : null;
+      if (mutation != null) {


thebrianchen · 2021-12-10T21:15:19Z

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

+        if (!results.containsKey(batchId)) {
+          List<Document> documents = new ArrayList<>();
+          documents.add(document);
+          results = results.insert(batchId, documents);
+        } else {
+          results.get(batchId).add(document);
+        }


thebrianchen · 2021-12-10T21:27:07Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Overlay.java

+ * <p>Holds information about a mutation and the largest batch id in Firestore when the mutation was
+ * created.
+ */
+public class Overlay {


Turned it into an AutoValue.

thebrianchen · 2022-01-24T16:20:11Z

Better version being implemented in #3323

add mutation batch id to index backfill

6d64d18

thebrianchen added the api: firestore label Dec 2, 2021

thebrianchen self-assigned this Dec 2, 2021

google-cla bot added the cla: yes Override cla label Dec 2, 2021

google-oss-bot added the size/L label Dec 2, 2021

thebrianchen commented Dec 2, 2021

View reviewed changes

thebrianchen requested a review from schmidt-sebastian December 2, 2021 22:47

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Dec 2, 2021

Brian Chen added 2 commits December 2, 2021 16:58

fix compareTo in FieldIndex

c27c051

removed submodule

063c88b

schmidt-sebastian reviewed Dec 2, 2021

View reviewed changes

Brian Chen and others added 2 commits December 3, 2021 08:21

Update firebase-firestore/src/main/java/com/google/firebase/firestore…

b7fa22d

…/index/IndexEntry.java Co-authored-by: Sebastian Schmidt <[email protected]>

Merge branch 'bc/mutation-id' of github.com:firebase/firebase-android…

7202745

…-sdk into bc/mutation-id

schmidt-sebastian reviewed Dec 3, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Dec 3, 2021

Refactor document ordering into LocalDocumentsView

c0457b6

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

remove semantic_comparator unused

add5707

thebrianchen force-pushed the bc/mutation-id branch from 324714c to add5707 Compare December 7, 2021 23:44

Merge branch 'master' into bc/mutation-id

7e7164d

thebrianchen requested a review from schmidt-sebastian December 7, 2021 23:49

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Dec 7, 2021

filter out local mutations from getDocumentsMatchingQuery after maste…

aff17e3

…r merge

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Dec 8, 2021

Brian Chen added 4 commits December 8, 2021 15:51

fix crashlytics submodule

0b9b735

merge master resolve conflicts

438592c

refactor LDV to handle returning next documents to index

a82bfbf

refactor largest batch id into Mutation and largestBatchId/Mutation i…

4c830a4

…nto Overlay

thebrianchen requested a review from schmidt-sebastian December 9, 2021 22:39

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Dec 9, 2021

schmidt-sebastian reviewed Dec 10, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Dec 10, 2021

Brian Chen added 7 commits December 10, 2021 15:32

resolving simple comments, make overlay autovalue

c47695f

revert adding batch id to document

9ea9b27

revert batch id to applyToLocalView

2ec7f09

Merge branch 'master' into bc/mutation-id

1af2624

Merge branch 'master' into bc/mutation-id

273656b

update tests after merging master

1345cf6

fetch overlays by collection group

4b10ea0

thebrianchen commented Jan 14, 2022

View reviewed changes

thebrianchen requested a review from schmidt-sebastian January 14, 2022 02:30

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jan 14, 2022

schmidt-sebastian mentioned this pull request Jan 20, 2022

Backfill Mutations #3323

Merged

thebrianchen closed this Jan 24, 2022

firebase locked and limited conversation to collaborators Feb 24, 2022

Add mutation batch id to index backfill #3202

Add mutation batch id to index backfill #3202

Uh oh!

Conversation

thebrianchen commented Dec 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Coverage Report 1

Affected Products

Test Logs

Notes

Uh oh!

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

Uh oh!

Size Report 1

Affected Products

firebase-firestore

Test Logs

Notes

Uh oh!

schmidt-sebastian 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!

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

thebrianchen commented Dec 9, 2021

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!

google-oss-bot commented Dec 2, 2021 •

edited

Loading

Coverage Report ¹

google-oss-bot commented Dec 2, 2021 •

edited

Loading

Size Report ¹

`firebase-firestore`