feat: Add support for Sum and Avg aggregation query #437
Conversation
product-auto-label
bot
added
size: l
Mariatta
force-pushed
the
feat-sum-avg-create-pr
branch
from
db1f820
to
de5dbef
Compare
Add .sum() and .avg() functions to aggregation Refactor limit to be passed in to the nested query's limit Unit tests
Mariatta
force-pushed
the
feat-sum-avg-create-pr
branch
from
de5dbef
to
105d4a0
Compare
|
LGTM, but we might want to add some more tests like in googleapis/python-firestore#715 |
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
kokoro:force-run
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| @@ -70,54 +70,156 @@ def nested_query(aggregation_query_client, ancestor_key): | |||
|
|
|||
There was a problem hiding this comment.
sorry for missing this previously, it looks like transactions will also need to be covered and tested.
There was a problem hiding this comment.
I added some tests to make sure the transaction id is captured when running a query inside the context of a transaction
Let me know if you had anything else in mind. I'm still trying to get my head around the relationship between transactions and queries, and I'm not seeing any existing tests or samples around this
There was a problem hiding this comment.
Looks like there are a few examples here: https://github.com/googleapis/python-datastore/blob/main/tests/system/test_transaction.py, if you want to move them into there instead (I don't have a strong preference)
There was a problem hiding this comment.
Yeah I was a little unsure which file to put these in since they involves both queries and transactions. I just looked into moving these to test_transaction.py, but all the test fixtures for configuring queries live in this file, so it's probably easiest to keep it here
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| When an aggregation query is run in a transaction, the transaction id should be sent with the request. | ||
| The result is the same as when it is run outside of a transaction. | ||
| """ | ||
| import mock |
There was a problem hiding this comment.
linked above, maybe we can make these more in line with https://github.com/googleapis/python-datastore/blob/main/tests/system/test_transaction.py so that we don't have to do any mocking
There was a problem hiding this comment.
The mock here is used to capture the transaction field sent along with the request, which I don't believe is captured by the other tests.
But yeah, on second thought that part probably doesn't belong here. I broke this test up and moved those parts into the unit tests
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| aggregation_pb.alias = self.alias | ||
| aggregation_pb.sum = query_pb2.AggregationQuery.Aggregation.Sum() | ||
| aggregation_pb.sum.property.name = self.property_ref | ||
| aggregation_pb.alias = self.alias |
There was a problem hiding this comment.
is this duplicated? (see line 98)
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
Add .sum() and .avg() functions to aggregation
Refactor limit to be passed in to the nested query's limit
Unit tests
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:
Fixes #<issue_number_goes_here> 🦕