Port TargetIndexMatcher #5975

schmidt-sebastian · 2022-02-04T17:46:14Z

This is a straightforward port from Android.

changeset-bot · 2022-02-04T17:46:17Z

⚠️ No Changeset found

Latest commit: dd8ac83

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

google-oss-bot · 2022-02-04T18:05:15Z

Size Report ¹

Affected Products

`@firebase/firestore`

Type	Base (`0482aa5`)	Merge (`a031e10`)	Diff
browser	231 kB	233 kB	+2.16 kB (+0.9%)
esm5	288 kB	291 kB	+2.53 kB (+0.9%)
main	460 kB	463 kB	+3.65 kB (+0.8%)
module	231 kB	233 kB	+2.16 kB (+0.9%)
react-native	231 kB	233 kB	+2.16 kB (+0.9%)

`bundle`

Type	Base (`0482aa5`)	Merge (`a031e10`)	Diff
firestore (Persistence)	232 kB	234 kB	+2.58 kB (+1.1%)
firestore (Query Cursors)	190 kB	190 kB	+91 B (+0.0%)
firestore (Query)	191 kB	191 kB	+91 B (+0.0%)
firestore (Read data once)	180 kB	180 kB	+91 B (+0.1%)
firestore (Realtime updates)	182 kB	182 kB	+91 B (+0.1%)
firestore (Transaction)	164 kB	164 kB	+85 B (+0.1%)
firestore (Write data)	163 kB	163 kB	+85 B (+0.1%)

firebase
Type Base (0482aa5) Merge (a031e10) Diff
firebase-compat.js 755 kB 758 kB +2.52 kB (+0.3%)
firebase-firestore-compat.js 283 kB 285 kB +2.52 kB (+0.9%)
firebase-firestore.js 780 kB 784 kB +4.06 kB (+0.5%)

Test Logs

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

google-oss-bot · 2022-02-04T18:20:04Z

Size Analysis Report ¹

