Adding FieldValue.increment() #2075

schmidt-sebastian · 2018-11-13T23:09:25Z

I am sending this out for early review, so I can get it merged by Friday. I haven't been able to run the integration tests (written on the bus to Santa Cruz), but I would appreciate some early comments.

Port of firebase/firebase-android-sdk#105 and firebase/firebase-js-sdk#1368

var-const

The most important comment is about detecting signed integer overflow.

var-const · 2018-11-14T19:17:02Z

Firestore/core/src/firebase/firestore/model/field_transform.h

    return *transformation_.get();
  }

+  const bool idempotent() const {


Nit: I don't think there is ever a reason to return a const primitive. It doesn't actually affect the behavior from the point of view of the caller who may assign the return value to either a const or a non-const variable. For non-primitive types, it's also generally isn't advisable, because const would prevent moving:

std::vector<int> make_huge_vector(); const std::vector<int> make_huge_vector_const(); auto foo = make_huge_vector(); // The vector is moved into foo auto bar = make_huge_vector_const(); // No move here -- return value optimization should help, but it's not guaranteed

See Core Guidelines for additional details:

It is not recommended to return a const value. Such older advice is now obsolete; it does not add value, and it interferes with move semantics

Yeah, I didn't necessarily mean to do this. This might just have been more of a copy&paste snafu. Thanks for explaining!

var-const · 2018-11-14T19:41:12Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+
+- (void)expectApproximateLocalAndRemoteValue:(double)expectedSum {
+  FIRDocumentSnapshot *snap = [_accumulator awaitLocalEvent];
+  XCTAssertEqual(@(expectedSum), snap[@"sum"]);


Should this be XCTAssertEqualWithAccuracy?

Yes. Fixed.

I should have probably asked you not to review these tests until I was able to run them.

var-const · 2018-11-14T19:42:53Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+- (void)testIntegerIncrementWithExistingInteger {
+  [self writeInitialData:@{@"sum" : @1337}];
+  [self updateDocumentRef:_docRef data:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}];
+  [self expectLocalAndRemoteValue:13378];


s/13378/1338

var-const · 2018-11-14T19:44:05Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+  [self writeInitialData:@{@"sum" : @"overwrite"}];
+  [self updateDocumentRef:_docRef
+                     data:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1337]}];
+  [self expectLocalAndRemoteValue:1337];


Unrelated to this PR: why was it decided to overwrite instead of reject in this case?

The backend semantics for NUMERIC_ADD follow those of ARRA_UNION and coerce to the expected type first. This ensures consistency between all document transforms.

