Adding FieldValue.numericAdd() #105

schmidt-sebastian · 2018-11-01T22:26:16Z

This PR adds support for the Numeric Add transform and follows the semantics of the backend implementation. To avoid flicker with latency compensation, it adds the concept of "base values", which are stored as a list of patch mutations along with the transform mutations in a mutation batch. The base mutations are applied first and are used to overwrite values from the remote document cache.

This PR also simplifies some plumbing in regards to the "previous value", which is used when rendering Server Timestamps. Before, if two server timestamps replaced the same value in sequence, each timestamp would point to this previous value. Now, the previous value is chained, and the second timestamp obtains its previous value by asking the first timestamp for its previous value. This is slightly less optimal, but simplifies the changes that this PR introduces to Local Store.

As an additional benefit, and to get this PR passed 1000 lines of code, I added a flag to run the Integration Tests against Hexa. Because I don't know how to add custom configurations in gradle, this is for now hardcoded.

schmidt-sebastian · 2018-11-02T00:49:22Z

Hm, looks like this broke testServerTimestampsCanReturnPreviousValue. I'll take a look.

schmidt-sebastian · 2018-11-02T03:55:58Z

Hm, looks like this broke testServerTimestampsCanReturnPreviousValue. I'll take a look.

I reverted the change that simplified the plumbing as this broke updates for transforms that were nested inside of a map.

mikelehen

On the whole, this looks fantastic. The baseMutations approach made this much less disruptive than I expected. Nice work! I have one potential concern about merge-set behavior, but we can perhaps punt on that. Otherwise, just lots of minor nits.

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

firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto

firebase-firestore/src/main/java/com/google/firebase/firestore/FieldValue.java

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

...base-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java

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

schmidt-sebastian

Addressed most feedback. Feel free to push back on some of my decisions :)

...base-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java

...restore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java

firebase-firestore/src/main/java/com/google/firebase/firestore/FieldValue.java

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

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

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

...restore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java

mikelehen

Thanks! Mild pushback / clarification on a couple things.

mikelehen · 2018-11-06T14:21:10Z

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

