WIP: OR Queries #3185

ehsannas · 2021-11-29T17:23:35Z

No description provided.

Add Filter.Java. Remove core/Filter.Java. Add CompositeFilter.java. Make the Filter class abstract. Purely formatting change. (ran googleJavaFormat). Added UnqualifiedFieldFilter, and `where` API. Update core/Query.java to be "aware" of composite filters. Fix the logic for `matches(doc)` for CompositeFilters. Fix remaining usages. Update RemoteSerializer.java. Add TODO for bundles. Add the other filter APIs. integrate with indexing. Respect the limit when evaluating multiple dnf terms. Remove UnqualifiedFieldFilter. Move validation to the right place. Don't impose limit on sub-filter queries. Enforce limit constraint for queries with more than 1 DNF term. Add the parsed filter to the query.

ehsannas · 2021-11-29T18:02:21Z

rebased.

google-oss-bot · 2021-11-29T18:08:56Z

The public api surface has changed for the subproject firebase-firestore:
error: Added method com.google.firebase.firestore.DocumentReference.getKey() [AddedMethod]
error: Added class com.google.firebase.firestore.Filter [AddedClass]
error: Added method com.google.firebase.firestore.FirebaseFirestore.getDatabaseId() [AddedMethod]
error: Added method com.google.firebase.firestore.FirebaseFirestore.getUserDataReader() [AddedMethod]
error: Added method com.google.firebase.firestore.Query.where(com.google.firebase.firestore.Filter...) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-11-29T18:13:06Z

Coverage Report

Affected SDKs

No changes between base commit (110b867) and head commit (888a2d5e).

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 (888a2d5e) is created by Prow via merging commits: 110b867 a445976.

google-oss-bot · 2021-11-29T18:22:52Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (110b867) Head (888a2d5e) Diff

aar 1.23 MB 1.24 MB +9.83 kB (+0.8%)

apk (release) 3.32 MB 3.33 MB +4.12 kB (+0.1%)

Test Logs

Notes

Head commit (888a2d5e) is created by Prow via merging commits: 110b867 a445976.

… methods public.

google-oss-bot · 2021-11-29T22:26:50Z

The public api surface has changed for the subproject firebase-firestore:
error: Added class com.google.firebase.firestore.Filter [AddedClass]
error: Added method com.google.firebase.firestore.Query.where(com.google.firebase.firestore.Filter...) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-11-29T22:42:04Z

The public api surface has changed for the subproject firebase-firestore:
error: Added class com.google.firebase.firestore.Filter [AddedClass]
error: Added method com.google.firebase.firestore.Query.where(com.google.firebase.firestore.Filter...) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-12-06T16:56:56Z

