Add != and notIn support #6418

thebrianchen · 2020-09-08T21:41:01Z

Porting from web and Android.

lint

wilhuff · 2020-09-10T19:16:17Z

Firestore/CHANGELOG.md

+- [feature] Added `whereField(_:notIn:)` and `whereField(_:isNotEqualTo:)` query
+operators. `whereField(_:notIn:)` finds documents where a specified field’s
+value is not in a specified array. `whereField(_:isNotEqualTo:)` finds documents
+where a specified field's value does not equal the specified value.


There's a major caveat to this feature, so it's probably worth calling this out explicitly, maybe with a trailing sentence like:

Neither query operator will match documents where the specified field is not present.

wilhuff · 2020-09-10T19:18:02Z

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

+
+  FSTAssertThrows([[coll queryWhereField:@"x" isNotEqualTo:@1] queryWhereField:@"y"
+                                                                  isNotEqualTo:@2],
+                  @"Invalid Query. All where filters with an inequality (lessThan, "


Should notEqual be in this list of inequalities?

Thanks for catching. Updated.

Firestore/core/src/api/query_core.cc

wilhuff · 2020-09-10T19:22:55Z

Firestore/core/src/api/query_core.cc

@@ -423,6 +418,34 @@ FieldValue Query::ParseExpectedReferenceValue(
  }
 }