+   * @return The mutations that are used to populate the base values when this mutation batch is
+   *     applied locally.
+   */
+  public List<Mutation> getBaseMutations() {


"There may be more suitable use cases for this in the future, in which case the more general name may make more sense." Can you provide an example of such a use case?

Offhand I strongly suspect this will never be used for anything else, and so I'd err towards present naming clarity rather than futureproof-ing the name. I can be convinced otherwise though.

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

...restore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java

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

mikelehen · 2018-11-06T15:10:08Z

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

+    // Note: Theoretically, we should only include non-idempotent fields in this field mask as this
+    // mask is used to populate the base state for all DocumentTransforms.  By including  all
+    // fields, we incorrectly prevent rebasing of idempotent transforms (such as `arrayUnion()`)
+    // when any non-idempotent transforms are present.


This is a good comment, but it may not make sense out-of-context (it assumes knowledge of how this method is going to be used). I'd actually move it into LocalStore where getFieldMask() is called rather than having it here.

Yeah, that makes sense. It's basically the same reason as to why I don't want to exclude the idempotent fields here.

mikelehen · 2018-11-06T15:11:42Z

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

@@ -968,6 +968,9 @@ public void testHandlesSetMutationThenTransformMutationThenTransformMutation() {
  @Test
  public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransformMutation() {
    if (garbageCollectorIsEager()) {
+      // Since this test doesn't open a Query, Eager GC removes the documents from the cache as soon


"open a Query" => "start a listen" ?

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

mikelehen

Oh... nice.

mikelehen · 2018-11-08T00:06:41Z

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

+   * @return The mutations that are used to populate the base values when this mutation batch is
+   *     applied locally.
+   */
+  public List<Mutation> getBaseMutations() {


Oh... nice.

mikelehen · 2018-11-08T00:08:44Z

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

+    assertNotContains("foo/bar");
+
+    applyRemoteEvent(addedRemoteEvent(doc("foo/bar", 1, map("sum", 1337)), asList(2), emptyList()));
+    assertChanged(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS));


Oh... nice.

mikelehen · 2018-11-08T00:09:34Z

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

+                          mutation.getKey(),
+                          ObjectValue.emptyObject(),
+                          fieldMask,
+                          mutation.getPrecondition()));


Oh... nice.

wilhuff

proto changes: oh... nice!

wilhuff

LGTM

schmidt-sebastian · 2018-11-13T20:03:28Z

FYI: Pushed commit to rename this to "increment" as per the API Council.

google-oss-bot · 2018-12-26T17:33:47Z

@schmidt-sebastian: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
connected-check	`bb10ecb`	link	`/test connected-check`
connected-check-changed	`991fb7c`	link	`/test connected-check-changed`
smoke-tests-release	`991fb7c`	link	`/test smoke-tests-release`

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.

google-oss-bot · 2019-03-06T19:23:48Z

@schmidt-sebastian: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests-release	`468c282`	link	`/test smoke-tests-release`

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.

google-oss-bot · 2019-03-06T19:28:51Z

@schmidt-sebastian: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests-debug	`468c282`	link	`/test smoke-tests-debug`

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.

googlebot added the cla: yes Override cla label Nov 1, 2018

google-oss-bot added the size/XL label Nov 1, 2018

schmidt-sebastian force-pushed the mrschmidt-numericincrement branch 2 times, most recently from f3e1ff8 to 59e73c2 Compare November 1, 2018 23:09

google-oss-bot added size/XXL and removed size/XL labels Nov 1, 2018

schmidt-sebastian force-pushed the mrschmidt-numericincrement branch 2 times, most recently from 0d26c0e to 1ca97b4 Compare November 2, 2018 00:25

schmidt-sebastian requested a review from mikelehen November 2, 2018 00:27

schmidt-sebastian assigned mikelehen Nov 2, 2018

schmidt-sebastian force-pushed the mrschmidt-numericincrement branch from 1ca97b4 to 6eadd27 Compare November 2, 2018 03:52

google-oss-bot added size/XL and removed size/XXL labels Nov 2, 2018

schmidt-sebastian force-pushed the mrschmidt-numericincrement branch from 6eadd27 to acd83e1 Compare November 2, 2018 03:56

Adding FieldValue.numericAdd()

20152fe

schmidt-sebastian force-pushed the mrschmidt-numericincrement branch from acd83e1 to 20152fe Compare November 2, 2018 05:13

Adding NaN and Infinity test

602d03f

google-oss-bot added size/XXL and removed size/XL labels Nov 2, 2018

schmidt-sebastian added 2 commits November 3, 2018 13:49

Use addExact in comment

ef1862b

Adding more Edge case testing

6e386c4

mikelehen suggested changes Nov 4, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Nov 4, 2018

Review feedback

0df4ee8

schmidt-sebastian commented Nov 5, 2018

View reviewed changes

schmidt-sebastian assigned mikelehen Nov 5, 2018

mikelehen suggested changes Nov 6, 2018

View reviewed changes

mikelehen approved these changes Nov 8, 2018

View reviewed changes

wilhuff approved these changes Nov 8, 2018

View reviewed changes

This was referenced Nov 8, 2018

Adding Numeric Add firebase/firebase-js-sdk#1365

Closed

Adding Numeric Add Support firebase/firebase-js-sdk#1367

Closed

Add Numeric Add firebase/firebase-js-sdk#1368

Merged

schmidt-sebastian added 2 commits November 8, 2018 13:42

Enable tests

f4cf039

Running Google Java Format

3f2e09e

wilhuff assigned schmidt-sebastian and unassigned wilhuff Nov 12, 2018

schmidt-sebastian changed the title ~~Adding FieldValue.numericAdd()~~ DO NOT MERGE Adding FieldValue.numericAdd() Nov 12, 2018

schmidt-sebastian mentioned this pull request Nov 13, 2018

Adding base_writes proto field firebase/firebase-ios-sdk#2066

Merged

Update Proto with wording from iOS

261c7c8

wilhuff approved these changes Nov 13, 2018

View reviewed changes

Rename numericAdd to increment

bb10ecb

schmidt-sebastian mentioned this pull request Nov 14, 2018

Adding FieldValue.increment() firebase/firebase-ios-sdk#2075

Merged

Fix wording

5b4f0a2

schmidt-sebastian mentioned this pull request Nov 19, 2018

Adding FieldValue.increment() googleapis/google-cloud-java#4018

Merged

schmidt-sebastian added 2 commits December 26, 2018 08:57

Merge

29758d1

Update Protobuf to use increment()

991fb7c

schmidt-sebastian force-pushed the mrschmidt-numericincrement branch from c7e8172 to 991fb7c Compare December 26, 2018 17:18

schmidt-sebastian added 2 commits March 6, 2019 10:57

Merge

94493b8

v1 Update

468c282

schmidt-sebastian changed the title ~~DO NOT MERGE Adding FieldValue.numericAdd()~~ Adding FieldValue.numericAdd() Mar 6, 2019

schmidt-sebastian merged commit 3823f1d into master Mar 6, 2019

schmidt-sebastian deleted the mrschmidt-numericincrement branch March 12, 2019 23:59

firebase locked and limited conversation to collaborators Oct 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding FieldValue.numericAdd() #105

Adding FieldValue.numericAdd() #105

schmidt-sebastian commented Nov 1, 2018 •

edited

Loading

schmidt-sebastian commented Nov 2, 2018

schmidt-sebastian commented Nov 2, 2018

mikelehen left a comment •

edited

Loading

schmidt-sebastian left a comment

mikelehen left a comment

mikelehen Nov 6, 2018

mikelehen Nov 6, 2018

schmidt-sebastian Nov 6, 2018

mikelehen Nov 6, 2018

schmidt-sebastian Nov 6, 2018

mikelehen left a comment

mikelehen Nov 8, 2018

mikelehen Nov 8, 2018

mikelehen Nov 8, 2018

wilhuff left a comment

wilhuff left a comment

schmidt-sebastian commented Nov 13, 2018

google-oss-bot commented Dec 26, 2018 •

edited

Loading

google-oss-bot commented Mar 6, 2019

google-oss-bot commented Mar 6, 2019

Adding FieldValue.numericAdd() #105

Adding FieldValue.numericAdd() #105

Conversation

schmidt-sebastian commented Nov 1, 2018 • edited Loading

schmidt-sebastian commented Nov 2, 2018

schmidt-sebastian commented Nov 2, 2018

mikelehen left a comment • edited Loading

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

schmidt-sebastian commented Nov 13, 2018

google-oss-bot commented Dec 26, 2018 • edited Loading

google-oss-bot commented Mar 6, 2019

google-oss-bot commented Mar 6, 2019

schmidt-sebastian commented Nov 1, 2018 •

edited

Loading

mikelehen left a comment •

edited

Loading

google-oss-bot commented Dec 26, 2018 •

edited

Loading