Serve OR queries from the index. #3368

ehsannas · 2022-01-25T22:41:19Z

No description provided.

google-oss-bot · 2022-01-25T23:58:07Z

Coverage Report ¹

Affected Products

`firebase-firestore`

Overall coverage changed from 45.43% (41e38cb) to 45.52% (d8f2149) by +0.09%.

Filename	Base (`41e38cb`)	Merge (d8f2149)	Diff
AsyncQueue.java	77.11%	78.11%	+1.00%
Bound.java	33.93%	39.29%	+5.36%
DeleteMutation.java	95.00%	90.00%	-5.00%
IndexBackfiller.java	80.00%	76.19%	-3.81%
LogicUtils.java	?	50.00%	?
MemoryIndexManager.java	74.07%	66.67%	-7.41%
PatchMutation.java	100.00%	98.39%	-1.61%
QueryEngine.java	98.51%	98.48%	-0.02%
SetMutation.java	97.14%	94.29%	-2.86%
SQLiteIndexManager.java	99.37%	98.69%	-0.68%

Test Logs

Notes

Commit (d8f2149) is created by Prow via merging PR base commit (41e38cb) and head commit (4b7858b).
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/xZ2UpYW9Pu.html

google-oss-bot · 2022-01-25T23:59:45Z

Size Report ¹

Affected Products

firebase-firestore
Type Base (41e38cb) Merge (d8f2149) Diff
aar 1.24 MB 1.24 MB +2.05 kB (+0.2%)
apk (release) 3.33 MB 3.33 MB -924 B (-0.0%)

Test Logs

Notes

Commit (d8f2149) is created by Prow via merging PR base commit (41e38cb) and head commit (4b7858b).

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

ehsannas · 2022-01-26T02:45:20Z

/retest

ehsannas · 2022-01-26T20:20:47Z

the idea is that a DNF transform on a composite filter will result in a condition that looks like

condition1 || condition2 || ...

where each condition is an AND operation of 1 or more FieldFilters.

We can then run sub-queries for condition1, condition2, etc, and find the UNION of them. In order to simply use the existing code such as TargetIndexMatcher, getArrayValues, getNotInValues, getLowerBound, getUpperBound, etc, we create a new Target object (referred to as "sub-target" in the code) for each sub-query. Each "sub-target" contains only an AND operation of 1 or more FieldFilters (similar to good old queries).

schmidt-sebastian · 2022-01-26T23:34:26Z

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

+   * Iterates over all field indexes that are used to serve the given target, and returns the least
+   * recent offset of them all. Asserts that the target can be served from index.
+   */
+  FieldIndex.IndexOffset getLeastRecentIndexOffset(Target target);


minOffset? You could alternatively also drop the LeastRecent/Min name altogether and just call this getOffset(Target target)

I think it'd be more clear to keep a wording that signals to the caller that this method will do a min operation of a bunch of offsets, rather than just a getter method that retrieves one offset. I'll go with minOffset.

schmidt-sebastian · 2022-01-26T23:34:56Z

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

+  FieldIndex.IndexOffset getLeastRecentIndexOffset(Target target);
+
+  /** Returns the lowest offset for the provided index group. */
+  FieldIndex.IndexOffset getLeastRecentIndexOffset(Collection<FieldIndex> fieldIndexes);


Can we just pass the collection group? The index manager has all indices in memory.

Also we should probably add a static import for IndexOffset or make IndexOffset a high level class.

schmidt-sebastian · 2022-01-26T23:36:08Z

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

@@ -74,6 +74,18 @@
  /** Returns all configured field indexes. */
  Collection<FieldIndex> getFieldIndexes();

+  /** Returns whether we can serve the given target from the index. */


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

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

schmidt-sebastian · 2022-01-27T00:41:24Z

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

 import androidx.annotation.Nullable;
+import com.google.common.collect.ObjectArrays;


We have a goal to kick our Guava dependency, as the Android build doesn't deal well with version conflicts for popular libraries. Hence, we should not directly use Guava and we eventually want to remove it altogether (it is used by GRPC).

