Review feedback

Author: wilhuff

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/WriteBatchTest.java

@@ -259,6 +259,12 @@ public void testCanWriteTheSameDocumentMultipleTimes() {
 
   @Test
   public void testCanWriteVeryLargeBatches() {
+    // On Android, SQLite Cursors are limited reading no more than 2 MB per row (despite being able
+    // to write very large values). This test verifies that the SQLiteMutationQueue properly works
+    // around this limitation.
+
+    // Create a map containing nearly 1 MB of data. Note that if you use 1024 below this will create
+    // a document larger than 1 MB, which will be rejected by the backend as too large.
     String a = Character.toString('a');
     StringBuilder buf = new StringBuilder(1000);
     for (int i = 0; i < 1000; i++) {
@@ -273,6 +279,9 @@ public void testCanWriteVeryLargeBatches() {
     DocumentReference doc = testDocument();
     WriteBatch batch = doc.getFirestore().batch();
 
+    // Write a batch containing 3 copies of the data, creating a ~3 MB batch. Writing to the same
+    // document in a batch is allowed and so long as the net size of the document is under 1 MB the
+    // batch is allowed.
     batch.set(doc, values);
     for (int i = 0; i < 2; i++) {
       batch.update(doc, values);

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

@@ -44,6 +44,16 @@
 /** A mutation queue for a specific user, backed by SQLite. */
 final class SQLiteMutationQueue implements MutationQueue {
 
+  /**
+   * On Android, SQLite Cursors are limited reading no more than 2 MB per row (despite being able to
+   * write very large values). All reads of the mutations column in the mutations table need to read
+   * in chunks with SUBSTR to avoid going over this limit.
+   *
+   * <p>The value here has to be 2 MB or smaller, while allowing for all possible other values that
+   * might be selected out along with the mutations column in any given result set. Nearly 1 MB is
+   * conservative, but allows all combinations of document paths and batch ids without needing to
+   * figure out if the row has gotten too large.
+   */
   private static final int BLOB_MAX_INLINE_LENGTH = 1000000;
 
   private final SQLitePersistence db;
@@ -216,9 +226,9 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) {
     int nextBatchId = batchId + 1;
 
     return db.query(
-            "SELECT m.batch_id, SUBSTR(m.mutations, 1, ?) FROM mutations m "
-                + "WHERE m.uid = ? AND m.batch_id >= ? "
-                + "ORDER BY m.batch_id ASC LIMIT 1")
+            "SELECT batch_id, SUBSTR(mutations, 1, ?) FROM mutations "
+                + "WHERE uid = ? AND batch_id >= ? "
+                + "ORDER BY batch_id ASC LIMIT 1")
         .binding(BLOB_MAX_INLINE_LENGTH, uid, nextBatchId)
         .firstValue(row -> decodeInlineMutationBatch(row.getInt(0), row.getBlob(1)));
   }
@@ -227,8 +237,8 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) {
   public List<MutationBatch> getAllMutationBatches() {
     List<MutationBatch> result = new ArrayList<>();
     db.query(
-            "SELECT m.batch_id, SUBSTR(m.mutations, 1, ?) "
-                + "FROM mutations m "
+            "SELECT batch_id, SUBSTR(mutations, 1, ?) "
+                + "FROM mutations "
                 + "WHERE uid = ? ORDER BY batch_id ASC")
         .binding(BLOB_MAX_INLINE_LENGTH, uid)
         .forEach(row -> result.add(decodeInlineMutationBatch(row.getInt(0), row.getBlob(1))));
@@ -407,7 +417,7 @@ public void performConsistencyCheck() {
   }
 
   /**
-   * Decodes mutation batch bytes obtained via substring. If the blob is too smaller than
+   * Decodes mutation batch bytes obtained via substring. If the blob is smaller than
    * BLOB_MAX_INLINE_LENGTH, executes additional queries to load the rest of the blob.
    *
    * @param batchId The batch ID of the row containing the bytes, for fallback lookup if the value