Add != and NOT_IN queries (not public) #1872

thebrianchen · 2020-08-10T22:04:43Z

Porting from web.

google-oss-bot · 2020-08-10T22:11:49Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (ed8977b) Head (6324327b) Diff

aar 996 kB 999 kB +2.95 kB (+0.3%)

apk (release) 3.13 MB 3.13 MB +1.00 kB (+0.0%)

Test Logs

Notes

Head commit (6324327b) is created by Prow via merging commits: ed8977b cc4397b.

google-oss-bot · 2020-08-10T22:14:32Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 46.74% (ed8977b) to 46.79% (6324327b) by +0.05%.

Filename	Base (`ed8977b`)	Head (6324327b)	Diff
AsyncQueue.java	77.89%	76.88%	-1.01%
FieldFilter.java	88.68%	88.33%	-0.35%
KeyFieldNotInFilter.java	?	0.00%	?
NotInFilter.java	?	100.00%	?
Query.java	4.03%	3.96%	-0.07%
RemoteSerializer.java	83.17%	83.68%	+0.51%
StructuredQuery.java	33.08%	33.38%	+0.31%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (6324327b) is created by Prow via merging commits: ed8977b cc4397b.

oops

wilhuff · 2020-08-12T20:57:57Z

firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

+   * @param <T> The type of the values of in common between the expected list and actual set.
+   */
+  // PORTING NOTE: JUnit and XCTest use reversed conventions on expected and actual values :-(.
+  public static <T> void assertArrayEquals(List<T> expected, List<T> actual) {


This name doesn't seem right. Neither argument is an array and the ordered-container property of arrays is explicitly ignored in the implementation.

assertSetEquals above are essentially asserting the two containers are equal without regard for ordering. I think you probably could use the same name here, since that's what you're doing.

However, I think you can combine this with the one above. Make it:

public static <T> void assertSetEquals(Iterable<T> expected, Iterable<T> actual) { Set<T> expectedSet = Sets.newHashSet(expected); Set<T> actualSet = Sets.newHashSet(actual); assertEquals(expectedSet, actualSet); }

This has the added benefit of allowing Collection<T> to be passed as an argument directly, which will eliminate all the Lists.newArrayList in your callers.

Done. Thanks for the explanation!

wilhuff · 2020-08-12T20:58:40Z

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

+
+    QuerySnapshot snapshot = waitFor(collection.whereNotEqualTo("zip", 98101L).get());
+    assertArrayEquals(
+        Lists.newArrayList(expectedDocsMap.values()), querySnapshotToValues(snapshot));


As noted on assertArrayEquals, remove this Lists.newArrayList, here and throughout.

Removed here and throughout.

wilhuff · 2020-08-12T22:12:27Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

+  /**
+   * Given an operator, returns the set of operators that cannot be used with it.
+   *
+   * <p>Operators in a query must adhere to the following set of rules: 1. Only one array operator


To make a list use

<ol> <li>Only one array operator ... <li>One one disjunctive ... </ol>

The list wrapper element is required (<ol> indicates an ordered list (aka numbered); use <ul> for unordered (aka bulleted)).

Done for reals.

wilhuff · 2020-08-12T22:24:42Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java

      } else {
        hardAssert(
            operator != Operator.ARRAY_CONTAINS && operator != Operator.ARRAY_CONTAINS_ANY,
            operator.toString() + "queries don't make sense on document keys");
        return new KeyFieldFilter(path, operator, value);
      }
    } else if (Values.isNullValue(value)) {
-      if (operator != Filter.Operator.EQUAL) {
+      // TODO(ne-queries): Update error message to include != operator.
+      if (operator != Filter.Operator.EQUAL && operator != Filter.Operator.NOT_EQUAL) {


Operator must already be imported here (see references to Operator.ARRAY_CONTAINS) so you can drop the Filter in Filter.Operator.

wilhuff · 2020-08-12T22:27:03Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/KeyFieldNotInFilter.java

@@ -0,0 +1,39 @@
+// Copyright 2019 Google LLC


wilhuff · 2020-08-12T22:30:20Z

firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java

@@ -234,4 +240,22 @@ public static int compareByteStrings(ByteString left, ByteString right) {
    }
    return Util.compareIntegers(left.size(), right.size());
  }
+
+  public static List<DocumentKey> extractDocumentKeysFromArrayValue(


This isn't likely to be a widely used or general utility. Better to make this a package private static member of e.g. KeyFieldInFilter and just call it from the other one.

(Another clue that this doesn't belong here is that you're referring to KeyFieldInFilter in the error message. This is therefore pretty specific to those classes.)

Done. Thanks for pointing out the code smell, that's a helpful check I'll keep in mind.

wilhuff · 2020-08-14T02:10:16Z

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

+            "bb", docD);
+    CollectionReference collection = testCollectionWithDocs(testDocs);
+    QuerySnapshot docs = waitFor(collection.whereNotEqualTo(FieldPath.documentId(), "aa").get());
+    assertEquals(asList(docB, docC, docD), querySnapshotToValues(docs));


Apologies for what's going to be an irritating question (since I didn't pick up on this earlier), but why do you assertSetEquals(x, expected) in the test above but assertEquals(asList(x), expected) here when we're testing the same underlying operator?

To me it seems like either the ordering matters or it doesn't. If ordering of query results is predictable, assertSetEquals seems wrong because it allows the result to be unordered when our customers really care that it's ordered. The reverse is also true: if ordering doesn't matter, then this test is too strict, and could create a false positive.

To my understanding, query results always have a well defined ordering (breaking ties on key ordering). This seems to point to assertSetEquals being wrong above.

What's your take on this? Why the discrepancy?

(The same concern applies to the whereNotIn, below).

Thanks for catching this actually! I originally ran into an issue where I was converting the snapshots into a HashSet in order to easily remove the unneeded documents, but converting them back into a List resulted in an ordering that didn't match the query snapshots from the query. Instead of trying to create the expectedDocsMap in the correct order, I took the "shortcut" of comparing them as sets to avoid the ordering errors without realizing that order matters in query results.

I took a look at the code, and realized that Using Maps.newHashMap() keeps the documents in the correct order. Switched back to using assertEquals with Lists.newArrayList().

wilhuff · 2020-08-14T02:15:23Z

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

+  /**
+   * Given an operator, returns the set of operators that cannot be used with it.
+   *
+   * <p>Operators in a query must adhere to the following set of rules: 1. Only one array operator


The list wrapper element is required (<ol> indicates an ordered list (aka numbered); use <ul> for unordered (aka bulleted)).

wilhuff · 2020-08-14T02:29:25Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/NotInFilter.java

@@ -0,0 +1,36 @@
+// Copyright 2019 Google LLC


2020 here too

wilhuff

LGTM

thebrianchen added the api: firestore label Aug 10, 2020

thebrianchen self-assigned this Aug 10, 2020

googlebot added the cla: yes Override cla label Aug 10, 2020

google-oss-bot added the size/XL label Aug 10, 2020

add != and NOT_IN support

c5ffda3

oops

thebrianchen force-pushed the bc/ne branch from 1e6b86d to c5ffda3 Compare August 11, 2020 16:13

thebrianchen requested a review from wilhuff August 11, 2020 16:52

thebrianchen assigned wilhuff and unassigned thebrianchen Aug 11, 2020

wilhuff reviewed Aug 12, 2020

View reviewed changes

wilhuff assigned thebrianchen and unassigned wilhuff Aug 12, 2020

resolve gil comments

025ef85

thebrianchen requested a review from wilhuff August 14, 2020 00:42

thebrianchen assigned wilhuff and unassigned thebrianchen Aug 14, 2020

wilhuff reviewed Aug 14, 2020

View reviewed changes

wilhuff assigned thebrianchen and unassigned wilhuff Aug 14, 2020

switch back to Lists.newArrayList() and remove assertSetEquals override

cc4397b

thebrianchen requested a review from wilhuff August 14, 2020 16:23

thebrianchen assigned wilhuff and unassigned thebrianchen Aug 14, 2020

wilhuff approved these changes Aug 14, 2020

View reviewed changes

wilhuff assigned thebrianchen and unassigned wilhuff Aug 14, 2020

thebrianchen merged commit 1ee3caa into master Aug 14, 2020

thebrianchen deleted the bc/ne branch August 14, 2020 17:31

thebrianchen mentioned this pull request Sep 8, 2020

Add != and notIn support firebase/firebase-ios-sdk#6418

Merged

firebase locked and limited conversation to collaborators Sep 14, 2020

Add != and NOT_IN queries (not public) #1872

Add != and NOT_IN queries (not public) #1872

Uh oh!

Conversation

thebrianchen commented Aug 10, 2020

Uh oh!

google-oss-bot commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

google-oss-bot commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-firestore

Test Logs

Notes

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!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

google-oss-bot commented Aug 10, 2020 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Aug 10, 2020 •

edited

Loading

`firebase-firestore`