ValueUtil changes for FieldValue migration #7991

schmidt-sebastian · 2021-04-28T02:52:32Z

This is part of #7904. It will break the feature branch, but I am planning to send out small reviewable chunks with an end goal of a feature branch that passes CI.

This PR adds a couple of new utilities that are used in the FieldValue port (and changes some signatures). They are all based on Android: https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.java

The PR also adds logic to treat zero-length strings/bytes the same as null bytes and strings, as nanopb uses both representations.

google-oss-bot · 2021-04-28T02:53:24Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

schmidt-sebastian · 2021-05-07T15:23:03Z

@wu-hui Do you have time to take a look at this?

wu-hui

Some minor changes requested, almost LGTM.

wu-hui · 2021-05-14T14:47:51Z

Firestore/core/src/model/value_util.cc

-    // An empty blob is represented by a nullptr
-    return util::Compare(left.bytes_value != nullptr,
-                         right.bytes_value != nullptr);
+    // An empty blob is represented by a nullptr (or an empty byte array)


Question: this seems to be a behavior change from below? Can you explain why? Not that I am against..

This is not meant to be a behavior change but rather a bug fix. When we create an empty bytes array directly with NanoPB, it uses nullptr and does not actually allocate memory. When we receive documents from the backend, these empty byte arrays are represented using a zero-length array, but one with an actual address in memory. We need these two representations to yield the same comparison behavior so our users don't see any behavior change.

Sounds good, thanks.

wu-hui · 2021-05-14T15:11:14Z

Firestore/core/src/model/value_util.cc

