Store the coerced base value in the BaseMutation #564

schmidt-sebastian · 2019-06-24T20:58:46Z

Fixes #491

The PR changes what we store in a BaseMutation. With the changes here, every transform operation computes what it needs to store in the BaseMutation. For Numeric increment, this is either an existing numeric value or the IntegerValue.of(0), replacing all invalid base values at time of creation.

schmidt-sebastian · 2019-06-24T22:32:34Z

/test new-smoke-tests

mikelehen

Mostly LGTM with nits, some naming angst, and a discovered bug in our base mutation logic. We can probably punt on the latter...

firebase-firestore/CHANGELOG.md

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

...tore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java

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

mikelehen · 2019-06-25T17:06:51Z

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

+        previousValue = ((Document) maybeDoc).getField(transform.getFieldPath());
+      }
+
+      FieldValue value = transform.getOperation().computeBaseValue(previousValue);


We have a lot of "value" stuff going on here: extractBaseValue(), baseValue, previousValue, value, computeBaseValue() and it's a bit misleading and hard to keep track of. I think we're probably overloading "baseValue" a bit. You tried to differentiate via verb (compute vs extract), but it might be better to change the noun...

So I'm wondering if we could rename "baseValue" in "extractBaseValue()" to make it a bit clearer that it's a "composite" value to be used as a patch mutation... extractBaseMutationValue()? extractBaseValues()?

Idunno. WDYT?

baseValue => baseObject, previousValue => existingValue, value => coercedValue (?)

Maaaaaaybe. The current code includes all of the following in different places: existingValue, coercedValue, previousValue, currentValue, baseObject, baseValue, maybeDoc, baseDoc ... which I'm pretty certain is more terms than we have concepts.

Can you see if we can get rid of previousValue in applyToLocalView()? And then I'll take another look at the naming.

...firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformOperation.java

schmidt-sebastian

Addressed your feedback and found away to fix the bug you found (which will forever be known as "@mikelehen's bug").

firebase-firestore/CHANGELOG.md

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

...tore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java

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

schmidt-sebastian · 2019-06-25T20:40:47Z

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

+        previousValue = ((Document) maybeDoc).getField(transform.getFieldPath());
+      }
+
+      FieldValue value = transform.getOperation().computeBaseValue(previousValue);


baseValue => baseObject, previousValue => existingValue, value => coercedValue (?)

...firestore/src/main/java/com/google/firebase/firestore/model/mutation/TransformOperation.java

mikelehen

I'm not convinced we need both previousValue and currentValue (even though things break if I try to get rid of previousValue)... Change my mind.

mikelehen · 2019-06-25T21:33:50Z

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

-   * <p>Base values provide consistent behavior for non-idempotent transforms and allow us to return
-   * the same latency-compensated value even if the backend has already applied the mutation. The
-   * base value is null for idempotent mutations, as they can be re-played even if the backend has
+   * <p>The base value is a sparse objects that consists of only the document fields that for which


objects => object
that for which => for which

mikelehen · 2019-06-25T22:21:27Z

...ore/src/main/java/com/google/firebase/firestore/model/mutation/ServerTimestampOperation.java