+std::vector<core::Filter::Operator> Query::ConflictingOps(


The core::Filter::Operator seems like it could be Filter::Operator (given the argument type).

wilhuff · 2020-09-10T19:24:46Z

Firestore/core/src/api/query_core.cc

+    Filter::Operator op) const {
+  switch (op) {
+    case Filter::Operator::NotEqual:
+      return std::vector<core::Filter::Operator>{Filter::Operator::NotEqual,


I suspect you can drop the type name in the initializer here (i.e. that you can return {Filter::Operator::NotEqual, Filter::Operator::NotIn};).

wilhuff · 2020-09-10T20:17:56Z

Firestore/Source/Public/FirebaseFirestore/FIRQuery.h

+ * the specified field and the value does not equal any of the values from the provided array.
+ *
+ * A query can have only one `notIn` filter, and it cannot be combined with an `arrayContains`,
+ * `arrayContainsAny`, `in`, or `notEqual` filter.


Is it worth calling out the behavior of not-in [null] here? It's counter-intuitive because not-equal null automagically makes that work, but not-in has no equivalent.

I originally considered it, but thought it was too specific of an edge case. Added a comment to about the behavior -- should I go more into detail?

Yeah, this is tough. Here's my take, which avoids "results in no document matches" which is a fairly indirect way of saying that the filter can't match the field.

One special case is that notIn filters cannot match nil values. To query for documents where a field exists and is nil, use a notEqual filter, which can handle this special case.

Separately, note that Objective-C and Swift spell "null" as nil.

Thanks for the suggestion! Updated here and in Android with the appropriate names.

Firestore/core/src/core/key_field_not_in_filter.cc

wilhuff · 2020-09-10T20:22:21Z

Firestore/core/src/core/not_in_filter.h

+namespace firebase {
+namespace firestore {
+
+namespace model {


You can get these forward declarations by including Firestore/core/src/model/model_fwd.h.

Also, this is inconsistent with key_field_not_in_filter.h which neither makes the forward declarations nor includes the model_fwd.h header.

Using model_fwd.h in key_field_not_in_filter.h, key_field_in_filter.h, and not_in_filter.h.

I'll submit a separate PR that cleans up the unnecessary inlined forward declarations for the other filters.

wilhuff · 2020-09-10T20:25:28Z

Firestore/core/src/core/query.h

-   */
-  absl::optional<Filter::Operator> FirstDisjunctiveOperator() const;
+  absl::optional<Filter::Operator> FindOperator(
+      std::vector<Filter::Operator> ops) const;


Since FindOperator isn't storing the ops value in a field (or otherwise retaining it beyond the lifetime of the call), you should pass ops via const reference.

wilhuff · 2020-09-10T20:27:54Z

Firestore/core/test/unit/remote/serializer_test.cc

@@ -369,9 +369,10 @@ class SerializerTest : public ::testing::Test {
    create_time_proto->set_nanos(4321);
  }

-  void ExpectUnaryOperator(const FieldValue& value,
+  void ExpectUnaryOperator(std::string opStr,


In C++ variables should be snake_case.

Firestore/Source/Public/FirebaseFirestore/FIRQuery.h

wilhuff

Very close to LGTM.

wilhuff · 2020-09-14T22:26:46Z

Firestore/Source/Public/FirebaseFirestore/FIRQuery.h

+ * the specified field and the value does not equal any of the values from the provided array.
+ *
+ * A query can have only one `notIn` filter, and it cannot be combined with an `arrayContains`,
+ * `arrayContainsAny`, `in`, or `notEqual` filter.


Yeah, this is tough. Here's my take, which avoids "results in no document matches" which is a fairly indirect way of saying that the filter can't match the field.

One special case is that notIn filters cannot match nil values. To query for documents where a field exists and is nil, use a notEqual filter, which can handle this special case.

Separately, note that Objective-C and Swift spell "null" as nil.

wilhuff · 2020-09-14T22:27:39Z

Firestore/core/src/api/query_core.cc

@@ -333,7 +360,8 @@ void Query::ValidateOrderByField(const FieldPath& order_by_field,
  if (order_by_field != inequality_field) {
    ThrowInvalidArgument(
        "Invalid query. You have a where filter with an inequality "
-        "(lessThan, lessThanOrEqual, greaterThan, or greaterThanOrEqual) on "
+        "(notEqual, lessThan, lessThanOrEqual, greaterThan, or "


nit: reflow?

wilhuff · 2020-09-14T22:28:11Z

Firestore/core/src/api/query_core.h

@@ -20,6 +20,7 @@
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>


No longer needed?

wilhuff

LGTM

thebrianchen added the api: firestore label Sep 8, 2020

thebrianchen self-assigned this Sep 8, 2020

googlebot added the cla: yes label Sep 8, 2020

google-oss-bot added the public-api-change label Sep 8, 2020

Add != and notIn support

9fecaa6

thebrianchen force-pushed the bc/ne-queries branch from d470581 to 9fecaa6 Compare September 9, 2020 19:09

# This is a combination of 3 commits.

609bda9

lint

thebrianchen force-pushed the bc/ne-queries branch from f4e74ba to 609bda9 Compare September 9, 2020 20:13

thebrianchen requested a review from wilhuff September 9, 2020 20:25

thebrianchen assigned wilhuff and thebrianchen and unassigned thebrianchen and wilhuff Sep 9, 2020

fix validation test

064a865

thebrianchen assigned wilhuff and unassigned thebrianchen Sep 10, 2020

wilhuff suggested changes Sep 10, 2020

View reviewed changes

wilhuff reviewed Sep 10, 2020

View reviewed changes

Firestore/Source/Public/FirebaseFirestore/FIRQuery.h Show resolved Hide resolved

wilhuff assigned thebrianchen and unassigned wilhuff Sep 10, 2020

resolve gil comments

477edd1

thebrianchen requested a review from wilhuff September 11, 2020 00:22

thebrianchen assigned wilhuff and unassigned thebrianchen Sep 11, 2020

Add NOT_IN to inequality list

0aeb0f9

wilhuff reviewed Sep 14, 2020

View reviewed changes

wilhuff assigned thebrianchen and unassigned wilhuff Sep 14, 2020

address comments rd. 2

2811b10

thebrianchen requested a review from wilhuff September 15, 2020 16:39

thebrianchen assigned wilhuff and unassigned thebrianchen Sep 15, 2020

wilhuff approved these changes Sep 15, 2020

View reviewed changes

thebrianchen merged commit 21921e9 into master Sep 15, 2020

thebrianchen deleted the bc/ne-queries branch September 15, 2020 21:12

firebase locked and limited conversation to collaborators Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add != and notIn support #6418

Add != and notIn support #6418

thebrianchen commented Sep 8, 2020

wilhuff Sep 10, 2020

thebrianchen Sep 10, 2020

wilhuff Sep 10, 2020

thebrianchen Sep 10, 2020

wilhuff Sep 10, 2020

thebrianchen Sep 10, 2020

wilhuff Sep 10, 2020

thebrianchen Sep 10, 2020

wilhuff Sep 10, 2020

thebrianchen Sep 10, 2020 •

edited

Loading

wilhuff Sep 14, 2020

thebrianchen Sep 15, 2020

wilhuff Sep 10, 2020

thebrianchen Sep 10, 2020

thebrianchen Sep 10, 2020

wilhuff Sep 10, 2020

thebrianchen Sep 10, 2020

wilhuff Sep 10, 2020

thebrianchen Sep 10, 2020

wilhuff left a comment

wilhuff Sep 14, 2020

wilhuff Sep 14, 2020

thebrianchen Sep 15, 2020

wilhuff Sep 14, 2020

thebrianchen Sep 15, 2020

wilhuff left a comment

Add != and notIn support #6418

Add != and notIn support #6418

Conversation

thebrianchen commented Sep 8, 2020

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

thebrianchen Sep 10, 2020 • edited Loading

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

wilhuff 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

wilhuff left a comment

Choose a reason for hiding this comment

thebrianchen Sep 10, 2020 •

edited

Loading