schmidt-sebastian · 2022-01-27T00:43:53Z

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

+            subQueries.get(0)
+            :
+            // Construct "subQuery1 UNION subQuery2 UNION ... LIMIT N"
+            TextUtils.join(" UNION ", subQueries) + " LIMIT " + target.getLimit();


We need to make sure that we do not pass more than 999 bind elements. The best way to do this probably to validate that the query only contains 100 DNF terms (or what we decide the limit is).

Can you also add parenthesis to indicate whether the LIMIT belongs to all statements or just the last one?

I'll add an assertion here, and when I post the DNF calculation code I'll throw an exception if the number of DNF terms exceeds a certain number.

Also updating this to SELECT * FROM (query1 UNION query2 UNION ...) LIMIT X.

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

schmidt-sebastian · 2022-02-04T21:52:05Z

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

+   * Iterates over all field indexes that are used to serve the given target, and returns the least
+   * recent offset of them all. Asserts that the target can be served from index.
+   */
+  IndexOffset minOffset(Target target);


All Java methods should start with a verb: getMinOffset()

You should also align the two comments on their usage of "lowest" and "least recent" (which contrasts min).

schmidt-sebastian · 2022-02-04T21:53:23Z

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

+    return true;
+  }
+
+  private IndexOffset getLeastRecentIndexOffset(Collection<FieldIndex> fieldIndexes) {


This name might still need an update.

ehsannas · 2022-02-05T05:28:22Z

/retest

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

ehsannas marked this pull request as draft January 25, 2022 22:42

ehsannas added 6 commits January 25, 2022 16:47

Serve or queries from the index.

6d9be9a

Update test.

6e62711

Clean up.

4860613

Move LogicUtils to 'util' directory.

893a014

Reuse existing code for finding least recent index offset.

28ac1cb

Add missing function to MemoryIndexManager

81b6e16

ehsannas force-pushed the ehsann/or-queries-pr-6 branch from e023f88 to 81b6e16 Compare January 25, 2022 23:10

Add license to new file.

c829fe1

ehsannas requested review from schmidt-sebastian and wu-hui January 26, 2022 20:21

ehsannas assigned schmidt-sebastian Jan 26, 2022

ehsannas marked this pull request as ready for review January 26, 2022 20:22

ehsannas changed the title ~~Serve or queries from the index.~~ Serve OR queries from the index. Jan 26, 2022

schmidt-sebastian reviewed Jan 27, 2022

View reviewed changes

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

Address comments.

d65a6e0

ehsannas assigned schmidt-sebastian and unassigned ehsannas Feb 4, 2022

ehsannas requested a review from schmidt-sebastian February 4, 2022 18:54

schmidt-sebastian approved these changes Feb 4, 2022

View reviewed changes

schmidt-sebastian assigned ehsannas and unassigned schmidt-sebastian Feb 4, 2022

Address comments.

4b7858b

ehsannas merged commit 69104c7 into master Feb 6, 2022

ehsannas deleted the ehsann/or-queries-pr-6 branch February 6, 2022 21:36

firebase locked and limited conversation to collaborators Mar 9, 2022

		import androidx.annotation.Nullable;
		import com.google.common.collect.ObjectArrays;

Serve OR queries from the index. #3368

Serve OR queries from the index. #3368

Uh oh!

Conversation

ehsannas commented Jan 25, 2022

Uh oh!

google-oss-bot commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-firestore

Test Logs

Notes

Uh oh!

google-oss-bot commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-firestore

Test Logs

Notes

Uh oh!

ehsannas commented Jan 26, 2022

Uh oh!

ehsannas commented Jan 26, 2022

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!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehsannas commented Feb 5, 2022

Uh oh!

Uh oh!

google-oss-bot commented Jan 25, 2022 •

edited

Loading

Coverage Report ¹

`firebase-firestore`

google-oss-bot commented Jan 25, 2022 •

edited

Loading

Size Report ¹

`firebase-firestore`