Add Numeric Add #1368

schmidt-sebastian · 2018-11-08T05:05:28Z

Port of firebase/firebase-android-sdk#105

Unlike Android, this PR doesn't treat INT64 overflows, as JavaScript's precision is limited to 53 bit. We likely can't properly expose the full backend semantics to our customers since we eventually surface the results as a number.

This limitation is currently - along with the rest of the API - going through API review.

mikelehen

LGTM with a few nits!

mikelehen · 2018-11-08T15:36:14Z

packages/firebase/index.d.ts

+     * are subject to precision loss. Furthermore, once processed by the Firestore
+     * backend, all integer operations are capped between -2^63 and 2^63-1.
+     *
+     * If field is not an integer or double, or if the field does not yet exist,


"If field" => "If the field"

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

packages/firebase/index.d.ts

+     * Returns a special value that can be used with set() or update() that tells
+     * the server to add the given value to the field's current value.
+     *
+     * If either the operand or the current field value is of type double, both


FWIW it's tempting to try to avoid using "double" because JavaScript just has Numbers (and so we should just talk about integer and non-integer numbers). But the backend has doubles, so I guess this needs to cover that. Anyway, it's going to be subtly confusing no matter how we explain it, so this is probably fine. :-)

I changed this section to:

* If either the operand or the current field value uses floating point * precision, all arithmetic will follow IEEE 754 semantics. If both values * are integers, values outside of JavaScript's safe number range * (`Number.MIN_SAFE_INTEGER` to `Number.MAX_SAFE_INTEGER`) are also subject * to precision loss. Furthermore, once processed by the Firestore backend, * all integer operations are capped between -2^63 and 2^63-1. * * If the current field value is not of type number, or if the field does not * yet exist, the transformation will set the field to the given value.

mikelehen · 2018-11-08T15:44:31Z

packages/firestore/src/api/field_value.ts

@@ -50,6 +53,11 @@ export abstract class FieldValueImpl implements firestore.FieldValue {
    return new ArrayRemoveFieldValueImpl(elements);
  }

