Skip to content
This repository was archived by the owner on Mar 2, 2026. It is now read-only.

feat: Support the Count aggregation query#673

Merged
nicain merged 11 commits into
mainfrom
feat-aggregate-query
Jan 12, 2023
Merged

feat: Support the Count aggregation query#673
nicain merged 11 commits into
mainfrom
feat-aggregate-query

Conversation

@Mariatta

@Mariatta Mariatta commented Jan 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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> 🦕
@Mariatta Mariatta requested review from a team January 4, 2023 23:01
@product-auto-label product-auto-label Bot added size: xl Pull request size is extra large. api: firestore Issues related to the googleapis/python-firestore API. labels Jan 4, 2023
@Mariatta Mariatta force-pushed the feat-aggregate-query branch from 023213f to 76a3690 Compare January 4, 2023 23:03
danieljbruce
danieljbruce reviewed Jan 5, 2023
View reviewed changes

@danieljbruce danieljbruce left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass.
Comment thread tests/system/test_system.py
result = count_query.get()
assert len(result) == 1
for r in result[0]:
assert r.alias == "total"

@danieljbruce danieljbruce Jan 5, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Lines up with what I have here https://github.com/googleapis/python-firestore/pull/673/files.
Comment thread tests/system/test_system.py Outdated
assert len(result[0]) == 2

for r in result[0]:
assert r.alias in ["total", "all"]

@danieljbruce danieljbruce Jan 5, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more precise to do an assert check here that also verifies the order of "total" and "all" in r.alias? Perhaps two assert statements instead of using a loop?

@Mariatta Mariatta Jan 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my tests, the order of the result has been inconsistent. It doesn't always return it in the order of "total" and then "all".

I've now adjusted this test to assert that:

  • there are two elements returned in the result
  • the returned result has two values and they are both unique
  • the result contains both aliases: "total" and "all"
kolea2
kolea2 previously requested changes Jan 6, 2023
View reviewed changes
Comment thread tests/system/test_system.py
@Mariatta Mariatta force-pushed the feat-aggregate-query branch from 29864f6 to 674d242 Compare January 6, 2023 23:29
kolea2
kolea2 reviewed Jan 9, 2023
View reviewed changes
Comment thread google/cloud/firestore_v1/async_aggregation.py Outdated
kolea2
kolea2 reviewed Jan 9, 2023
View reviewed changes

@kolea2 kolea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm to me after header fix and PTAL at last comment.

Would like someone from yoshi-python to review as I've only looked from an API perspective. Thanks!
Comment thread google/cloud/firestore_v1/base_aggregation.py
nicain
nicain reviewed Jan 10, 2023
View reviewed changes
Comment thread google/cloud/firestore_v1/aggregation.py Outdated
Mariatta and others added 2 commits January 10, 2023 11:30
@Mariatta
Remove unneeded variable
fda8be5 
Refactor system test suite to fallback to default creds
@Mariatta
Merge branch 'main' into feat-aggregate-query
4443568
@Mariatta Mariatta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2023
@kolea2 kolea2 dismissed their stale review January 11, 2023 18:47

comments addressed. thanks!

@Mariatta Mariatta requested review from kolea2 and nicain January 12, 2023 17:33
kolea2
kolea2 approved these changes Jan 12, 2023
View reviewed changes

@kolea2 kolea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after @googleapis/yoshi-python takes a look as well. Thanks!
Comment thread tests/system/test_system.py
FIRESTORE_CREDS
)
project = FIRESTORE_PROJECT or credentials.project_id
else:

@kolea2 kolea2 Jan 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like someone from @googleapis/yoshi-python to take a look at this specifically

@meredithslota meredithslota Jan 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kolea2 did anyone from @googleapis/yoshi-python review this? I can't tell from the conversation.

@kolea2 kolea2 Jan 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe @nicain did!
@nicain nicain merged commit dfd4c5d into main Jan 12, 2023
@nicain nicain deleted the feat-aggregate-query branch January 12, 2023 20:27
@release-please release-please Bot mentioned this pull request Jan 12, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: firestore Issues related to the googleapis/python-firestore API. size: xl Pull request size is extra large.

6 participants

@Mariatta @nicain @meredithslota @danieljbruce @kolea2 @yoshi-kokoro