Initial support for CompositeFilter class #3290

ehsannas · 2022-01-05T17:20:25Z

No description provided.

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

google-oss-bot · 2022-01-05T17:31:59Z

Coverage Report ¹

Affected Products

No changes between base commit (275b3eb) and merge commit (3a4eee8).

Test Logs

Notes

Commit (3a4eee8) is created by Prow via merging PR base commit (275b3eb) and head commit (993be7a).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/Eks3g4OxAu.html

google-oss-bot · 2022-01-05T17:35:49Z

Size Report ¹

Affected Products

base
Type Base (96a4b39) Merge (40aedfd) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-annotations
Type Base (96a4b39) Merge (40aedfd) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.83 kB ? (?)
firebase-appcheck-interop
Type Base (96a4b39) Merge (40aedfd) Diff
aar ? 5.17 kB ? (?)
apk (aggressive) ? 281 kB ? (?)
apk (release) ? 924 kB ? (?)
firebase-common
Type Base (96a4b39) Merge (40aedfd) Diff
aar ? 44.3 kB ? (?)
apk (aggressive) ? 77.8 kB ? (?)
apk (release) ? 635 kB ? (?)
firebase-components
Type Base (96a4b39) Merge (40aedfd) Diff
aar ? 41.4 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 29.5 kB ? (?)
firebase-database-collection
Type Base (96a4b39) Merge (40aedfd) Diff
aar ? 33.8 kB ? (?)
apk (aggressive) ? 268 kB ? (?)
apk (release) ? 901 kB ? (?)
firebase-firestore
Type Base (96a4b39) Merge (40aedfd) Diff
aar ? 1.24 MB ? (?)
apk (aggressive) ? 443 kB ? (?)
apk (release) ? 3.33 MB ? (?)
protolite-well-known-types
Type Base (96a4b39) Merge (40aedfd) Diff
aar ? 993 kB ? (?)
apk (aggressive) ? 134 kB ? (?)
apk (release) ? 661 kB ? (?)

Test Logs

Merge (40aedfd): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/3290/binary-size/1484215667354767362

Notes

Commit (40aedfd) is created by Prow via merging PR base commit (96a4b39) and head commit (993be7a).

https://storage.googleapis.com/firebase-sdk-metric-reports/CY650TRWRL.html

schmidt-sebastian · 2022-01-10T15:36:42Z

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