+  static numericAdd(n: number): FieldValueImpl {


This should probably use validateExactNumberOfArgs() to verify we got 1 arg. delete() and serverTimestamp() should probably be verifying no args.

Done. I added a validateNoArgs and used it in delete() and serverTimestamp().

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

packages/firestore/src/local/indexeddb_schema.ts

+     * non-idempotent write before the write is removed from the queue.
+     *
+     * These mutations are never sent to the backend and serialized via
+     * JsonProtoSerializer.toMutation().


This kind of sounds like you're saying they're never serialized via JsonProtoSerializer.toMutation(). In general it seems awkward to combine these two statements into one sentence. They don't seem very related.

I removed the last part of this.

mikelehen · 2018-11-08T15:55:41Z

packages/firestore/src/local/local_store.ts

+    const keys = mutations.reduce(
+      (keys, m) => keys.add(m.key),
+      documentKeySet()
+    );


s/Nice/Oh... Nice/g

mikelehen · 2018-11-08T16:07:37Z

packages/firestore/src/local/local_store.ts

+                const fieldMask = mutation.fieldMask;
+                if (fieldMask) {
+                  if (maybeDoc instanceof Document) {
+                    const baseValues = fieldMask.applyTo(maybeDoc.data);


Can you port the latest Android? (Assign baseValues using ternary operator and include comment about Precondition.exists(true)).

mikelehen · 2018-11-08T16:08:14Z

packages/firestore/src/local/memory_persistence.ts

@@ -228,6 +228,7 @@ export class MemoryEagerDelegate implements ReferenceDelegate {
    return cache
      .getMatchingKeysForTargetId(txn, queryData.targetId)
      .next(keys => {
+        console.log('Found matching keys ' + keys.size);


Shouldn't tslint yell about this? (and throughout the rest of this PR)

Uhm. Sorry. I had a bunch of GitHub trouble where it wouldn't show my latest changes. Looks like I rebasing my stashed changes from last week solved my troubles :/

mikelehen · 2018-11-08T16:17:38Z

packages/firestore/src/model/transform_operation.ts

+      return new DoubleValue(sum);
+    }
+
+    // If the existing value is not a number, use the value of the transform as


Can you put this in an else block? (since I don't like early return and conceptually I think there are 3 possible cases here and we should just structure the code as such)

mikelehen · 2018-11-08T16:22:42Z

packages/firestore/test/unit/specs/write_spec.test.ts

@@ -595,7 +595,7 @@ describeSpec('Writes:', [], () => {

  specTest(
    'Writes are released when there are no queries left',
-    ['eager-gc'],
+    ['eager-gc','exclusive'],


Mila must have pushed to this branch. Investigating.

schmidt-sebastian

Feedback addressed, and random changes reverted.

schmidt-sebastian · 2018-11-08T19:14:36Z

packages/firebase/index.d.ts

+     * Returns a special value that can be used with set() or update() that tells
+     * the server to add the given value to the field's current value.
+     *
+     * If either the operand or the current field value is of type double, both


I changed this section to:

* If either the operand or the current field value uses floating point * precision, all arithmetic will follow IEEE 754 semantics. If both values * are integers, values outside of JavaScript's safe number range * (`Number.MIN_SAFE_INTEGER` to `Number.MAX_SAFE_INTEGER`) are also subject * to precision loss. Furthermore, once processed by the Firestore backend, * all integer operations are capped between -2^63 and 2^63-1. * * If the current field value is not of type number, or if the field does not * yet exist, the transformation will set the field to the given value.

schmidt-sebastian · 2018-11-08T19:14:39Z

packages/firebase/index.d.ts

+     * are subject to precision loss. Furthermore, once processed by the Firestore
+     * backend, all integer operations are capped between -2^63 and 2^63-1.
+     *
+     * If field is not an integer or double, or if the field does not yet exist,


schmidt-sebastian · 2018-11-08T19:25:46Z

packages/firestore/src/api/field_value.ts

@@ -50,6 +53,11 @@ export abstract class FieldValueImpl implements firestore.FieldValue {
    return new ArrayRemoveFieldValueImpl(elements);
  }

+  static numericAdd(n: number): FieldValueImpl {


Done. I added a validateNoArgs and used it in delete() and serverTimestamp().

schmidt-sebastian · 2018-11-08T19:26:23Z

packages/firestore/src/local/indexeddb_schema.ts

+     * non-idempotent write before the write is removed from the queue.
+     *
+     * These mutations are never sent to the backend and serialized via
+     * JsonProtoSerializer.toMutation().


I removed the last part of this.

schmidt-sebastian · 2018-11-08T19:26:38Z

packages/firestore/src/local/local_store.ts

+    const keys = mutations.reduce(
+      (keys, m) => keys.add(m.key),
+      documentKeySet()
+    );


s/Nice/Oh... Nice/g

schmidt-sebastian · 2018-11-08T19:27:22Z

packages/firestore/src/local/memory_persistence.ts

@@ -228,6 +228,7 @@ export class MemoryEagerDelegate implements ReferenceDelegate {
    return cache
      .getMatchingKeysForTargetId(txn, queryData.targetId)
      .next(keys => {
+        console.log('Found matching keys ' + keys.size);


Uhm. Sorry. I had a bunch of GitHub trouble where it wouldn't show my latest changes. Looks like I rebasing my stashed changes from last week solved my troubles :/

schmidt-sebastian · 2018-11-08T19:27:47Z

packages/firestore/src/model/transform_operation.ts

+      return new DoubleValue(sum);
+    }
+
+    // If the existing value is not a number, use the value of the transform as


schmidt-sebastian · 2018-11-08T19:28:40Z

packages/firestore/test/unit/specs/write_spec.test.ts

@@ -595,7 +595,7 @@ describeSpec('Writes:', [], () => {

  specTest(
    'Writes are released when there are no queries left',
-    ['eager-gc'],
+    ['eager-gc','exclusive'],


Mila must have pushed to this branch. Investigating.

schmidt-sebastian · 2018-11-08T19:35:13Z

packages/firestore/src/local/local_store.ts

+                const fieldMask = mutation.fieldMask;
+                if (fieldMask) {
+                  if (maybeDoc instanceof Document) {
+                    const baseValues = fieldMask.applyTo(maybeDoc.data);


mikelehen

Thanks, changes look good!

Feiyang1 · 2018-11-08T19:28:44Z

packages/firestore/src/api/field_value.ts

@@ -83,6 +91,12 @@ export class ArrayRemoveFieldValueImpl extends FieldValueImpl {
  }
 }

+export class NumericAddFieldValueImpl extends FieldValueImpl {
+  constructor(readonly _operand: number) {


I would do public readonly to make the intent clear.

Feiyang1 · 2018-11-08T19:37:15Z

packages/firestore/src/api/user_data_converter.ts

@@ -644,6 +646,15 @@ export class UserDataConverter {
      context.fieldTransforms.push(
        new FieldTransform(context.path, arrayRemove)
      );
+    } else if (value instanceof NumericAddFieldValueImpl) {


An alternative if you are not matching super class.

switch( value.constructor) { case NumericAddFieldValueImpl: .... }

schmidt-sebastian · 2018-11-13T20:22:24Z

FYI: Pushed commit to rename numericAdd() to increment().

schmidt-sebastian · 2018-12-26T17:27:13Z

PR updated with new Protobuf naming.

mohshraim · 2019-01-16T10:18:58Z

Hello @schmidt-sebastian
We are waiting this important feature..
Any chance to have it in near future beta release?
Thanks

mikelehen · 2019-01-16T17:06:31Z

@mohshraim We're waiting on the backend changes to fully deploy. Sorry for the delay.

schmidt-sebastian requested review from bojeil-google, depoll, Feiyang1, gsoltis, hiranya911, mikelehen, rsgowman and var-const as code owners November 8, 2018 05:05

google-oss-bot added the needs-triage label Nov 8, 2018

schmidt-sebastian requested review from wilhuff and zxu123 as code owners November 8, 2018 05:05

Add Numeric Add

03e0063

schmidt-sebastian force-pushed the mrschmidt-increment3 branch from 30eea3f to 03e0063 Compare November 8, 2018 05:06

schmidt-sebastian assigned mikelehen Nov 8, 2018

mikelehen suggested changes Nov 8, 2018

View reviewed changes

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

Review comments

4f835a4

schmidt-sebastian commented Nov 8, 2018

View reviewed changes

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

mikelehen approved these changes Nov 8, 2018

View reviewed changes

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

Feiyang1 approved these changes Nov 8, 2018

View reviewed changes

Feiyang1 force-pushed the master branch from 54b304a to 6b3754f Compare November 9, 2018 00:11

schmidt-sebastian changed the title ~~Add Numeric Add~~ DO NOT MERGE Add Numeric Add Nov 12, 2018

Rename numericAdd() to increment()

2a9ce2a

schmidt-sebastian mentioned this pull request Nov 14, 2018

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

Merged

schmidt-sebastian added 3 commits December 26, 2018 09:19

Merge branch 'master' into mrschmidt-increment3

83ddee8

Use increment in Protobuf message

e62de99

[AUTOMATED]: Prettier Code Styling

06ff0d3

Remove unused 'size' varibale

9c4490c

wilhuff mentioned this pull request Jan 10, 2019

FR: firebase.firestore.FieldValue.increment(value) #1471

Closed

schmidt-sebastian added 3 commits March 6, 2019 11:35

Merge

d83d07a

Test fix

81332de

[AUTOMATED]: Prettier Code Styling

3e64e2f

schmidt-sebastian requested a review from hsubox76 as a code owner March 6, 2019 20:08

schmidt-sebastian changed the title ~~DO NOT MERGE Add Numeric Add~~ Add Numeric Add Mar 6, 2019

schmidt-sebastian merged commit d364569 into master Mar 6, 2019

wilhuff deleted the mrschmidt-increment3 branch March 11, 2019 21:01

firebase locked and limited conversation to collaborators Oct 15, 2019

Add Numeric Add #1368

Add Numeric Add #1368

Conversation

schmidt-sebastian commented Nov 8, 2018 • edited Loading

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

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

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

Choose a reason for hiding this comment

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

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

schmidt-sebastian commented Nov 13, 2018

schmidt-sebastian commented Dec 26, 2018

mohshraim commented Jan 16, 2019

mikelehen commented Jan 16, 2019

schmidt-sebastian commented Nov 8, 2018 •

edited

Loading