feat: OR Query implementation by Mariatta · Pull Request #698 · googleapis/python-firestore

Mariatta · 2023-03-20T22:39:31Z

Introduce new Filter classes:

FieldFilter
And
Or

Add filter keyword arg to Query.where()
The positional arguments in Query.where() are now optional. UserWarning is now emitted when using where() without keyword args.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

danieljbruce

Looks good. Just a few comments.

danieljbruce · 2023-03-22T19:45:34Z

-                op=_enum_from_op_string(op_string),
-                value=_helpers.encode_value(value),
+
+        field_path_module.split_field_path(field_path)  # raises


Was there meant to be more text in this comment?

Hmm, this seems to have always been there since this code was added: https://github.com/googleapis/python-firestore/blame/main/google/cloud/firestore_v1/base_query.py#L246

Perhaps a leftover debugging info. I can remove it.

danieljbruce · 2023-03-22T19:51:40Z

-        new_filters = self._field_filters + (filter_pb,)
+            new_filters += (filter_pb,)
+        elif isinstance(filter, BaseFilter):
+            new_filters += (filter._to_pb(),)


Something we ran into with nodejs-datastore was that if we stored new variable types in this object that contained the old filter types then users who interacted with this array/list directly would see new filter types in this variable. If the user is then going to expect this variable to be an array of the old filter type then it could cause problems. Would the user ever interact with this variable directly out of the base query?

new_filters is a local variable only on the where function. User will not interact with it directly.

danieljbruce · 2023-03-22T20:06:45Z

-            return _filter_pb(self._field_filters[0])
+            filter = self._field_filters[0]
+            if isinstance(filter, query.StructuredQuery.CompositeFilter):
+                return query.StructuredQuery.Filter(composite_filter=filter)


This function returns a protobuf?

yes.
It returns a new instance of the protobuf query.StructuredQuery.Filter()

Mariatta · 2023-03-24T16:30:09Z

 def query(query_docs):
    collection, stored, allowed_vals = query_docs
-    query = collection.where("a", "==", 1)
+    query = collection.where(filter=FieldFilter("a", "==", 1))


To the reviewers, this is an example of how the change in the where function.
It adds the keyword only arg filter, and you can see how it works before vs now.

The old behavior without the filter arg is still expected to work, correct? Are there any system tests that still validate the old behavior or is that unit test only now?

Yes it still works, and I only have the unit test for it. I can add the system test too.

I added additional system tests here: eff9328

Mariatta · 2023-03-24T16:32:07Z

        assert r.value == 2  # there are still only 2 docs
+
+
+def test_query_with_and_composite_filter(query_docs):


The test cases below shows how to build composite filters using And and Or and how to pass them to the filter argument in where.

Introduce new Filter classes: FieldFilter And Or Add "filter" keyword arg to "Query.where()" The positional arguments in "Query.where()" are now optional. UserWarning is now emitted when using "where()" without keyword args.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…firestore into feat-or-query

andrewsg · 2023-03-31T18:18:06Z

 def query(query_docs):
    collection, stored, allowed_vals = query_docs
-    query = collection.where("a", "==", 1)
+    query = collection.where(filter=FieldFilter("a", "==", 1))


The old behavior without the filter arg is still expected to work, correct? Are there any system tests that still validate the old behavior or is that unit test only now?

Mariatta requested review from a team March 20, 2023 22:39

product-auto-label Bot added size: xl Pull request size is extra large. api: firestore Issues related to the googleapis/python-firestore API. labels Mar 20, 2023

Mariatta force-pushed the feat-or-query branch 2 times, most recently from 2cb52d3 to 1a7f8fd Compare March 21, 2023 00:48

Mariatta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 21, 2023

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

kolea2 suggested changes Mar 22, 2023

View reviewed changes

Comment thread google/cloud/firestore_v1/base_collection.py

Comment thread tests/system/test_system.py

danieljbruce reviewed Mar 22, 2023

View reviewed changes

Mariatta requested a review from kolea2 March 23, 2023 03:44

Mariatta commented Mar 24, 2023

View reviewed changes

kolea2 reviewed Mar 30, 2023

View reviewed changes

Comment thread google/cloud/firestore_v1/base_collection.py

Comment thread tests/system/test_system.py

Mariatta added 5 commits March 30, 2023 23:02

feat: OR Query implementation

ae87912

Introduce new Filter classes: FieldFilter And Or Add "filter" keyword arg to "Query.where()" The positional arguments in "Query.where()" are now optional. UserWarning is now emitted when using "where()" without keyword args.

Add test coverage

40532d8

Add system tests using transaction

df33723

Add more complex system test

8045504

Remove leftover comment

ce28b92

Mariatta force-pushed the feat-or-query branch from 0f34575 to ce28b92 Compare March 31, 2023 06:02

Mariatta and others added 3 commits March 30, 2023 23:03

Lint fix

8b1b678

🦉 Updates from OwlBot post-processor

19f7098

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Merge branch 'feat-or-query' of https://github.com/googleapis/python-…

8a9728a

…firestore into feat-or-query

kolea2 approved these changes Mar 31, 2023

View reviewed changes

andrewsg reviewed Mar 31, 2023

View reviewed changes

Add system test demonstrating legacy where still works and returns value

eff9328

andrewsg approved these changes Mar 31, 2023

View reviewed changes

Remove debugging comment.

fa0a16c

Mariatta merged commit 44dd5d6 into main Apr 3, 2023

Mariatta deleted the feat-or-query branch April 3, 2023 15:04

release-please Bot mentioned this pull request Apr 3, 2023

chore(main): release 2.11.0 #702

Merged

XGeffrier mentioned this pull request Apr 26, 2023

UserWarning: Detected filter using positional arguments. #705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: OR Query implementation#698

feat: OR Query implementation#698
Mariatta merged 10 commits into
mainfrom
feat-or-query

Mariatta commented Mar 20, 2023 •

edited

Loading

Uh oh!

Uh oh!

danieljbruce left a comment

danieljbruce Mar 22, 2023

Mariatta Mar 23, 2023

danieljbruce Mar 22, 2023

Mariatta Mar 23, 2023

Uh oh!

danieljbruce Mar 22, 2023

Mariatta Mar 23, 2023

Mariatta Mar 24, 2023

andrewsg Mar 31, 2023

Mariatta Mar 31, 2023

Mariatta Mar 31, 2023

Mariatta Mar 24, 2023

Uh oh!

Uh oh!

Uh oh!

andrewsg Mar 31, 2023

Labels

5 participants

		assert r.value == 2 # there are still only 2 docs


		def test_query_with_and_composite_filter(query_docs):

Uh oh!

Conversation

Mariatta commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

danieljbruce 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

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Labels

5 participants

Mariatta commented Mar 20, 2023 •

edited

Loading