feat: OR Queries by MarkDuckworth · Pull Request #1800 · googleapis/nodejs-firestore

MarkDuckworth · 2022-11-23T17:44:14Z

Adding support for OR queries

ehsannas

Thanks!

…rkduckworth/or-queries

* OR operator support and integration tests. * Updating documentation and member visibility. * Remove usage of OPERATOR_UNSPECIFIED standing in for OR. * Removing private and internal tags from OR query public API members. * Ensure that new OR Query features are in firestore.d.ts and are exported from the main entry point. * Update documentation for OR query features to match android PR 4274. * Removing CompositeFilter and UnaryFilter from the type definitions and JS doc. * Corrected the descending order test for OR queries. * fix: update generated proto types; fix the update script (#1825) * Adding OR enum value for composit filter operator. --------- Co-authored-by: Alexander Fenster <[email protected]>

…ulator.

ehsannas

Thanks for the update. Looks good, but 1 case can be changed.

ehsannas · 2023-02-23T01:31:31Z

+      expectDocs(
+        await collection
+          .where(
+            Filter.or(
+              Filter.where('a', '==', 2),
+              Filter.where('b', 'in', [2, 3])
+            )
+          )
+          .get(),
+        'doc3',
+        'doc4',
+        'doc6'
+      );


This one doesn't need a composite index.

so you can break it to 2 tests
supports OR queries with in --> doesn't need composite index (because it doesn't have inequality)
supports OR queries with not-in --> needs composite index.

Split this. Thanks.

danieljbruce

First look. I notice that the .d.ts files are included here, but aren't these files usually generated?

danieljbruce · 2023-03-03T20:46:48Z

+   * });
+   * ```
+   */
+  public static or(...filters: Filter[]): Filter {


I implemented these static functions outside of the filter class so that they could be used without prefixing with Filter.or or Filter.and for instance.

This came up in API design review and we decided to go with this approach because it is more consistent with existing static functions in the nodejs-firestore SDK. See for example, the FieldValue class.

danieljbruce · 2023-03-03T20:48:21Z

+ * @private
+ * @internal
+ */
+export class UnaryFilter extends Filter {


I think we want to call this a PropertyFilter to be consistent with the documentation.

UnaryFilter was ported from Android, which I need to stay consistent with. This class is not public, so we could conceivably change the name in the future.

danieljbruce · 2023-03-03T21:01:15Z

+  }
+
+  public toProto(): api.StructuredQuery.IFilter {
+    if (this.filters.length === 1) {


Just a note here that for the datastore PR, we still use an AND filter with one element because it fits in naturally with the way that the code already is.

That sounds good. I think that's also acceptable. This particular implementation was done to be consistent with other Firestore SDK implementations (ported from Android SDK).

danieljbruce · 2023-03-03T21:07:55Z

+    fieldPathOrFilter: string | firestore.FieldPath | Filter,
+    opStr?: firestore.WhereFilterOp,
+    value?: unknown
  ): Query<T> {


Just a note that this is like the .filter function in datastore

I'm not sure on the history, but I agree the naming conventions deviate between Firestore and Datastore.

danieljbruce · 2023-03-03T21:12:43Z

+    }
+    return new CompositeFilterInternal(
+      parsedFilters,
+      compositeFilterData._getOperator() === 'AND' ? 'AND' : 'OR'


Should we throw an error here instead if OR or AND is not provided?

MarkDuckworth · 2023-03-03T23:08:26Z

First look. I notice that the .d.ts files are included here, but aren't these files usually generated?

@danieljbruce I'm not sure of the history of this, but I have an open ticket to modify this SDK so that we use the generated d.ts files. See b/269761436

MarkDuckworth added 4 commits November 22, 2022 11:17

WIP: OR Query with unit tests. No OR operator.

7d908a6

Updating documentation and member visibility.

71b613d

Formatting

ccbc926

Skipping OR Query system test.

cc66a6c

MarkDuckworth requested review from a team November 23, 2022 17:44

product-auto-label Bot added size: l Pull request size is large. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Nov 23, 2022

Updated copyright holder for new files.

eee4aeb

MarkDuckworth changed the title ~~Composite operators for OR Queries.~~ refactor: Internal implementation of composite operators for OR Queries. Nov 23, 2022

MarkDuckworth assigned ehsannas Nov 23, 2022

ehsannas reviewed Nov 29, 2022

View reviewed changes

ehsannas assigned MarkDuckworth and unassigned ehsannas Nov 29, 2022

PR fixes.

e7d3bf0

MarkDuckworth assigned ehsannas and unassigned MarkDuckworth Nov 29, 2022

ehsannas approved these changes Nov 29, 2022

View reviewed changes

MarkDuckworth added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 9, 2023

MarkDuckworth and others added 2 commits February 17, 2023 07:48

Merge branch 'main' of github.com:googleapis/nodejs-firestore into ma…

b5178b3

…rkduckworth/or-queries

product-auto-label Bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Feb 17, 2023

Merge branch 'main' into markduckworth/or-queries

6ad2840

MarkDuckworth changed the title ~~refactor: Internal implementation of composite operators for OR Queries.~~ feat: OR Queries Feb 18, 2023

MarkDuckworth added 2 commits February 17, 2023 17:07

Tests that require an index are configured to only run against the em…

fbe2a22

…ulator.

MarkDuckworth requested review from ehsannas and removed request for ehsannas February 22, 2023 17:33

MarkDuckworth unassigned ehsannas Feb 22, 2023

MarkDuckworth requested a review from ehsannas February 22, 2023 17:37

MarkDuckworth assigned ehsannas Feb 22, 2023

ehsannas reviewed Feb 23, 2023

View reviewed changes

Test cleanup based on PR comments.

c5e8be0

ehsannas approved these changes Mar 2, 2023

View reviewed changes

Merge branch 'main' into markduckworth/or-queries

73d4756

MarkDuckworth added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 3, 2023

yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 3, 2023

ehsannas removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 3, 2023

danieljbruce reviewed Mar 3, 2023

View reviewed changes

ehsannas merged commit 983a477 into main Mar 6, 2023

ehsannas deleted the markduckworth/or-queries branch March 6, 2023 17:44

release-please Bot mentioned this pull request Mar 6, 2023

chore(main): release 6.5.0 #1826

Merged

danReynolds mentioned this pull request Apr 30, 2023

Firestore Filter.OR clarification #1845

Closed

Conversation

MarkDuckworth commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehsannas left a comment

Choose a reason for hiding this comment

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!

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

MarkDuckworth commented Mar 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MarkDuckworth commented Nov 23, 2022 •

edited

Loading