This report is too large (445,136 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/SOSOOyq8ut.html

github-actions · 2022-02-04T19:33:49Z

Changeset File Check ✅

No modified packages are missing from the changeset file.
No changeset formatting errors detected.

ehsannas

Looks good. Just a few nits

ehsannas · 2022-02-04T19:46:09Z

packages/firestore/src/model/target_index_matcher.ts

+/**
+ * A light query planner for Firestore.
+ *
+ * This class matches a `FieldIndex`z against a Firestore Query `Target`. It


remove typo (z)

ehsannas · 2022-02-04T19:48:13Z

packages/firestore/src/model/target_index_matcher.ts

+  private readonly collectionId: string;
+  // The single inequality filter of the target (if it exists).
+  private readonly inequalityFilter?: FieldFilter;
+  // The list of equality filter of the target.


s/filter/filters

ehsannas · 2022-02-04T20:48:28Z

packages/firestore/src/model/target_index_matcher.ts

+
+    // Process all equalities first. Equalities can appear out of order.
+    for (; segmentIndex < segments.length; ++segmentIndex) {
+      // We attempt to greedily match all segments to equality filters. If a a


s/If a a/If a

ehsannas · 2022-02-04T20:48:48Z

packages/firestore/src/model/target_index_matcher.ts

+    // Process all equalities first. Equalities can appear out of order.
+    for (; segmentIndex < segments.length; ++segmentIndex) {
+      // We attempt to greedily match all segments to equality filters. If a a
+      // filter matches an index segments, we can mark the segment as used.


s/an index segments/an index segment

ehsannas · 2022-02-04T20:49:11Z

packages/firestore/src/model/target_index_matcher.ts

+    for (; segmentIndex < segments.length; ++segmentIndex) {
+      // We attempt to greedily match all segments to equality filters. If a a
+      // filter matches an index segments, we can mark the segment as used.
+      // Since it is not possible to use the same filed path in both an equality


s/filed/field

ehsannas · 2022-02-04T20:55:35Z

packages/firestore/src/model/target_index_matcher.ts

+      ++segmentIndex;
+    }
+
+    // All remaining segment need to represent the prefix of the target's


s/segment/segments

ehsannas · 2022-02-04T21:11:09Z

packages/firestore/test/unit/model/target_index_matcher.test.ts

+    }
+  });
+
+  it('inequalitiesWithDefaultOrder', () => {


inequalities with default order ?

Same for the next two tests

ehsannas · 2022-02-04T21:29:39Z

packages/firestore/test/unit/model/target_index_matcher.test.ts

+        queryToTarget(query('collId'))
+      );
+      const index = fieldIndex('collId');
+      expect(() => targetIndexMatcher.servedByIndex(index)).to.not.throw;


I don't see any throw statements in target_index_matcher.ts. Similarly, I don't see any throw statements in TargetIndexMatcher.java.

does .to.throw and .to.not.throw apply to assertions (e.g. the debugAssert in the code)? The other examples of .to.throw that I found in other test files corresponded to a throw statement in the source.

Yes it applies to the assert.

packages/firestore/test/unit/model/target_index_matcher.test.ts

ehsannas · 2022-02-04T21:32:37Z

packages/firestore/test/unit/model/target_index_matcher.test.ts

+    }
+  });
+
+  it('array-contains is idempotent', () => {


ts:idempotent
java:independent

independent it is

schmidt-sebastian

Wheee... so many typos. Thanks for catching.

schmidt-sebastian · 2022-02-10T16:08:14Z

packages/firestore/src/model/target_index_matcher.ts

+  private readonly collectionId: string;
+  // The single inequality filter of the target (if it exists).
+  private readonly inequalityFilter?: FieldFilter;
+  // The list of equality filter of the target.


schmidt-sebastian · 2022-02-10T16:08:29Z

packages/firestore/src/model/target_index_matcher.ts

+
+    // Process all equalities first. Equalities can appear out of order.
+    for (; segmentIndex < segments.length; ++segmentIndex) {
+      // We attempt to greedily match all segments to equality filters. If a a


schmidt-sebastian · 2022-02-10T16:08:46Z

packages/firestore/src/model/target_index_matcher.ts

+    // Process all equalities first. Equalities can appear out of order.
+    for (; segmentIndex < segments.length; ++segmentIndex) {
+      // We attempt to greedily match all segments to equality filters. If a a
+      // filter matches an index segments, we can mark the segment as used.


schmidt-sebastian · 2022-02-10T16:08:51Z

packages/firestore/src/model/target_index_matcher.ts

+    for (; segmentIndex < segments.length; ++segmentIndex) {
+      // We attempt to greedily match all segments to equality filters. If a a
+      // filter matches an index segments, we can mark the segment as used.
+      // Since it is not possible to use the same filed path in both an equality


schmidt-sebastian · 2022-02-10T16:09:13Z

packages/firestore/src/model/target_index_matcher.ts

+      // We attempt to greedily match all segments to equality filters. If a a
+      // filter matches an index segments, we can mark the segment as used.
+      // Since it is not possible to use the same filed path in both an equality
+      // and inequality/oderBy cause, we do not have to consider the possibility


... sorry :/

schmidt-sebastian · 2022-02-10T16:09:35Z

packages/firestore/src/model/target_index_matcher.ts

+      ++segmentIndex;
+    }
+
+    // All remaining segment need to represent the prefix of the target's


schmidt-sebastian · 2022-02-10T16:09:35Z

packages/firestore/src/model/target_index_matcher.ts

+    }
+
+    // All remaining segment need to represent the prefix of the target's
+    // orderBy


schmidt-sebastian · 2022-02-10T16:10:22Z

packages/firestore/test/unit/model/target_index_matcher.test.ts

+    }
+  });
+
+  it('inequalitiesWithDefaultOrder', () => {


schmidt-sebastian · 2022-02-10T16:10:53Z

packages/firestore/test/unit/model/target_index_matcher.test.ts

+        queryToTarget(query('collId'))
+      );
+      const index = fieldIndex('collId');
+      expect(() => targetIndexMatcher.servedByIndex(index)).to.not.throw;


Yes it applies to the assert.

schmidt-sebastian · 2022-02-10T16:11:25Z

packages/firestore/test/unit/model/target_index_matcher.test.ts

+    }
+  });
+
+  it('array-contains is idempotent', () => {


independent it is

ehsannas · 2022-02-10T16:45:49Z

forgot to push your commit?

…rebase-js-sdk into mrschmidt/targetindexmatcher

schmidt-sebastian · 2022-02-10T18:21:46Z

@ehsannas Yup, had to pull and then push!

Port TargetIndexMatcher

37250f0

Remove only

861e13a

schmidt-sebastian requested a review from ehsannas February 4, 2022 17:55

schmidt-sebastian assigned ehsannas Feb 4, 2022

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

Lint

438e0c3

Update target_index_matcher.test.ts

aa59e04

ehsannas reviewed Feb 4, 2022

View reviewed changes

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

Typos

ef609b2

schmidt-sebastian commented Feb 10, 2022

View reviewed changes

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

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

Merge branch 'mrschmidt/targetindexmatcher' of github.com:firebase/fi…

dd8ac83

…rebase-js-sdk into mrschmidt/targetindexmatcher

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

ehsannas approved these changes Feb 10, 2022

View reviewed changes

schmidt-sebastian merged commit eaa5170 into master Feb 10, 2022

schmidt-sebastian deleted the mrschmidt/targetindexmatcher branch February 10, 2022 20:51

firebase locked and limited conversation to collaborators Mar 13, 2022

Port TargetIndexMatcher #5975

Port TargetIndexMatcher #5975

Uh oh!

Conversation

schmidt-sebastian commented Feb 4, 2022

Uh oh!

changeset-bot bot commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

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

Uh oh!

Size Report 1

Affected Products

@firebase/firestore

bundle

firebase

Test Logs

Uh oh!

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

Uh oh!

Size Analysis Report 1

Test Logs

Uh oh!

github-actions bot commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changeset File Check ✅

Uh oh!

ehsannas 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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

changeset-bot bot commented Feb 4, 2022 •

edited

Loading

google-oss-bot commented Feb 4, 2022 •

edited

Loading

Size Report ¹

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Feb 4, 2022 •

edited

Loading

Size Analysis Report ¹

github-actions bot commented Feb 4, 2022 •

edited

Loading