+  public FieldValue applyToLocalView(
+      @Nullable FieldValue currentValue,
+      @Nullable FieldValue previousValue,
+      Timestamp localWriteTime) {
    return new ServerTimestampValue(localWriteTime, previousValue);


So this is the only implementation of applyToLocalView() that uses previousValue instead of currentValue, and I was surprised that it does... It seems reasonable to expect that if you do:

db.batch() .update(ref, "foo", "old") .update(ref, "foo", FieldValue.serverTimestamp() .commit();

then the ServerTimestampBehavior.PREVIOUS value would be "old" (i.e. currentValue) rather than whatever the value was before the batch...

But I see if you change this to currentValue then we have a test that breaks... But I think it breaks for a bad reason (a deeper bug in our code, possibly in the way UserDataConverter splits updates into a PatchMutation and a TransformMutation)

I would be a fan of:

Getting rid of previousValue and always using currentValue. This will clean up the code and make it much easier to reason about. FWIW I had a hard time groking previousValue and currentValue since they're both "previous" values. More explicit names might be preBatchValue and preMutationValue... This might technically be a breaking change for ServerTimestampBehavior.PREVIOUS in the case I demonstrated above, but I think it's probably subtle enough we don't care.

Tracking down / fixing whatever the bug that exposes. :-) [sorry for the extra homework...]

The problem the currentValue/previousValue "hack" addresses is actually different from what you suspect. A problematic call could be something simple as:

docRef.update(map("nested", map("time", FieldValue.serverTimestamp())

This turns into two mutations in a single mutation batch:

A PatchMutation that replaces any existing value of "nested" with map().

A Transform mutation that sets nested.time to a ServerTimestamp.

By the time the Transform is applied, maybeDoc no longer contains the previous field value.

I added a comment to the test that explains this.

I do think we can improve our naming here, but I don't feel that re-working this entire setup is time well spent.

mikelehen · 2019-06-25T22:27:41Z

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

+        previousValue = ((Document) maybeDoc).getField(transform.getFieldPath());
+      }
+
+      FieldValue value = transform.getOperation().computeBaseValue(previousValue);


Maaaaaaybe. The current code includes all of the following in different places: existingValue, coercedValue, previousValue, currentValue, baseObject, baseValue, maybeDoc, baseDoc ... which I'm pretty certain is more terms than we have concepts.

Can you see if we can get rid of previousValue in applyToLocalView()? And then I'll take another look at the naming.

mikelehen

I am okaaay with this, but I'm still concerned that mutation application is growing more complicated and having applyToLocalView() take both a previousValue and a currentValue is difficult to reason about. I understand your hesitance to let this fix snowball into a bigger refactor, but FWIW I would be okay with just sitting on the fix until we have time to think harder about the naming / structure. Assigning to @wilhuff to take a (quick?) pass over this for a second opinion. If he thinks this is sane enough for now, then I'm fine with it.

If we decide to rename / refactor, I think although a bit awkward, renaming currentValue => preMutationValue and previousValue => preBatchValue would help some. Going a step further, we could refactor localWriteTime and previousValue into a MutationContext object so that they're less prominent (since they're ignored everywhere except ServerTimestampTransform).

mikelehen · 2019-06-26T17:23:23Z

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

@@ -181,6 +181,12 @@ public void testServerTimestampsCanReturnPreviousValue() {
    DocumentSnapshot previousSnapshot = accumulator.awaitRemoteEvent();
    verifyTimestampsAreResolved(previousSnapshot);

+    // The following update includes an update of tge nested map "deep", which updates it to contain


tge => the

You might consider configuring your IDE to make typos more prominent. 😛

wilhuff

I know we discussed at length in chat so I won't recap that here.

I'm OK with the idea of using the value from the prior mutation batch as a way to prevent the implicit patch introduced by a server timestamp from clearing previousValue before we can use it. It's a workaround for sure, but it would fix this user-visible issue without a major redesign.

However, I'm concerned that the blast radius of this change is much larger than just that conceptual change. I've highlighted two cases where we could possibly narrow the scope of the PR.

wilhuff · 2019-06-26T21:21:11Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/value/ObjectValue.java

@@ -57,6 +60,31 @@ public int typeOrder() {
    return TYPE_ORDER_OBJECT;
  }

+  /** Recursively extracts the FieldPaths that are set in this ObjectValue. */
+  public FieldMask getFieldMask() {


Why did the way mutations calculate field masks need to change?

Before, we used the mutation's FieldMask to compute the baseMutation, which caused us to over-select fields. With this PR, each mutation returns the precise set of field it cares about in an ObjectValue, but we still need to provide the field mask to the PatchMutation.

wilhuff · 2019-06-26T21:24:08Z

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

@@ -207,7 +211,12 @@ private Document requireDocument(@Nullable MaybeDocument maybeDoc) {
        previousValue = ((Document) baseDoc).getField(fieldTransform.getFieldPath());
      }

-      transformResults.add(transform.applyToLocalView(previousValue, localWriteTime));
+      FieldValue currentValue = null;


It seems as if we could use currentValue if available or fall back on previousValue if not currentValue is not available. At that point the signatures for the TransformOperations wouldn't have to change.

mikelehen · 2019-06-28T03:34:15Z

firebase-firestore/CHANGELOG.md

@@ -2,6 +2,8 @@
 - [fixed] Fixed an internal assertion that was triggered when an
  update with a `FieldValue.serverTimestamp()` and an update with a
  `FieldValue.increment()` were pending for the same document (#491).
+- [changed] Improved performance for queries with filters that only return a
+  small subset of the documents in a collection.


I'm not sure this is a good description of this fix.

This is the merge commit...

Whoops... 😂 SORRY! When I opened the PR, github took me to https://github.com/firebase/firebase-android-sdk/pull/564/files/700c7c155505a388cc4f902c30e46751bbc6607a..bec9bd64fc3c8be10bb90cdf465820a3766844d4 which shows the added changelog as part of your diff... I thought github was smarter than that. :-/

Store the coerced base value in BaseMutation

031212b

googlebot added cla: yes Override cla labels Jun 24, 2019

google-oss-bot added the size/L label Jun 24, 2019

schmidt-sebastian requested a review from mikelehen June 24, 2019 21:00

schmidt-sebastian assigned mikelehen Jun 24, 2019

schmidt-sebastian added the api: firestore label Jun 24, 2019

mikelehen suggested changes Jun 25, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jun 25, 2019

schmidt-sebastian added 2 commits June 25, 2019 13:25

Support two increments in a batch

bc71889

Renames/Comments cleanup

d16d343

schmidt-sebastian commented Jun 25, 2019

View reviewed changes

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jun 25, 2019

mikelehen suggested changes Jun 25, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jun 25, 2019

Comment

a502f18

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jun 26, 2019

mikelehen approved these changes Jun 26, 2019

View reviewed changes

mikelehen assigned wilhuff and unassigned mikelehen Jun 26, 2019

Fix typo in comment

700c7c1

wilhuff reviewed Jun 26, 2019

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jun 26, 2019

Merge

7624a3b

Use currentValue or previousValue

bec9bd6

schmidt-sebastian merged commit b45353e into master Jun 28, 2019

mikelehen reviewed Jun 28, 2019

View reviewed changes

schmidt-sebastian mentioned this pull request Jun 28, 2019

Store the coerced base value in the BaseMutation firebase/firebase-js-sdk#1928

Merged

schmidt-sebastian deleted the mrschmidt/basevalues branch July 3, 2019 15:46

firebase locked and limited conversation to collaborators Oct 9, 2019

Store the coerced base value in the BaseMutation #564

Store the coerced base value in the BaseMutation #564

Uh oh!

Conversation

schmidt-sebastian commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian commented Jun 24, 2019

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikelehen 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!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff 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!

schmidt-sebastian commented Jun 24, 2019 •

edited

Loading

schmidt-sebastian left a comment •

edited

Loading