The public api surface has changed for the subproject firebase-firestore:
warning: Added method com.google.firebase.firestore.Filter.equals(Object) [AddedAbstractMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-12-06T22:32:10Z

The public api surface has changed for the subproject firebase-firestore:
warning: Added method com.google.firebase.firestore.Filter.equals(Object) [AddedAbstractMethod]
warning: Added method com.google.firebase.firestore.Filter.hashCode() [AddedAbstractMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-12-06T22:32:27Z

The public api surface has changed for the subproject firebase-firestore:
warning: Added method com.google.firebase.firestore.Filter.equals(Object) [AddedAbstractMethod]
warning: Added method com.google.firebase.firestore.Filter.hashCode() [AddedAbstractMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

ehsannas · 2021-12-07T16:16:12Z

Task :firebase-app-distribution:apiInformation FAILED... Looks like tot is broken

ehsannas · 2021-12-07T16:30:16Z

posted a fix via #3213

ehsannas · 2021-12-07T19:55:46Z

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

+  }
+
+  @Override
+  public String getCanonicalId() {


Sebastian commented that I should updated this function to be consistent with existing canonical IDs. I haven't done that yet.

google-oss-bot · 2021-12-08T20:22:07Z

The public api surface has changed for the subproject firebase-firestore:
warning: Added method com.google.firebase.firestore.Filter.equals(Object) [AddedAbstractMethod]
warning: Added method com.google.firebase.firestore.Filter.hashCode() [AddedAbstractMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-12-08T20:45:53Z

@ehsannas: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
api-information	`a445976`	link	`/test api-information`
check-changed	`a445976`	link	`/test check-changed`
device-check-changed	`a445976`	link	`/test device-check-changed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

schmidt-sebastian

Some high level comments sprinkled with random nits. Overall, the functionality is in a good place. It probably makes sense to split this up into multiple PRs soon so that we can review smaller units in greater detail and get them merged.

schmidt-sebastian · 2021-12-08T18:15:13Z

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

+  }
+
+  @NonNull
+  public static Filter inList(@NonNull String field, @Nullable Object value) {


I left a comment regarding the naming of this method in the API proposal. This should probably be inArray.

Acknowledged

schmidt-sebastian · 2021-12-08T20:09:06Z

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

+  public Query where(@NonNull Filter... filters) {
+    // If no filters were provided, we can return the current query.
+    // TODO (ehsann): Maybe we shouldn't accept zero filters.
+    if (filters.length == 0) {


Allowing zero filters makes it easier to programatically add filters.

Acknowledged

schmidt-sebastian · 2021-12-08T20:11:49Z

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

  }

  /**
   * Parses the given documentIdValue into a ReferenceValue, throwing appropriate errors if the
   * value is anything other than a DocumentReference or String, or if the string is malformed.
   */
-  private Value parseDocumentIdValue(Object documentIdValue) {
+  private Value parseDocumentIdValue(


This method should be static if you end up passing in all instance members. It however looks like you are just passing in this.firestore and this.query so I think you can probably revert this change altogether.

yeah, this should be reverted. I moved this code to Filter and then moved it back here, and forgot to change the signature back to its original form.

schmidt-sebastian · 2021-12-08T20:16:17Z

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

+   *
+   * @return A new field filter that contains the parsed Value.
+   */
+  private FieldFilter parseFieldFilterValue(FieldFilter filter) {


Can we refactor this so that FieldFilters always have Values? Maybe we can have com.google.firebase.fiterstore.Filter's that use Objects and FieldFilters that use Value.

I'll try this suggestion.

@schmidt-sebastian I think this wouldn't be a good idea because FieldFilter and CompositeFilter must inherit from Filter. If we add field and operator and value (as Object) in com.google.firebase.fiterstore.Filter, then FieldFilter and CompositeFilter will have those members too.

In this PR:

Filter Class (no members) ^ ^ | | | FieldFilter Class. Members: | Field | Op | value (Value) | valueObject (Object) | CompositeFilter Class. Members: Filter[] logicType

If we keep Object in the API layer:

Filter Class. Members: Field, Op, valueObject (Object) ^ ^ | | | FieldFilter Class. Members: | Field(by inheritance) | Op(by inheritance) | valueObject (Object) (by inheritance) | value (Value) | CompositeFilter Class. Members: Field(by inheritance) Op(by inheritance) valueObject (Object) (by inheritance) Filter[] logicType

Another alternative is to create a third Filter class (E.g. com.google.firebase.fiterstore.core.Filter)

Filter Class (no members) ^ ^ ^ | | | | | FieldFilter Class. Members: | | Field | | Op | | value (Value) | | | | | CompositeFilter Class. Members: | Filter[] | logicType | com.google.firebase.fiterstore.core.Filter Class. Members: Field Op value (Object)

This is what I did initially (and sadly chose UnqualifiedFilter as the name instead of core.Filter).

What do you think? Did I misunderstand your suggestion?

I think the Web API has a good precedent for this:

The API layer only knows about the Object: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/lite-api/query.ts#L132
This then gets translated to Value when we construct the internal Filter.

There is some duplication here (since you need to keep the filter operator) but the two classes are distinct from one another.

My main goal is to get the Object out of our core layer and into the API layer. If you add an hierarchy here that shares some members you might not achieve this goal.

schmidt-sebastian · 2021-12-08T20:17:38Z

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

@@ -652,7 +610,7 @@ private Query orderBy(
          "Invalid query. You must not call Query.endAt() or Query.endBefore() before "
              + "calling Query.orderBy().");
    }
-    validateOrderByField(fieldPath);


Is this intentional?

yes, this check is moved to core/Query.java. Generally there was a bunch of validation checks that really didn't belong to the API layer and should have been done in core/Query.java I believe. The addition of new filters made it more necessary to move them. I'll carve out the code moves as separate PRs that we can easily review.

schmidt-sebastian · 2021-12-08T21:27:41Z

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

      if (filter instanceof FieldFilter) {
        FieldFilter fieldfilter = (FieldFilter) filter;
        if (fieldfilter.isInequality()) {
          return fieldfilter.getField();
        }
+      } else if (filter instanceof CompositeFilter) {
+        CompositeFilter compositeFilter = (CompositeFilter) filter;
+        FieldFilter found = compositeFilter.getInequalityFilter();
+        if (found != null) {
+          return found.getField();
+        }


Can there be a method on Filter that we can call?

Acknowledged.

schmidt-sebastian · 2021-12-08T21:28:51Z

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

+            compositeFilter.firstFieldFilterWhere(f -> operators.contains(f.getOperator()));
+        if (fieldFilter != null) {
+          return fieldFilter.getOperator();
+        }


Same comment as a above - would we nice if we did not have to differentiate the different filter types.

Acknowledged.

schmidt-sebastian · 2021-12-08T21:30:22Z

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

+  // DNF (Disjunctive Normal Form) representation of this target's filters.
+  // Each element in this list is an AND filter of FieldFilters.
+  // The query result is the disjunction (OR) of each of these AND filters.
+  private final List<CompositeFilter> dnf;


The Target is essentially the proto representation. We should not store the DNF here.

I see. Thinking about this more, I think we should be able to push it to SQLiteIndexManager.java as that's the only place where it's really needed.

schmidt-sebastian · 2021-12-08T21:41:45Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java

   */
  @Nullable
-  FieldIndex getFieldIndex(Target target);
+  FieldIndex getFieldIndex(Target target, @Nullable CompositeFilter andFilter);


The reason this method exists is just so that the QueryEngine can quickly short-circuit Index-based execution if no index is defined. The callsite is:

FieldIndex i = getFIeldIndex(); if (i != null) {...}

It seems like this now adds some complication as we need to pass more state between getFieldIndex and getDocumentsMatchingTarget. We should probably revisit how this works to minimize these interactions (i.e. add a hasMatchingIndex() method or make getDocumentsMatchingTarget() nullable).

Yes, you got it exactly right. I think I know how to simplify it now.

schmidt-sebastian · 2021-12-08T21:42:38Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java

+      return result;
+    }
+
+    // If there's a limit constraint, we should perform all branches of the query (as done above),


The limit handling should be pushed into SQLite.

Acknowledged.

ehsannas

Thanks a lot for going through this PR. The PR is larger than I originally wanted it to be. For the most part I agree with your comments. I will start slicing this PR into smaller, more contained changes that can be reviewed/landed more easily. Coming soon to GitHub....

Thanks!

ehsannas · 2021-12-09T21:16:12Z

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

+  }
+
+  @NonNull
+  public static Filter inList(@NonNull String field, @Nullable Object value) {


Acknowledged

ehsannas · 2021-12-09T21:16:21Z

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

@@ -388,6 +389,26 @@ public Query whereNotIn(@NonNull FieldPath fieldPath, @NonNull List<? extends Ob
    return whereHelper(fieldPath, Operator.NOT_IN, values);


yeah. Acknowledged.

ehsannas · 2021-12-09T21:16:39Z

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

+  public Query where(@NonNull Filter... filters) {
+    // If no filters were provided, we can return the current query.
+    // TODO (ehsann): Maybe we shouldn't accept zero filters.
+    if (filters.length == 0) {


Acknowledged

ehsannas · 2021-12-09T21:17:24Z

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

  }

  /**
   * Parses the given documentIdValue into a ReferenceValue, throwing appropriate errors if the
   * value is anything other than a DocumentReference or String, or if the string is malformed.
   */
-  private Value parseDocumentIdValue(Object documentIdValue) {
+  private Value parseDocumentIdValue(


yeah, this should be reverted. I moved this code to Filter and then moved it back here, and forgot to change the signature back to its original form.

ehsannas · 2021-12-09T21:18:15Z

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

+   *
+   * @return A new field filter that contains the parsed Value.
+   */
+  private FieldFilter parseFieldFilterValue(FieldFilter filter) {


I'll try this suggestion.

ehsannas · 2021-12-09T21:30:18Z

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

      if (filter instanceof FieldFilter) {
        FieldFilter fieldfilter = (FieldFilter) filter;
        if (fieldfilter.isInequality()) {
          return fieldfilter.getField();
        }
+      } else if (filter instanceof CompositeFilter) {
+        CompositeFilter compositeFilter = (CompositeFilter) filter;
+        FieldFilter found = compositeFilter.getInequalityFilter();
+        if (found != null) {
+          return found.getField();
+        }


Acknowledged.

ehsannas · 2021-12-09T21:30:22Z

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

+            compositeFilter.firstFieldFilterWhere(f -> operators.contains(f.getOperator()));
+        if (fieldFilter != null) {
+          return fieldFilter.getOperator();
+        }


Acknowledged.

ehsannas · 2021-12-09T21:32:45Z

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

+  // DNF (Disjunctive Normal Form) representation of this target's filters.
+  // Each element in this list is an AND filter of FieldFilters.
+  // The query result is the disjunction (OR) of each of these AND filters.
+  private final List<CompositeFilter> dnf;


I see. Thinking about this more, I think we should be able to push it to SQLiteIndexManager.java as that's the only place where it's really needed.

ehsannas · 2021-12-09T21:34:16Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexManager.java

   */
  @Nullable
-  FieldIndex getFieldIndex(Target target);
+  FieldIndex getFieldIndex(Target target, @Nullable CompositeFilter andFilter);


Yes, you got it exactly right. I think I know how to simplify it now.

ehsannas · 2021-12-09T21:34:23Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java

+      return result;
+    }
+
+    // If there's a limit constraint, we should perform all branches of the query (as done above),


Acknowledged.

google-oss-bot added the do-not-merge/work-in-progress label Nov 29, 2021

google-cla bot added the cla: yes Override cla label Nov 29, 2021

ehsannas marked this pull request as draft November 29, 2021 17:23

google-oss-bot added the size/XXL label Nov 29, 2021

ehsannas force-pushed the ehsann/or-queries branch from a780ffd to a31fa87 Compare November 29, 2021 18:02

ehsannas added 3 commits November 29, 2021 14:09

Move the parsing back to firestore/Query.java to avoid having to make…

005d125

… methods public.

cleanup a bit.

79958d0

Use StructuredQuery.CompositeFilter.Operator.

5dbdc4a

Add copyright info.

45e6b22

ehsannas added 4 commits November 30, 2021 14:16

Add RestrictTo and Update api.txt.

2992489

Move the assertion back to where it was before.

6b45e9c

Add the code for validating addition of a CompositeFilter to a query.

8aed557

DNF transform.

a0f67d6

ehsannas added 2 commits December 6, 2021 16:24

Classes that override equals should also override hashCode.

0c8242f

fix formatting.

cfc31c2

ehsannas requested review from schmidt-sebastian, wu-hui and thebrianchen December 7, 2021 18:56

ehsannas commented Dec 7, 2021

View reviewed changes

Add QueryTest unit test for or queries.

a445976

schmidt-sebastian reviewed Dec 8, 2021

View reviewed changes

ehsannas commented Dec 9, 2021

View reviewed changes

ehsannas closed this Aug 2, 2022

firebase locked and limited conversation to collaborators Sep 2, 2022

		@@ -388,6 +389,26 @@ public Query whereNotIn(@NonNull FieldPath fieldPath, @NonNull List<? extends Ob
		return whereHelper(fieldPath, Operator.NOT_IN, values);

WIP: OR Queries #3185

WIP: OR Queries #3185

Uh oh!

Conversation

ehsannas commented Nov 29, 2021

Uh oh!

ehsannas commented Nov 29, 2021

Uh oh!

google-oss-bot commented Nov 29, 2021

Uh oh!

google-oss-bot commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

Test Logs

Notes

Uh oh!

google-oss-bot commented Nov 29, 2021 • 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 Nov 29, 2021

Uh oh!

google-oss-bot commented Nov 29, 2021

Uh oh!

google-oss-bot commented Dec 6, 2021

Uh oh!

google-oss-bot commented Dec 6, 2021

Uh oh!

google-oss-bot commented Dec 6, 2021

Uh oh!

ehsannas commented Dec 7, 2021

Uh oh!

ehsannas commented Dec 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Dec 8, 2021

Uh oh!

google-oss-bot commented Dec 8, 2021

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!

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!

google-oss-bot commented Nov 29, 2021 •

edited

Loading

google-oss-bot commented Nov 29, 2021 •

edited

Loading

`firebase-firestore`