+
+bool IsNaNValue(const google_firestore_v1_Value& value) {
+  return value.which_value_type == google_firestore_v1_Value_double_value_tag &&
+         isnan(value.double_value);


Should be std::isnan from cmath instead.

isnan is also available because cmath also include math.h, but should not be directly used. Just learnt this today from Costa's code review :-)

Very nice. Hopefully I can use this information in my next code review.

wu-hui · 2021-05-14T15:15:42Z

Firestore/core/src/model/value_util.h

 /**
 * Generate the canonical ID for the provided field value (as used in Target
 * serialization).
 */
 std::string CanonicalId(const google_firestore_v1_Value& value);

+/**
+ * Generate the canonical ID for the provided array value (as used in Target


"Generates"

wu-hui · 2021-05-14T15:17:27Z

Firestore/core/src/model/value_util.h

+bool Contains(google_firestore_v1_ArrayValue haystack,
+              google_firestore_v1_Value needle);
+
+/** Returns `nullptr` in its Protobuf representation. */


Maybe Returns a null Protobuf value? nullptr is pretty specifically a pointer to address 0.

wu-hui · 2021-05-14T15:18:23Z

Firestore/core/src/model/value_util.h

+/** Returns `nullptr` in its Protobuf representation. */
+google_firestore_v1_Value NullValue();
+
+/** Returns `true` if `value` is `nullptr` in its Protobuf representation. */


...if given value is a null Protobuf value?

wu-hui · 2021-05-14T15:18:48Z

Firestore/core/src/model/value_util.h

+/** Returns `true` if `value` is `NaN` in its Protobuf representation. */
+bool IsNaNValue(const google_firestore_v1_Value& value);
+
+google_firestore_v1_Value RefValue(const DatabaseId& database_id,


Please add comment.

schmidt-sebastian

Thanks for the review!

schmidt-sebastian · 2021-05-14T15:38:28Z

Firestore/core/src/model/value_util.cc

-    // An empty blob is represented by a nullptr
-    return util::Compare(left.bytes_value != nullptr,
-                         right.bytes_value != nullptr);
+    // An empty blob is represented by a nullptr (or an empty byte array)


This is not meant to be a behavior change but rather a bug fix. When we create an empty bytes array directly with NanoPB, it uses nullptr and does not actually allocate memory. When we receive documents from the backend, these empty byte arrays are represented using a zero-length array, but one with an actual address in memory. We need these two representations to yield the same comparison behavior so our users don't see any behavior change.

schmidt-sebastian · 2021-05-14T15:39:32Z

Firestore/core/src/model/value_util.cc

+
+bool IsNaNValue(const google_firestore_v1_Value& value) {
+  return value.which_value_type == google_firestore_v1_Value_double_value_tag &&
+         isnan(value.double_value);


Very nice. Hopefully I can use this information in my next code review.

schmidt-sebastian · 2021-05-14T15:40:30Z

Firestore/core/src/model/value_util.h

+bool Contains(google_firestore_v1_ArrayValue haystack,
+              google_firestore_v1_Value needle);
+
+/** Returns `nullptr` in its Protobuf representation. */


schmidt-sebastian · 2021-05-14T15:40:35Z

Firestore/core/src/model/value_util.h

+/** Returns `nullptr` in its Protobuf representation. */
+google_firestore_v1_Value NullValue();
+
+/** Returns `true` if `value` is `nullptr` in its Protobuf representation. */


schmidt-sebastian · 2021-05-14T15:41:21Z

Firestore/core/src/model/value_util.h

+/** Returns `true` if `value` is `NaN` in its Protobuf representation. */
+bool IsNaNValue(const google_firestore_v1_Value& value);
+
+google_firestore_v1_Value RefValue(const DatabaseId& database_id,


wu-hui

There is one place you missed to update a comment. But LGTM still!

wu-hui · 2021-05-14T21:05:20Z

Firestore/core/src/model/value_util.cc

-    // An empty blob is represented by a nullptr
-    return util::Compare(left.bytes_value != nullptr,
-                         right.bytes_value != nullptr);
+    // An empty blob is represented by a nullptr (or an empty byte array)


Sounds good, thanks.

schmidt-sebastian changed the base branch from master to mrschmidt/mutabledocuments April 28, 2021 02:52

schmidt-sebastian changed the title ~~Mrschmidt/valueutil~~ This PR adds a couple of new classes and methods that are used in the FieldValue port. Apr 28, 2021

google-cla bot added the cla: yes label Apr 28, 2021

google-oss-bot added the api: firestore label Apr 28, 2021

ValueUtil changes for FieldValue migration

cc4d0f1

schmidt-sebastian force-pushed the mrschmidt/valueutil branch from 93af3ba to cc4d0f1 Compare April 28, 2021 02:59

schmidt-sebastian changed the title ~~This PR adds a couple of new classes and methods that are used in the FieldValue port.~~ @schmidt-sebastian ValueUtil changes for FieldValue migration Apr 28, 2021

schmidt-sebastian changed the title ~~@schmidt-sebastian ValueUtil changes for FieldValue migration~~ ValueUtil changes for FieldValue migration Apr 28, 2021

schmidt-sebastian requested a review from wu-hui April 28, 2021 03:02

schmidt-sebastian assigned wu-hui Apr 28, 2021

schmidt-sebastian mentioned this pull request Apr 30, 2021

FieldValue changes for transform_operation #8017

Merged

wu-hui requested changes May 14, 2021

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui May 14, 2021

Review

3441ee3

schmidt-sebastian commented May 14, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian May 14, 2021

wu-hui approved these changes May 14, 2021

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui May 14, 2021

Update value_util.h

be5467a

schmidt-sebastian merged commit e614d95 into mrschmidt/mutabledocuments May 14, 2021

schmidt-sebastian deleted the mrschmidt/valueutil branch May 14, 2021 22:31

firebase locked and limited conversation to collaborators Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ValueUtil changes for FieldValue migration #7991

ValueUtil changes for FieldValue migration #7991

schmidt-sebastian commented Apr 28, 2021 •

edited

Loading

google-oss-bot commented Apr 28, 2021

schmidt-sebastian commented May 7, 2021

wu-hui left a comment

wu-hui May 14, 2021

schmidt-sebastian May 14, 2021

wu-hui May 14, 2021

wu-hui May 14, 2021

schmidt-sebastian May 14, 2021

wu-hui May 14, 2021

schmidt-sebastian May 14, 2021

schmidt-sebastian May 14, 2021

wu-hui May 14, 2021

schmidt-sebastian May 14, 2021

wu-hui May 14, 2021

schmidt-sebastian May 14, 2021

wu-hui May 14, 2021

schmidt-sebastian May 14, 2021

schmidt-sebastian left a comment

schmidt-sebastian May 14, 2021

schmidt-sebastian May 14, 2021

schmidt-sebastian May 14, 2021

schmidt-sebastian May 14, 2021

schmidt-sebastian May 14, 2021

wu-hui left a comment

wu-hui May 14, 2021

ValueUtil changes for FieldValue migration #7991

ValueUtil changes for FieldValue migration #7991

Conversation

schmidt-sebastian commented Apr 28, 2021 • edited Loading

google-oss-bot commented Apr 28, 2021

schmidt-sebastian commented May 7, 2021

wu-hui 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

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

wu-hui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Apr 28, 2021 •

edited

Loading