+
+/** @hide */
+@RestrictTo(RestrictTo.Scope.LIBRARY)
+class FieldFilterData extends Filter {


s/FieldFilterData/Filter.UnaryFilter ?

We don't use non-public classes: https://engdoc.corp.google.com/eng/doc/devguide/java/style/index.md?cl=head#s3.4.1-one-top-level-class. You can make this an inner class instead.

Using an inner class now.

I don't want to use UnaryFilter as it could get confused with

firebase-android-sdk/firebase-firestore/src/proto/google/firestore/v1/query.proto

Lines 168 to 170 in ac3276c

// A filter with a single operand.

message UnaryFilter {

// A unary operator.

There is also already a FieldFilter class. Between two FieldFilters and two UnaryFilters, I would pick two UnaryFilters as the naming UnaryFilter vs CompositeFilter offers clear and obvious distinctions.

Pinging this once more as I find the current naming a bit hard to follow. It is not obvious to me that a CompositeFilter is not a FieldFilter.

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

schmidt-sebastian · 2022-01-10T15:40:10Z

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

+  private final FieldFilter.Operator operator;
+  private final Object value;
+
+  public FieldFilterData(@NonNull FieldPath field, FieldFilter.Operator operator, Object value) {


I am not 100% certain but it seems like we don't use @NonNull in non-public methods, so I would drop it here and below. NonNull is assumed to be the default.

While we are at it "value" is nullable though.

schmidt-sebastian · 2022-01-10T15:41:50Z

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

@@ -387,15 +388,17 @@ public Query whereNotIn(@NonNull FieldPath fieldPath, @NonNull List<? extends Ob
  }

  /**
-   * Parses the given value object and creates a new {@code FieldFilter} with the given field,
-   * operator, and value. Also performs validation on the filter before retuning it.
+   * Takes a {@code FieldFilterData} object, parses the value object and returns a new {@code


Suggested change

* Takes a {@code FieldFilterData} object, parses the value object and returns a new {@code

* Takes a {@link FieldFilterData} object, parses the value object and returns a new {@code

That way, we can click on it and it gets renamed if we use IDE refactoring.

schmidt-sebastian · 2022-01-10T15:43:00Z

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

   * @return The created {@code FieldFilter}.
   */
-  private FieldFilter parseFieldFilter(@NonNull FieldPath fieldPath, Operator op, Object value) {
+  @NonNull
+  private FieldFilter parseFieldFilterData(@NonNull FieldFilterData fieldFilterData) {


Again, I think we can drop NonNull here, even though we are a bit inconsistent with the replaced method.

s/parseFieldFilterData/parseUnaryFilter

Dropped NonNull. Don't think using UnaryFilter is a good idea.

schmidt-sebastian · 2022-01-10T16:09:38Z

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

+  public abstract List<FieldFilter> getAllFieldFilters();
+
+  /** Returns the field of the first filter that's an inequality, or null if none. */
+  public abstract FieldPath inequalityField();


Please prefix with "get" (and possibly future-proof it by making it getFirstInequalityField)

schmidt-sebastian · 2022-01-10T16:12:15Z

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

+
+  /**
+   * Performs a depth-first search to find and return the first FieldFilter in the composite filter
+   * that satisfies the condition. Returns null if none of the FieldFilters satisfy the condition.


Suggested change

* that satisfies the condition. Returns null if none of the FieldFilters satisfy the condition.

* that satisfies the condition. Returns {@code null} if none of the FieldFilters satisfy the condition.

schmidt-sebastian · 2022-01-10T16:12:43Z

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

+   * Performs a depth-first search to find and return the first FieldFilter in the composite filter
+   * that satisfies the condition. Returns null if none of the FieldFilters satisfy the condition.
+   */
+  private FieldFilter firstFieldFilterWhere(Function<FieldFilter, Boolean> condition) {


Suggested change

private FieldFilter firstFieldFilterWhere(Function<FieldFilter, Boolean> condition) {

@Nullable private FieldFilter firstFieldFilterWhere(Function<FieldFilter, Boolean> condition) {

Also, just because I can't help myself and to give you something to ignore: What about "findFirstMatchingFilter"? This method name could use a verb.

schmidt-sebastian · 2022-01-10T16:18:02Z

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

+    StringBuilder builder = new StringBuilder();
+    for (Filter filter : filters) {
+      if (builder.length() == 0) {
+        builder.append(isConjunction() ? "and(" : "or(");
+      } else {
+        builder.append(",");
+      }
+      builder.append(filter.getCanonicalId());
+    }
+    builder.append(")");


You can use TextUtils.join():

schmidt-sebastian · 2022-01-10T16:18:43Z

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

+  @Override
+  public List<FieldFilter> getAllFieldFilters() {
+    // This is already a field filter, so we return a list of size one.
+    return Arrays.asList(this);


Suggested change

return Arrays.asList(this);

return Collections.singletonList(this);

…ters.

ehsannas · 2022-01-13T17:01:18Z

/retest

schmidt-sebastian

Some more minor comments. Thanks for the update.

schmidt-sebastian · 2022-01-13T21:22:43Z

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

+
+/** @hide */
+@RestrictTo(RestrictTo.Scope.LIBRARY)
+class FieldFilterData extends Filter {


There is also already a FieldFilter class. Between two FieldFilters and two UnaryFilters, I would pick two UnaryFilters as the naming UnaryFilter vs CompositeFilter offers clear and obvious distinctions.

schmidt-sebastian · 2022-01-13T21:25:59Z

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

-  public FieldPath getField() {
-    return field;
-  }
+  static class FieldFilter extends Filter {


This is not public and thus should not need the annotation. Same below.

oops. Done.

schmidt-sebastian · 2022-01-13T21:30:50Z

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

    return filter;
  }

+  /**
+   * Takes a {@link Filter.CompositeFilter} object, parses each of its subfilters, and a new {@link


Word missing before "a new". The return type in the comment is also not correct.

schmidt-sebastian · 2022-01-13T21:31:20Z

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

+  /**
+   * Takes a {@link Filter.CompositeFilter} object, parses each of its subfilters, and a new {@link
+   * CompositeFilter} that is constructed using the parsed values. Returns null if the given
+   * Filter.CompositeFilter does not contain any subfilters. Returns a {@link FieldFilter} if the


Method no longer returns null.

schmidt-sebastian · 2022-01-13T21:32:10Z

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

-        query.filter(parseFieldFilter(filter.getField(), filter.getOperator(), filter.getValue())),
-        firestore);
+    com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter);
+    if (parsedFilter.getFlattenedFilters().isEmpty()) {


Would it be more efficient to call getFilters() here?

Yes, it would be. Good suggestion. Done.

schmidt-sebastian · 2022-01-13T21:34:10Z

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

+   * returns the first one that is, or null if none are.
+   */
+  @Nullable
+  private Operator findFilterOperator(


s/findFilterOperator/findFilterByOperarator or findFilterWithOperator?

schmidt-sebastian · 2022-01-13T21:34:47Z

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

+
+  @Override
+  public List<FieldFilter> getFlattenedFilters() {
+    List<FieldFilter> result = new ArrayList<>();


Would it make sense to memoize this result?

This is good suggestion. As of now we only invoke this once when validating the composite filter. If the next PRs add more invocations of this method, I think it's worth memoizing. I'll keep that in mind for future PRs.

Please add TODO, otherwise we will never look at this code again.

schmidt-sebastian · 2022-01-13T21:36:50Z

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

+  @Override
+  public String toString() {
+    return getCanonicalId();
+  }


We probably also need equals() and hashCode()

Also coming in next PRs.

schmidt-sebastian · 2022-01-13T21:37:19Z

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

+  public abstract List<FieldFilter> getFlattenedFilters();
+
+  /** Returns the field of the first filter that's an inequality, or null if none. */
+  public abstract FieldPath getFirstInequalityField();


Add @Nullable.

ehsannas · 2022-01-17T19:08:14Z

/retest

ehsannas · 2022-01-18T03:56:35Z

/retest

schmidt-sebastian · 2022-01-19T21:02:30Z

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

+  }
+
+  /**
+   * Returns the first inequality filter contained within this composite filter. Returns null if it


s/null/{@code null}

schmidt-sebastian · 2022-01-19T21:03:18Z

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

+
+  @Override
+  public List<FieldFilter> getFlattenedFilters() {
+    List<FieldFilter> result = new ArrayList<>();


Please add TODO, otherwise we will never look at this code again.

ehsannas · 2022-01-20T17:25:07Z

/retest

ehsannas added 2 commits January 4, 2022 14:55

Add composite filters.

dc01726

add inequalityField to Filter class.

9238952

google-oss-bot added the size/L label Jan 5, 2022

ehsannas commented Jan 5, 2022

View reviewed changes

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Show resolved Hide resolved

ehsannas commented Jan 5, 2022

View reviewed changes

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved

ehsannas requested a review from schmidt-sebastian January 5, 2022 17:24

ehsannas assigned schmidt-sebastian Jan 5, 2022

schmidt-sebastian reviewed Jan 10, 2022

View reviewed changes

schmidt-sebastian assigned ehsannas and unassigned schmidt-sebastian Jan 10, 2022

ehsannas added 2 commits January 12, 2022 14:27

Address comments.

d206710

Return empty filter rather than null from parsing empty composite fil…

b146b81

…ters.

schmidt-sebastian reviewed Jan 13, 2022

View reviewed changes

ehsannas added 2 commits January 14, 2022 13:33

Address comments.

651e953

Use UnaryFilter.

867f3b7

ehsannas assigned schmidt-sebastian and unassigned ehsannas Jan 18, 2022

schmidt-sebastian approved these changes Jan 19, 2022

View reviewed changes

schmidt-sebastian assigned ehsannas and unassigned schmidt-sebastian Jan 19, 2022

ehsannas added 2 commits January 20, 2022 10:24

address comments.

f33b042

Don't use an index for serving queries with composite filters yet.

993be7a

ehsannas merged commit 514402d into master Jan 20, 2022

ehsannas deleted the ehsann/or-queries-pr-2 branch January 20, 2022 18:09

firebase locked and limited conversation to collaborators Feb 20, 2022

	// A filter with a single operand.
	message UnaryFilter {
	// A unary operator.

	* Takes a {@code FieldFilterData} object, parses the value object and returns a new {@code
	* Takes a {@link FieldFilterData} object, parses the value object and returns a new {@code

	* that satisfies the condition. Returns null if none of the FieldFilters satisfy the condition.
	* that satisfies the condition. Returns {@code null} if none of the FieldFilters satisfy the condition.

	private FieldFilter firstFieldFilterWhere(Function<FieldFilter, Boolean> condition) {
	@Nullable private FieldFilter firstFieldFilterWhere(Function<FieldFilter, Boolean> condition) {

	return Arrays.asList(this);
	return Collections.singletonList(this);

Initial support for CompositeFilter class #3290

Initial support for CompositeFilter class #3290

Conversation

ehsannas commented Jan 5, 2022

google-oss-bot commented Jan 5, 2022 • edited Loading

Coverage Report 1

Affected Products

Test Logs

Notes

google-oss-bot commented Jan 5, 2022 • edited Loading

Size Report 1

Affected Products

base

firebase-annotations

firebase-appcheck-interop

firebase-common

firebase-components

firebase-database-collection

firebase-firestore

protolite-well-known-types

Test Logs

Notes

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

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

ehsannas commented Jan 13, 2022

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

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

ehsannas commented Jan 17, 2022

ehsannas commented Jan 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehsannas commented Jan 20, 2022

google-oss-bot commented Jan 5, 2022 •

edited

Loading

Coverage Report ¹

google-oss-bot commented Jan 5, 2022 •

edited

Loading

Size Report ¹

`base`

`firebase-annotations`

`firebase-appcheck-interop`

`firebase-common`

`firebase-components`

`firebase-database-collection`

`firebase-firestore`

`protolite-well-known-types`