Implement Index Scanning #6008

schmidt-sebastian · 2022-02-14T23:24:03Z

This adds all index evaluation logic to the Web SDK and ports all tests from Android. The query logic is mostly the same as Android, with the exception of notIn/notEquals queries which are executed by doing scans across split ranges.

Example:

Query q = coll.where("a", ">", 2).where("a", "!=", 3);

Android: SELECT * FROM (SELECT * FROM coll WHERE a > 2) WHERE a != 3

Web: Scan [a>2, a<3] && [a>3, ∞]

changeset-bot · 2022-02-14T23:24:05Z

⚠️ No Changeset found

Latest commit: 6b88a47

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

schmidt-sebastian · 2022-02-14T23:27:20Z

packages/firestore/src/local/index_manager.ts

@@ -103,13 +103,12 @@ export interface IndexManager {

  /**
   * Returns the documents that match the given target based on the provided
-   * index.
+   * index. Returns `null` if the target does not have a matching index.


This change is pending review on Android: firebase/firebase-android-sdk#3444

schmidt-sebastian · 2022-02-14T23:31:01Z

packages/firestore/src/platform/node/base64.ts

 }

 /** Converts a binary string to a Base64 encoded string. */
 export function encodeBase64(raw: string): string {
-  return new Buffer(raw, 'binary').toString('base64');
+  return Buffer.from(raw, 'binary').toString('base64');


This creates a deprecation warning in Node.

google-oss-bot · 2022-02-14T23:43:55Z

Size Report ¹

Affected Products

`@firebase/firestore`

Type	Base (`3a8d4c1`)	Merge (`eaf911b`)	Diff
browser	244 kB	251 kB	+6.59 kB (+2.7%)
esm5	304 kB	311 kB	+7.49 kB (+2.5%)
main	486 kB	500 kB	+14.0 kB (+2.9%)
module	244 kB	251 kB	+6.59 kB (+2.7%)
react-native	244 kB	251 kB	+6.59 kB (+2.7%)

`@firebase/firestore-lite`

Type	Base (`3a8d4c1`)	Merge (`eaf911b`)	Diff
browser	73.0 kB	73.1 kB	+137 B (+0.2%)
esm5	86.4 kB	86.5 kB	+137 B (+0.2%)
main	125 kB	126 kB	+116 B (+0.1%)
module	73.0 kB	73.1 kB	+137 B (+0.2%)
react-native	73.2 kB	73.3 kB	+137 B (+0.2%)

`bundle`

12 size changes

Type	Base (`3a8d4c1`)	Merge (`eaf911b`)	Diff
firestore (Persistence)	245 kB	252 kB	+6.72 kB (+2.7%)
firestore (Query Cursors)	192 kB	192 kB	+140 B (+0.1%)
firestore (Query)	193 kB	193 kB	+140 B (+0.1%)
firestore (Read data once)	181 kB	181 kB	+140 B (+0.1%)
firestore (Realtime updates)	183 kB	184 kB	+140 B (+0.1%)
firestore (Transaction)	166 kB	166 kB	+140 B (+0.1%)
firestore (Write data)	165 kB	165 kB	+140 B (+0.1%)
firestore-lite (Query Cursors)	56.8 kB	56.9 kB	+115 B (+0.2%)
firestore-lite (Query)	59.9 kB	60.0 kB	+137 B (+0.2%)
firestore-lite (Read data once)	44.4 kB	44.5 kB	+115 B (+0.3%)
firestore-lite (Transaction)	61.7 kB	61.8 kB	+115 B (+0.2%)
firestore-lite (Write data)	47.2 kB	47.3 kB	+115 B (+0.2%)

`firebase`

Type	Base (`3a8d4c1`)	Merge (`eaf911b`)	Diff
firebase-compat.js	769 kB	775 kB	+6.57 kB (+0.9%)
firebase-firestore-compat.js	296 kB	302 kB	+6.57 kB (+2.2%)
firebase-firestore-lite.js	250 kB	250 kB	+457 B (+0.2%)
firebase-firestore.js	812 kB	835 kB	+23.2 kB (+2.9%)

Test Logs

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

google-oss-bot · 2022-02-14T23:58:48Z

Size Analysis Report ¹

This report is too large (651,534 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

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

packages/firestore/src/local/simple_db.ts

packages/firestore/test/unit/local/simple_db.test.ts

packages/firestore/src/local/indexeddb_index_manager.ts

wu-hui · 2022-02-16T17:04:37Z

packages/firestore/src/local/indexeddb_index_manager.ts

+      // that we would split an existing range into multiple ranges that exclude
+      // the values from any notIn filter.
+
+      // The first index range starts with the lower bound and ends at the


I don't think I understand this well..what if notInValues[0] is not covered by the first range? What if it is < indexRange.lower?

That was indeed completely broken, and also did not work correctly on Android. I ported the Android fix firebase/firebase-android-sdk#3454 and adopted it to work with the Web port.

wu-hui · 2022-02-17T16:22:31Z

packages/firestore/src/local/indexeddb_index_manager.ts

+    ranges.push(
+      IDBKeyRange.bound(
+        indexRange.lower,
+        this.generateNotInBound(indexRange.lower, barriers[0]),


What if barriers[0] is larger than indexRange.upper, is that not possible?

I think we could do something like:

// Assume barriers is sorted, it is right? let runningLower = indexRange.lower let runningOpen = indexRange.lowerOpen for(const barrier: barriers) { if(barrier <= indexRange.lower) {continue;} if(barrier >= indexRange.upper) {break;} ranges.push([runningLower /*need to handle lowerOpen as well*/, barrier]); runningLower = barrier lowerOpen = true } // Handles last ranges.push([runningLower, indexRange.upper])

You could even do handle this by taking IDBKeyRange[], sort both indexRanges and barriers together..then process from left to right. But maybe let's not go there, it should be rare in real world scenarios.

Barriers are filtered both by the lower and the upper bound, so all barriers should fit between the range.

I have actually spent some time thinking about how to generalize the whole "barrier setup" so that I can treat IDBKeyRanges and barriers the same and only write one loop. I did not consider the approach that you outlined here, which solves some of the problems I had encountered while still reducing the lines of code. I updated the PR accordingly. Thanks!

Found a bunch of cases where this does not work. Will update soon.

I updated this code to work on browsers and Node. The browsers seem to do more validations, so I had to change some of the logic here. I also added a bunch of test code. Note that I myself don't fully grasp what is going on here anymore, but the tests are all passing and I think that I covered all corner cases.

Still broken

Okay, I rewrote this part of the PR in its entirety. It should be much cleaner now.

wu-hui · 2022-02-18T18:59:32Z

packages/firestore/src/local/indexeddb_index_manager.ts

+    const bounds: IndexEntry[] = [];
+    bounds.push(lower);
+    for (const notInValue of notInValues) {
+      const sortsAfter = indexEntryComparator(notInValue, lower);


Maybe cmpToLower and cmpToUpper?

Sounds good. I am not quite sold on either name, but using "cmp" helps a bit.

wu-hui · 2022-02-18T19:18:57Z

packages/firestore/src/local/indexeddb_index_manager.ts

+      const sortsAfter = indexEntryComparator(notInValue, lower);
+      const sortsBefore = indexEntryComparator(notInValue, upper);
+
+      if (sortsAfter > 0 && sortsBefore < 0) {


How do you feel about:

if (cmpToLower == 0) { // Lower happens to be excluded by `notInValue`. bounds[0] = lower.successor(); } else if (cmpToLower > 0 && cmpToUpper < 0) { // `notInValue` is in the middle of the range ... } else if(cmpToUpper == 0) { // Upper happens to be excluded by `notInValue`. } else { // `notInValue` (and hence the following values) is out of the range break; }

Done. Good suggestion with the break statement.

wu-hui · 2022-02-18T19:22:03Z

packages/firestore/src/local/indexeddb_index_manager.ts

+   * Maps from a target to its equivalent list of sub-targets. Each sub-target
+   * contains only one term from the target's disjunctive normal form (DNF).
+   */
+  private targetToDnfSubTargets = new ObjectMap<Target, Target[]>(


Unrelated to this PR: but SubTarget indicates SubQuery as well, and that term meant very different thing to most people.

I'm not quite sure I fully understand this. This part of the code only deals with targets - and only the ones normalized for indexing, not the targets we send to the backend.

My point is maybe we can rename the term sub-target..but we can deal with this later.

packages/firestore/src/local/indexeddb_index_manager.ts

wu-hui · 2022-02-22T14:57:23Z

packages/firestore/src/local/indexeddb_index_manager.ts

+   * Maps from a target to its equivalent list of sub-targets. Each sub-target
+   * contains only one term from the target's disjunctive normal form (DNF).
+   */
+  private targetToDnfSubTargets = new ObjectMap<Target, Target[]>(


My point is maybe we can rename the term sub-target..but we can deal with this later.

packages/firestore/src/local/indexeddb_index_manager.ts

wu-hui · 2022-02-22T14:57:59Z

packages/firestore/src/local/indexeddb_index_manager.ts

+    const bounds: IndexEntry[] = [];
+    bounds.push(lower);
+    for (const notInValue of notInValues) {
+      const cpmToLower = indexEntryComparator(notInValue, lower);


github-actions · 2022-02-22T16:40:31Z

Changeset File Check ⚠️

Changeset formatting error in following file:

Some packages have been changed but no changesets were found. Run `changeset add` to resolve this error.
If this change doesn't need a release, run `changeset add --empty`.

schmidt-sebastian force-pushed the mrschmidt/remainingindexmanager branch 2 times, most recently from 36f4a5a to ff719f2 Compare February 14, 2022 23:26

schmidt-sebastian commented Feb 14, 2022

View reviewed changes

schmidt-sebastian requested a review from wu-hui February 14, 2022 23:40

schmidt-sebastian assigned wu-hui Feb 14, 2022

schmidt-sebastian added the api: firestore label Feb 14, 2022

Base automatically changed from mrschmidt/updateindexentries to master February 15, 2022 17:02

Implement Index Scanning

7ccadd8

schmidt-sebastian force-pushed the mrschmidt/remainingindexmanager branch from d9a242d to 7ccadd8 Compare February 15, 2022 17:09

wu-hui reviewed Feb 16, 2022

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Feb 16, 2022

Review

5e5f31f

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Feb 16, 2022

wu-hui reviewed Feb 17, 2022

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Feb 17, 2022

schmidt-sebastian added 2 commits February 17, 2022 13:53

Cleanup

e55c5e7

Simplify

cf1880e

schmidt-sebastian assigned wu-hui and schmidt-sebastian and unassigned schmidt-sebastian and wu-hui Feb 17, 2022

Use cleaner code for barriers

2a4af04

schmidt-sebastian unassigned wu-hui Feb 17, 2022

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Feb 17, 2022

schmidt-sebastian added 3 commits February 17, 2022 18:15

Better test

fc1bd37

Lint

00df97d

Add assert

b87e324

schmidt-sebastian assigned schmidt-sebastian and unassigned wu-hui Feb 18, 2022

Use precompute bounds

ba1507f

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Feb 18, 2022

wu-hui reviewed Feb 18, 2022

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Feb 18, 2022

Next round

56e7441

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Feb 18, 2022

wu-hui approved these changes Feb 22, 2022

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Feb 22, 2022

Update indexeddb_index_manager.ts

aa5c523

schmidt-sebastian mentioned this pull request Feb 22, 2022

Backport tests from Web firebase/firebase-android-sdk#3474

Merged

Fix browser tests

6b88a47

schmidt-sebastian merged commit 13c0895 into master Feb 22, 2022

schmidt-sebastian deleted the mrschmidt/remainingindexmanager branch February 22, 2022 23:12

firebase locked and limited conversation to collaborators Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Index Scanning #6008

Implement Index Scanning #6008

schmidt-sebastian commented Feb 14, 2022 •

edited

Loading

changeset-bot bot commented Feb 14, 2022 •

edited

Loading

schmidt-sebastian Feb 14, 2022

schmidt-sebastian Feb 14, 2022

google-oss-bot commented Feb 14, 2022 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore-lite`

`bundle`

`firebase`

google-oss-bot commented Feb 14, 2022 •

edited

Loading

wu-hui Feb 16, 2022

schmidt-sebastian Feb 16, 2022

wu-hui Feb 17, 2022

schmidt-sebastian Feb 17, 2022

schmidt-sebastian Feb 17, 2022

schmidt-sebastian Feb 17, 2022

schmidt-sebastian Feb 18, 2022

schmidt-sebastian Feb 18, 2022

wu-hui Feb 18, 2022

schmidt-sebastian Feb 18, 2022

wu-hui Feb 18, 2022

schmidt-sebastian Feb 18, 2022

wu-hui Feb 18, 2022

schmidt-sebastian Feb 18, 2022

wu-hui Feb 22, 2022

wu-hui Feb 22, 2022

wu-hui Feb 22, 2022

schmidt-sebastian Feb 22, 2022

github-actions bot commented Feb 22, 2022

Implement Index Scanning #6008

Implement Index Scanning #6008

Conversation

schmidt-sebastian commented Feb 14, 2022 • edited Loading

changeset-bot bot commented Feb 14, 2022 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-bot commented Feb 14, 2022 • edited Loading

Size Report 1

Affected Products

@firebase/firestore

@firebase/firestore-lite

bundle

firebase

Test Logs

google-oss-bot commented Feb 14, 2022 • edited Loading

Size Analysis Report 1

Test Logs

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

github-actions bot commented Feb 22, 2022

Changeset File Check ⚠️

schmidt-sebastian commented Feb 14, 2022 •

edited

Loading

changeset-bot bot commented Feb 14, 2022 •

edited

Loading

google-oss-bot commented Feb 14, 2022 •

edited

Loading

Size Report ¹

`@firebase/firestore`

`@firebase/firestore-lite`

`bundle`

`firebase`

google-oss-bot commented Feb 14, 2022 •

edited

Loading

Size Analysis Report ¹