var-const · 2018-11-14T19:45:11Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+  [self expectApproximateLocalAndRemoteValue:14.37];
+}
+
+- (void)testDoubleIncrementWithExistingInteger {


Nit: maybe replace "double" in test name with "float", "floating point", or "real"? "DoubleIncrement" reads like "IncrementTwice", not "IncrementByADouble".

I am going to push back on this suggestions purely out of laziness. I want these tests names to match across all platforms, and changing one name here requires me to port this change to Android and Web. Thanks for flagging this though.

var-const · 2018-11-14T21:34:00Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+class NumericIncrementTransform : public TransformOperation {
+ public:
+  NumericIncrementTransform(FSTNumberValue* operand)
+      : operand_(std::move(operand)) {


Nit: std::move here is effectively a no-op (see one of the comments above).

var-const · 2018-11-14T21:34:44Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+      : operand_(std::move(operand)) {
+  }
+
+  ~NumericIncrementTransform() override {


Nit: defining an empty destructor shouldn't be necessary (unless the compiler complains, in which case can you copy the warning message?).

Removed from here and from all other TransformOperation subclasses.

var-const · 2018-11-14T21:38:17Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+    if ([previousValue isKindOfClass:[FSTIntegerValue class]] &&
+        [operand_ isKindOfClass:[FSTIntegerValue class]]) {
+      int64_t sum = SafeIncrement(
+          (reinterpret_cast<FSTIntegerValue*>(previousValue)).internalValue,


Under C++ rules, static_cast would be sufficient throughout the function (it is valid to static_cast a base class to a derived class). Unless it doesn't play well with Objective-C, static_cast is preferable -- in general, it is best to use the least powerful cast applicable.

I'm still trying to wrap my head around this, but updated this to use static_cast.

var-const · 2018-11-14T21:52:11Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+   * Implements integer addition. Overflows are resolved to LONG_MAX/LONG_MIN.
+   */
+  int64_t SafeIncrement(int64_t x, int64_t y) const {
+    int64_t r = x + y;


This is undefined behavior, and it's not safe to rely on any particular value of r. See this thread for some suggestions. Alternatively, a larger int type would be helpful, but unfortunately Abseil only has uint128, not int128, for the time being (I guess a workaround would be to use modulo, but it might end up harder to implement than checking).

Ugh. Thanks for pointing this out. At least on Mac with LVM, it seems to work :)

I went with one of the suggestions from the SO post, which is actually pretty reasonable: Check for overflow/underflow before it occurs.

var-const · 2018-11-14T21:52:42Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+
+  double OperandAsDouble() const {
+    if ([operand_ isKindOfClass:[FSTDoubleValue class]]) {
+      return (reinterpret_cast<FSTDoubleValue*>(operand_)).internalValue;


Same comment about reinterpret_cast.

schmidt-sebastian

Thanks for your thorough review. (Most) comments addressed. The tests are passing against Hexa now.

schmidt-sebastian · 2018-11-15T19:31:44Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+
+- (void)expectApproximateLocalAndRemoteValue:(double)expectedSum {
+  FIRDocumentSnapshot *snap = [_accumulator awaitLocalEvent];
+  XCTAssertEqual(@(expectedSum), snap[@"sum"]);


Yes. Fixed.

I should have probably asked you not to review these tests until I was able to run them.

schmidt-sebastian · 2018-11-15T19:36:20Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+  [self expectApproximateLocalAndRemoteValue:14.37];
+}
+
+- (void)testDoubleIncrementWithExistingInteger {


I am going to push back on this suggestions purely out of laziness. I want these tests names to match across all platforms, and changing one name here requires me to port this change to Android and Web. Thanks for flagging this though.

schmidt-sebastian · 2018-11-15T19:37:34Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+  [self writeInitialData:@{@"sum" : @"overwrite"}];
+  [self updateDocumentRef:_docRef
+                     data:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1337]}];
+  [self expectLocalAndRemoteValue:1337];


The backend semantics for NUMERIC_ADD follow those of ARRA_UNION and coerce to the expected type first. This ensures consistency between all document transforms.

schmidt-sebastian · 2018-11-15T19:38:52Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+  [self expectApproximateLocalAndRemoteValue:13.37];
+}
+
+- (void)testMultipleDoubleIncrements {


These tests here test the integration with the backend, and as such am I inclined to not add too much edge case testing. I am testing the internal logic that deals with NaN and Infinity in the unit tests.

schmidt-sebastian · 2018-11-15T19:39:04Z

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

+  [self updateDocumentRef:_docRef
+                     data:@{@"sum" : [FIRFieldValue fieldValueForDoubleIncrement:0.01]}];
+  [self updateDocumentRef:_docRef
+                     data:@{@"sum" : [FIRFieldValue fieldValueForDoubleIncrement:0.01]}];


Yes, that's correct. Fixed.

schmidt-sebastian · 2018-11-15T22:34:35Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+    if ([previousValue isKindOfClass:[FSTIntegerValue class]] &&
+        [operand_ isKindOfClass:[FSTIntegerValue class]]) {
+      int64_t sum = SafeIncrement(
+          (reinterpret_cast<FSTIntegerValue*>(previousValue)).internalValue,


I'm still trying to wrap my head around this, but updated this to use static_cast.

schmidt-sebastian · 2018-11-15T22:34:55Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+
+  double OperandAsDouble() const {
+    if ([operand_ isKindOfClass:[FSTDoubleValue class]]) {
+      return (reinterpret_cast<FSTDoubleValue*>(operand_)).internalValue;


schmidt-sebastian · 2018-11-15T22:42:02Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+class NumericIncrementTransform : public TransformOperation {
+ public:
+  NumericIncrementTransform(FSTNumberValue* operand)
+      : operand_(std::move(operand)) {


schmidt-sebastian · 2018-11-15T22:42:20Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+      : operand_(std::move(operand)) {
+  }
+
+  ~NumericIncrementTransform() override {


Removed from here and from all other TransformOperation subclasses.

schmidt-sebastian · 2018-11-15T22:43:44Z

Firestore/core/src/firebase/firestore/model/transform_operations.h

+   * Implements integer addition. Overflows are resolved to LONG_MAX/LONG_MIN.
+   */
+  int64_t SafeIncrement(int64_t x, int64_t y) const {
+    int64_t r = x + y;


Ugh. Thanks for pointing this out. At least on Mac with LVM, it seems to work :)

I went with one of the suggestions from the SO post, which is actually pretty reasonable: Check for overflow/underflow before it occurs.

schmidt-sebastian · 2018-11-16T00:01:42Z

Firestore/Source/Local/FSTLocalStore.mm

+        // NOTE: The base state should only be applied if there's some existing document to
+        // override, so use a Precondition of exists=true
+        [baseMutations addObject:[[FSTPatchMutation alloc] initWithKey:mutation.key
+                                                             fieldMask:FieldMask(*fieldMask)


Would it make sense to std::move the mask into the copy constructor?

That question seems somewhat nonsensical.

If you're copying you shouldn't move since the whole point of copying is to preserve the existing value.

If you want to move don't copy.

Looking at this more closely I think you want to copy, and the automatic copy you get should be sufficient:

[baseMutations addObject:[[FSTPatchMutation alloc] initWithKey:mutation.key fieldMask:*fieldMask value:baseValues precondition:Precondition::Exists(true)]];```

AFAICS, in this case std::move won't have any effect, because fieldMask points to a const object.

std::move is essentially a cast; simplifying a little bit, it's pretty much the same as static_cast<T&&>(foo). The only thing move does is affect the overload resolution:

void Foo(const Bar& bar); // Takes const lvalue reference void Foo(Bar&& bar); // Takes rvalue reference Bar bar; Foo(bar); // Chooses the first overload (const lvalue reference) Foo(std::move(bar)); // Chooses the second overload (rvalue reference)

So in this case, move could affect whether it's the copy constructor or the move constructor that gets selected. However, moving a const value results in a somewhat weird type const Foo&& (const rvalue reference), which cannot bind to a non-const rvalue argument; so when overload is being selected for const Foo&&, Foo&& is out of question, and const Foo& ends up being selected. This means that the copy constructor will be used in this case, which would have happened anyway without the move.

Note that moving an object is a commitment to never use the object again except to reassign to it; moving pretty much gives up possession of the object (at least, that's the case for non-POD objects). It's not unlike setting a pointer to null -- you can reassign a different pointer, but you can no longer access the object that the pointer was pointing to.

Thanks for the thorough explanation. I was worried it would create an extra copy of the FieldMask before it calls the copy-constructor, but now that I understand this better, I am no longer worried. I appreciate your explanation!

var-const · 2018-11-16T00:41:57Z

Firestore/Source/Local/FSTLocalStore.mm

+        // NOTE: The base state should only be applied if there's some existing document to
+        // override, so use a Precondition of exists=true
+        [baseMutations addObject:[[FSTPatchMutation alloc] initWithKey:mutation.key
+                                                             fieldMask:FieldMask(*fieldMask)


AFAICS, in this case std::move won't have any effect, because fieldMask points to a const object.

std::move is essentially a cast; simplifying a little bit, it's pretty much the same as static_cast<T&&>(foo). The only thing move does is affect the overload resolution:

void Foo(const Bar& bar); // Takes const lvalue reference void Foo(Bar&& bar); // Takes rvalue reference Bar bar; Foo(bar); // Chooses the first overload (const lvalue reference) Foo(std::move(bar)); // Chooses the second overload (rvalue reference)

So in this case, move could affect whether it's the copy constructor or the move constructor that gets selected. However, moving a const value results in a somewhat weird type const Foo&& (const rvalue reference), which cannot bind to a non-const rvalue argument; so when overload is being selected for const Foo&&, Foo&& is out of question, and const Foo& ends up being selected. This means that the copy constructor will be used in this case, which would have happened anyway without the move.

Note that moving an object is a commitment to never use the object again except to reassign to it; moving pretty much gives up possession of the object (at least, that's the case for non-POD objects). It's not unlike setting a pointer to null -- you can reassign a different pointer, but you can no longer access the object that the pointer was pointing to.

var-const · 2018-11-16T00:45:26Z

Firestore/Source/Local/FSTLocalStore.mm

+                  : [FSTObjectValue objectValue];
+          // NOTE: The base state should only be applied if there's some existing document to
+          // override, so use a Precondition of exists=true
+          [baseMutations


Thanks for the explanation.

var-const · 2018-11-16T00:48:38Z

Firestore/Source/Local/FSTLocalStore.mm

+    for (FSTMutation *mutation in mutations) {
+      FSTMaybeDocument *maybeDocument = [existingDocuments objectForKey:mutation.key];
+      if (!mutation.idempotent) {
+        // Theoretically, we should only include non-idempotent fields in this field mask as


Sorry, I'm not sure I understand the comment. The way I'm reading it, it seems to say that ideally, we would only include non-idempotent fields, which is sufficient to prevent flicker. However, we also include idempotent fields, which has a downside. What I don't understand is why we include all fields if it has a downside and including only non-idempotent fields is sufficient?

var-const · 2018-11-16T00:51:32Z

Firestore/Example/Tests/Local/FSTLocalStoreTests.mm

+// better implemented as spec tests but currently they don't support transforms.
+
+- (void)testHandlesSetMutationThenTransformMutationThenTransformMutation {
+  if ([self isTestBaseClass]) return;


Thanks for the explanation!

var-const

Thanks for clarifying the comment!

schmidt-sebastian · 2018-12-26T18:59:46Z

PR updated to use MaybeDocumentMap instead of FSTImmutableDocumentMap and to use new increment() Protobuf naming.

wilhuff · 2019-03-06T19:35:15Z

Merged master, including the changes to make MutationQueue a C++ interface. @schmidt-sebastian PTAL and this is ready for merging.

schmidt-sebastian · 2019-03-06T19:46:14Z

Thanks for the merge. PR looks good to me.

googlebot added the cla: yes label Nov 13, 2018

morganchen12 added the api: firestore label Nov 14, 2018

schmidt-sebastian force-pushed the mrschmidt-numericadd branch from 4c0da57 to 680a7e6 Compare November 14, 2018 18:52

schmidt-sebastian requested a review from var-const November 14, 2018 18:55

schmidt-sebastian assigned var-const Nov 14, 2018

schmidt-sebastian force-pushed the mrschmidt-numericadd branch from 680a7e6 to 113c54b Compare November 14, 2018 18:56

Adding Numeric Add

9b42925

schmidt-sebastian force-pushed the mrschmidt-numericadd branch from 113c54b to 9b42925 Compare November 14, 2018 18:57

var-const suggested changes Nov 14, 2018

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Nov 14, 2018

schmidt-sebastian changed the title ~~WIP~~ DO NOT MERGE: Adding FieldValue.increment() Nov 15, 2018

Review comments

8213eeb

schmidt-sebastian commented Nov 16, 2018

View reviewed changes

schmidt-sebastian assigned var-const and unassigned schmidt-sebastian Nov 16, 2018

var-const approved these changes Nov 16, 2018

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Nov 16, 2018

Small review nits in LocalStore

2f7e7e0

var-const approved these changes Nov 16, 2018

View reviewed changes

schmidt-sebastian added 2 commits November 16, 2018 14:05

Merge branch 'master' into mrschmidt-numericadd

29a55da

Addressing lint issue

b74b388

schmidt-sebastian force-pushed the mrschmidt-numericadd branch from dd0303f to b74b388 Compare November 17, 2018 07:04

Merge

c096fe1

wilhuff added 3 commits March 6, 2019 11:26

Merge branch 'master' into mrschmidt-numericadd

4f58e41

style

67c0a05

2019

55a525d

wilhuff changed the title ~~DO NOT MERGE: Adding FieldValue.increment()~~ Adding FieldValue.increment() Mar 6, 2019

changelog

8b43738

wilhuff added 3 commits March 6, 2019 12:13

fix double backtick

c84f263

Use Swift method signatures in CHANGELOG

34e45f1

style with clang-format 2018-08-24

a1c5d12

schmidt-sebastian merged commit 657f53a into master Mar 6, 2019

paulb777 deleted the mrschmidt-numericadd branch April 13, 2019 16:00

firebase locked and limited conversation to collaborators Oct 25, 2019

Adding FieldValue.increment() #2075

Adding FieldValue.increment() #2075

Uh oh!

Conversation

schmidt-sebastian commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

var-const 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!

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!

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!

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!

wilhuff Nov 16, 2018 • 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!

var-const Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

schmidt-sebastian commented Nov 13, 2018 •

edited

Loading

wilhuff Nov 16, 2018 •

edited

Loading

var-const Nov 16, 2018 •

edited

Loading

var-const Nov 16, 2018 •

edited

Loading