feat: OR Query implementation #418

Mariatta · 2023-02-08T19:58:44Z

Introduce new Filter classes:

PropertyFilter
And
Or

Add "filter" keyword arg to "Query.add_filter()"

UserWarning is now emitted when using "add_filter()" 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

LGTM! I am thinking of subclassing the composite filter like you did instead of Relying on factory methods like Filter.AND and Filter.OR in nodejs-datastore.

danieljbruce · 2023-03-06T18:21:43Z

google/cloud/datastore/query.py

+            repr += f"\n\t{filter}"
+        return repr
+
+    def build_pb(self, container_pb=None):


For reference, the node.js version of this is here:

https://github.com/googleapis/nodejs-datastore/pull/1061/files#diff-87404c91a22dbabcc334f9a5288341281cb2b6625533e1207efcac16dc1e2ffdR147

danieljbruce · 2023-03-06T18:26:58Z

google/cloud/datastore/query.py

@@ -209,30 +329,62 @@ def filters(self):
        """
        return self._filters[:]

-    def add_filter(self, property_name, operator, value):
+    def add_filter(


The corresponding function name for this in nodejs-datastore is .filter.

danieljbruce · 2023-03-06T18:35:07Z

google/cloud/datastore/query.py

+        operator=None,
+        value=None,
+        *,
+        composite_filter=None,


Why are there separate parameters for whether this is a property filter or a composite filter?

I was thinking of making it explicit that we can accept two kinds of filter: a CompositeFilter or a PropertyFilter.
After our offline chat about its, I think it would be better if we accept just one parameter filter which can any descendants of the BaseFilter class.

I will make this change so that we will accept only one optional parameter (filter) instead of property_filter and composite_filter. This way, we can also remove the check below (https://github.com/googleapis/python-datastore/pull/418/files#diff-6053552b52329f41530b743e3fcb37f9e7622c7e165f751b94d08174df79ac26R442) where it raises error if user tries to pass both paramaters at the same time.

Thanks.

I have replaced the two parameters so there is only a filter parameter here now.

danieljbruce

LGTM

danieljbruce · 2023-03-06T18:53:43Z

google/cloud/datastore/query.py

@@ -688,6 +875,19 @@ def _pb_from_query(query):
    composite_filter = pb.filter.composite_filter
    composite_filter.op = query_pb2.CompositeFilter.Operator.AND

+    for filter in query.filters:


The node.js version of this code is at https://github.com/googleapis/nodejs-datastore/blob/d854b57ed2b9b9f9ea148daedeb6d64e4d99de4d/src/entity.ts#L1252. In nodejs we use an AND composition here. This looks correct, but I was just curious about the reason why we see this internal difference.

kolea2 · 2023-03-07T14:52:25Z

google/cloud/datastore/query.py

@@ -688,6 +875,19 @@ def _pb_from_query(query):
    composite_filter = pb.filter.composite_filter
    composite_filter.op = query_pb2.CompositeFilter.Operator.AND

+    for filter in query.filters:
+        if isinstance(filter, BaseCompositeFilter):


this logic looks like it's repeated in a few places - any way we can consolidate?

@Mariatta This was merged without addressing this comment. Will there be a follow-up on this?

I think you're referring to the similarity to the if/else build_pb above (lines 107-116), which gets called right after this one. The if/else is similar but the code being executed in each path is different.

kolea2 · 2023-03-07T14:53:00Z

google/cloud/datastore/query.py

-        property_filter.op = pb_op_enum
-
-        # Set the value to filter on based on the type.
-        if property_name == "__key__":


how come this logic is being removed?

This is moved the the PropertyFilter constructor, which is a new datastructure that is meant as a replacement of the property_name, operator, and value variables.
See https://github.com/googleapis/python-datastore/pull/418/files/a6cfacfdd674de0cf59fd6571c05ae4d1624318e#diff-6053552b52329f41530b743e3fcb37f9e7622c7e165f751b94d08174df79ac26R70 where this gets moved to

kolea2 · 2023-03-07T14:59:43Z

google/cloud/datastore/query.py


            client = datastore.Client()

        .. doctest:: query-filter

            >>> query = client.query(kind='Person')
-            >>> query = query.add_filter('name', '=', 'James')


what happens to this existing code? is it a warning?

Yes there will be a DeprecationWarning if they use it. (it still works)
https://github.com/googleapis/python-datastore/pull/418/files#diff-e90ee28143f5bf4b16f8a49b045c7df27c5019c55d0481ef0fd3cb224514385dR407

I have changed this to UserWarning.

Introduce new Filter classes: - PropertyFilter - And - Or Add "property_filter" and "composite_filter" keyword args to "Query.add_filter()" DeprecationWarning is now emitted when using "add_filter()" without keyword args

rwhogg · 2023-03-08T22:38:49Z

google/cloud/datastore/query.py

+        return repr
+
+    def build_pb(self, container_pb=None):
+        """Build the protobuf representation based on values in the Property Filter."""


Typo: Property -> Composite

Fixed. thanks.

rwhogg · 2023-03-08T22:40:44Z

google/cloud/datastore/query.py

+                property_name, operator, value = filter
+                filter = PropertyFilter(property_name, operator, value)
+                child_pb = container_pb.filters.add().property_filter
+                filter.build_pb(container_pb=child_pb)


Tempted to say you should break line 116 (107, 110) out rather than type it 3 times. But I can also see an argument for leaving it where it is, so either works.

Yes it makes sense to move it out, I've updated it.

rwhogg · 2023-03-08T22:43:54Z

google/cloud/datastore/query.py

+        elif hasattr(client, "namespace"):
+            self._namespace = client.namespace
+        else:
+            self._project = None


Shouldn't this be self._namespace?

Good catch! Fixed.

google/cloud/datastore/query.py

rwhogg · 2023-03-08T22:47:53Z

google/cloud/datastore/query.py

+                    "Can't pass in both the positional arguments and 'filter' at the same time"
+                )
+
+            if property_name == "__key__" and not isinstance(value, Key):


Nit: Can we put __key__ into a constant somewhere and use that instead? Less risk of accidentally mistyping the number of underscores IMO.

Great idea, I've created the KEY_PROPERTY_NAME constant and replaced the use of "__key__" here.

rwhogg · 2023-03-08T22:50:06Z

google/cloud/datastore/query.py

+            property_name, operator, value = filter
+            filter = PropertyFilter(property_name, operator, value)
+            pb_to_add = pb.filter.composite_filter.filters._pb.add().property_filter
+            filter.build_pb(container_pb=pb_to_add)


Same comment I left above about the same line appearing at the end of a 3-way if branch.

rwhogg · 2023-03-08T22:58:53Z

tests/unit/test_query.py

+    assert len(pb.filters) == 3
+
+
+def test_and_query_composite_filter():


Nit: Can we say base_composite_filter here?

rwhogg · 2023-03-08T23:07:18Z

tests/system/test_query.py

+
+    assert alive_count > 0
+    assert appearance_count > 0
+    assert stark_family_count > 0


Can we check for exact numbers here?

rwhogg

Left some comments, but overall 🚀

Mariatta requested review from a team as code owners February 8, 2023 19:58

product-auto-label bot added size: l Pull request size is large. api: datastore Issues related to the googleapis/python-datastore API. labels Feb 8, 2023

danieljbruce reviewed Feb 8, 2023

View reviewed changes

danieljbruce reviewed Mar 6, 2023

View reviewed changes

Mariatta requested a review from a team as a code owner March 6, 2023 18:35

danieljbruce approved these changes Mar 6, 2023

View reviewed changes

kolea2 reviewed Mar 7, 2023

View reviewed changes

Mariatta force-pushed the feat-or-query-pr branch from a6cfacf to 88e9a1f Compare March 8, 2023 02:22

danieljbruce changed the title ~~feat: OR Query implementation~~ feat: OR Query implementation and pull filter proto building into a separate class Mar 8, 2023

danieljbruce changed the title ~~feat: OR Query implementation and pull filter proto building into a separate class~~ feat: OR Query implementation Mar 8, 2023

Mariatta added 3 commits March 8, 2023 12:46

feat: OR Query implementation

493768e

Introduce new Filter classes: - PropertyFilter - And - Or Add "property_filter" and "composite_filter" keyword args to "Query.add_filter()" DeprecationWarning is now emitted when using "add_filter()" without keyword args

Replace composite_filter and property_filter with just filter

94dd3f3

Change from DeprecationWarning to UserWarning

cb5c46e

Mariatta force-pushed the feat-or-query-pr branch from edaa705 to cb5c46e Compare March 8, 2023 20:55

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

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

rwhogg reviewed Mar 8, 2023

View reviewed changes

google/cloud/datastore/query.py Outdated Show resolved Hide resolved

rwhogg reviewed Mar 8, 2023

View reviewed changes

rwhogg approved these changes Mar 8, 2023

View reviewed changes

Address code review feedback

12f2508

Address code review feedback

8cb5ae8

Mariatta merged commit 3256951 into main Mar 9, 2023

Mariatta deleted the feat-or-query-pr branch March 9, 2023 16:44

release-please bot mentioned this pull request Mar 9, 2023

chore(main): release 2.15.0 #425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: OR Query implementation #418

feat: OR Query implementation #418

Mariatta commented Feb 8, 2023 •

edited

Loading

danieljbruce left a comment •

edited

Loading

danieljbruce Mar 6, 2023

danieljbruce Mar 6, 2023

danieljbruce Mar 6, 2023

Mariatta Mar 7, 2023

Mariatta Mar 8, 2023

danieljbruce left a comment

danieljbruce Mar 6, 2023

kolea2 Mar 7, 2023

meredithslota Mar 23, 2023

Mariatta Mar 23, 2023

kolea2 Mar 7, 2023

Mariatta Mar 7, 2023

kolea2 Mar 7, 2023

Mariatta Mar 7, 2023

Mariatta Mar 8, 2023

rwhogg Mar 8, 2023

Mariatta Mar 8, 2023

rwhogg Mar 8, 2023

Mariatta Mar 8, 2023

rwhogg Mar 8, 2023

Mariatta Mar 8, 2023

rwhogg Mar 8, 2023

Mariatta Mar 8, 2023

rwhogg Mar 8, 2023

rwhogg Mar 8, 2023

Mariatta Mar 8, 2023

rwhogg Mar 8, 2023

Mariatta Mar 8, 2023

rwhogg left a comment

		assert len(pb.filters) == 3


		def test_and_query_composite_filter():

feat: OR Query implementation #418

feat: OR Query implementation #418

Conversation

Mariatta commented Feb 8, 2023 • edited Loading

danieljbruce left a comment • edited Loading

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

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

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

rwhogg left a comment

Choose a reason for hiding this comment

Mariatta commented Feb 8, 2023 •

edited

Loading

danieljbruce left a comment •

edited

Loading