Replace usage of transform with updateTransforms #2262

thebrianchen · 2020-12-16T00:22:31Z

Porting from web.

google-oss-bot · 2020-12-16T00:30:53Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 46.63% (ee74cd0) to 46.78% (bc50b600) by +0.14%.

Click to show coverage changes in 12 files.

Filename	Base (`ee74cd0`)	Head (bc50b600)	Diff
DeleteMutation.java	84.21%	83.33%	-0.88%
DocumentTransform.java	29.66%	29.44%	-0.22%
GrpcCallProvider.java	68.18%	57.95%	-10.23%
LocalSerializer.java	96.97%	97.30%	+0.33%
Mutation.java	93.75%	98.41%	+4.66%
PatchMutation.java	91.11%	92.31%	+1.20%
RemoteSerializer.java	83.68%	83.29%	-0.39%
SetMutation.java	80.00%	84.85%	+4.85%
UserData.java	54.37%	66.00%	+11.63%
UserDataReader.java	65.71%	70.48%	+4.76%
VerifyMutation.java	46.15%	50.00%	+3.85%
Write.java	30.63%	36.25%	+5.62%

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 (bc50b600) is created by Prow via merging commits: ee74cd0 8c1b565.

google-oss-bot · 2020-12-16T00:34:45Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (ee74cd0) Head (bc50b600) Diff

aar 1.00 MB 1.00 MB -1.70 kB (-0.2%)

apk (release) 3.15 MB 3.15 MB -304 B (-0.0%)

Test Logs

Notes

Head commit (bc50b600) is created by Prow via merging commits: ee74cd0 8c1b565.

schmidt-sebastian

Very nice!

schmidt-sebastian · 2020-12-16T16:36:49Z

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

+                && batch.getWrites(i - 1).getTransform().getFieldTransformsCount() == 0
+                && batch.getWrites(i - 1).getUpdate().getFieldsCount() != 0,


Suggested change

&& batch.getWrites(i - 1).getTransform().getFieldTransformsCount() == 0

&& batch.getWrites(i - 1).getUpdate().getFieldsCount() != 0,

&& !batch.getWrites(i - 1).hasTransform()

&& batch.getWrites(i - 1).hasUpdate(),

Proto3 does have "has" functions for nested messages.

schmidt-sebastian · 2020-12-16T16:38:42Z

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

+                .removeWrites(i)
+                .removeWrites(i - 1)
+                .addWrites(i - 1, newMutationBuilder.build())
+                .build();


Can you change this so it only runs once per batch?

schmidt-sebastian · 2020-12-16T16:40:59Z

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

- * if the prior mutation resulted in a Document (always true for a SetMutation, but not necessarily
- * for an PatchMutation).
+ * <p>Field transforms are used only with Patch and Set Mutations. We use the `updateTransforms`
+ * field to store transforms, rather than the `transforms` field.


Nit: "transforms field" might be a bit misleading. Maybe rephrase as "rather than sending an additional transforms message"

schmidt-sebastian · 2020-12-16T16:41:56Z

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

@@ -129,7 +138,9 @@ public abstract MaybeDocument applyToLocalView(
   * @return a base value to store along with the mutation, or null for idempotent mutations.
   */
  @Nullable
-  public abstract ObjectValue extractBaseValue(@Nullable MaybeDocument maybeDoc);
+  public ObjectValue extractBaseValue(@Nullable MaybeDocument maybeDoc) {
+    return extractTransformBaseValue(maybeDoc);


It looks like we can just inline this call. The Web SDK is slightly different to support tree-shaking.

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

schmidt-sebastian · 2020-12-16T16:52:51Z

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

+        --i;
+      }
+    }
+


Another idea would be to simply build up a new list of Mutations, which we can then iterate in the three lines below. That would avoid all toBuilder() conversions.

Done. Used a builder to accumulate everything before building at the end of the loop

It looks like prepending an element to a list in Protobuf is an O(n) operation. Would it be possible to reverse this loop so we can append at the end? We could also just scrap the Builder entirely and just build up mutations directly (see the 5 lines below).

ended up using mutations directly.

schmidt-sebastian · 2020-12-16T16:57:11Z

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

@@ -59,13 +67,204 @@
  private RemoteSerializer remoteSerializer;
  private LocalSerializer serializer;

+  private Timestamp writeTime = Timestamp.now();
+  com.google.protobuf.Timestamp writeTimeProto =


Should this be private as well>

schmidt-sebastian · 2020-12-16T16:57:25Z

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

+          .setNanos(writeTime.getNanoseconds())
+          .build();
+
+  static class TestWriteBuilder {


learning from the best :)

schmidt-sebastian · 2020-12-16T16:58:30Z

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

+    MutationBatch decoded = serializer.decodeMutationBatch(batchProto);
+    assertEquals(1, decoded.getMutations().size());
+    assertTrue(decoded.getMutations().get(0) instanceof SetMutation);
+    Write serialized = remoteSerializer.encodeMutation(decoded.getMutations().get(0));


Maybe s/serialized/encoded ?

schmidt-sebastian · 2020-12-16T17:03:48Z

firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java


+    mutation =


Super optional nit (here and below): To me, set is the base case and patch is more advanced. I would flip the order of these assertions.

done here and below

thebrianchen added the api: firestore label Dec 16, 2020

thebrianchen self-assigned this Dec 16, 2020

google-cla bot added the cla: yes Override cla label Dec 16, 2020

google-oss-bot added the size/XXL label Dec 16, 2020

add update_transforms

283773f

thebrianchen force-pushed the bc/update-transforms branch from ad6ddee to 283773f Compare December 16, 2020 00:24

thebrianchen requested a review from schmidt-sebastian December 16, 2020 00:43

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Dec 16, 2020

schmidt-sebastian reviewed Dec 16, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Dec 16, 2020

resolve comments

b82f471

thebrianchen requested a review from schmidt-sebastian December 17, 2020 18:35

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Dec 17, 2020

schmidt-sebastian assigned thebrianchen Dec 17, 2020

remove builder usage

107e911

thebrianchen removed their assignment Dec 17, 2020

schmidt-sebastian approved these changes Dec 17, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Dec 17, 2020

fix nits

8c1b565

thebrianchen merged commit d40e7f1 into master Dec 17, 2020

thebrianchen deleted the bc/update-transforms branch December 17, 2020 22:40

thebrianchen mentioned this pull request Dec 21, 2020

Backport Android updateTransform changes firebase/firebase-js-sdk#4225

Merged

thebrianchen mentioned this pull request Jan 14, 2021

Replace usage of transforms with update transforms firebase/firebase-ios-sdk#7276

Merged

firebase locked and limited conversation to collaborators Jan 17, 2021

		&& batch.getWrites(i - 1).getTransform().getFieldTransformsCount() == 0
		&& batch.getWrites(i - 1).getUpdate().getFieldsCount() != 0,

Replace usage of transform with updateTransforms #2262

Replace usage of transform with updateTransforms #2262

Uh oh!

Conversation

thebrianchen commented Dec 16, 2020

Uh oh!

google-oss-bot commented Dec 16, 2020 • 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 Dec 16, 2020 • 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 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!

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!

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!

google-oss-bot commented Dec 16, 2020 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Dec 16, 2020 •

edited

Loading

`firebase-firestore`