feat: Count queries (not available for use yet) by dconeybe · Pull Request #1033 · googleapis/java-firestore

dconeybe · 2022-09-14T15:45:29Z

Add support for "count" queries; that is, a query that gets the number of documents in the result set without actually downloading the documents.

This PR implements all of the logic but does not expose the API. A follow-up PR will change the visibility of the related classes and methods to public.

…ogleapis-gen at HEAD, 3445f74bc2b52dc4df2b33a97a6a92026019e4fa

…ds, which were deleted by the import from googleapis-gen but are needed.

…ackage This fixes the following warning from `mvn -B dependency:analyze -DfailOnWarning=true` ``` [WARNING] Used undeclared dependencies found: [WARNING] com.google.http-client:google-http-client:jar:1.42.2:compile [WARNING] Non-test scoped test only dependencies found: [WARNING] com.google.http-client:google-http-client:jar:1.42.2:compile ```

…ng` (was `Long`)

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

…eObserver

…of the future.

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

…ived from the backend.

ehsannas

The testing is very thorough. Love it.

There are a few things that need to be addressed.
1- comments for the class and methods of AggregateQuery
2- comments for the class and methods of AggregateQuerySnapshot
3- Moving the integration test to it/

I'd say land this PR as is and address my comments in the next PR.

ehsannas · 2022-09-21T19:03:52Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java

+
+    @Override
+    public void onResponse(RunAggregationQueryResponse response) {
+      if (!isFutureNotified.compareAndSet(false, true)) {


May be good to add a comment that we expect only one response and why. Please do as part of the next PR.

Done: #1041

ehsannas · 2022-09-21T19:09:24Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java

    return false;
  }

+  // TODO(count) Make this method public


Also need a comment for the method. Please do as part of the next PR.

dconeybe · 2022-09-22T00:19:13Z

The testing is very thorough. Love it.

There are a few things that need to be addressed. 1- comments for the class and methods of AggregateQuery 2- comments for the class and methods of AggregateQuerySnapshot 3- Moving the integration test to it/

I'd say land this PR as is and address my comments in the next PR.

Thanks for the review, @ehsannas!

Regarding your comments...

I have a separate task in TaskFlow to update all of the public documentation in all SDKs, so I'll make sure to update the documentation in this repo as well (b/245781713).
Same
Since the classes under test have default visibility (and not public visibility) I needed to temporarily move those integration tests into the same package as the classes they are testing. Once they're changed to "public" then I'll move them into their proper location, in the "it" subdirectory. I left a "TODO" comment in the code to do that.

dconeybe added 9 commits August 26, 2022 11:23

Update protos from preview branch of https://github.com/googleapis/go…

05f352d

…ogleapis-gen at HEAD, 3445f74bc2b52dc4df2b33a97a6a92026019e4fa

update clirr-ignored-differences.xml

396c22a

fix copyrights in files exported from googleapis-gen

70a61ad

Firestore[Stub]Settings.java: add back newHttpJsonBuilder() and frien…

1ae9901

…ds, which were deleted by the import from googleapis-gen but are needed.

format code by running mvn com.coveo:fmt-maven-plugin:format

741c0fa

Merge remote-tracking branch 'origin/main' into count

0ded2c0

feat: Query.count() implementation, part 1 (#1026)

80189c1

Merge remote-tracking branch 'origin/main' into count

066718c

feat: Transaction.get(AggregateQuery) added (#1029)

23a1557

dconeybe self-assigned this Sep 14, 2022

product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Sep 14, 2022

This comment was marked as outdated.

Sign in to view

Merge remote-tracking branch 'origin/main' into count

f3ce77a

dconeybe changed the title ~~Add Count Aggregation Query Support~~ feat: Count Aggregation Query Support added Sep 15, 2022

dconeybe changed the title ~~feat: Count Aggregation Query Support added~~ feat: Count Queries Sep 15, 2022

dconeybe changed the title ~~feat: Count Queries~~ feat: Count Queries (implementation only - API is not public yet) Sep 15, 2022

dconeybe changed the title ~~feat: Count Queries (implementation only - API is not public yet)~~ feat: Count Queries Sep 15, 2022

Fix integration tests to use the default FirestoreOptions

8e22716

dconeybe changed the title ~~feat: Count Queries~~ feat: Count queries implemented (not available for use yet) Sep 16, 2022

dconeybe changed the title ~~feat: Count queries implemented (not available for use yet)~~ feat: Count queries (not available for use yet) Sep 16, 2022

dconeybe added 3 commits September 16, 2022 20:33

Merge remote-tracking branch 'origin/main' into count

b7ec1a1

Reduce visibility of COUNT queries until it's ready to be released

b227ea9

dconeybe marked this pull request as ready for review September 17, 2022 04:53

dconeybe requested review from a team as code owners September 17, 2022 04:53

dconeybe and others added 3 commits September 17, 2022 01:12

AggregateQuerySnapshot.java: change return value of getCount() to `lo…

862a8c9

…ng` (was `Long`)

AggregateQuery.java: close the stream after receiving the first result

ddcaf21

🦉 Updates from OwlBot post-processor

77936cd

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

dconeybe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2022

dconeybe and others added 3 commits September 19, 2022 16:56

LocalFirestoreHelper.java: make sure to call onStart() on the Respons…

4b8d8f6

…eObserver

AggregateQuery.java: simplify the logic to avoid repeated completion …

c963f7b

…of the future.

🦉 Updates from OwlBot post-processor

16af34a

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

dconeybe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 19, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 19, 2022

dconeybe added 3 commits September 20, 2022 00:24

Convert ITAggregateQueryTest to a unit test

0d61d13

AggregateQuery.java: improve error messages if bad responses are rece…

8b8da7a

…ived from the backend.

QueryCountTest.java unit test added

e8a2f61

dconeybe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 20, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 20, 2022

Merge remote-tracking branch 'origin/main' into count

ef39a25

dconeybe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 21, 2022

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

ehsannas approved these changes Sep 21, 2022

View reviewed changes

dconeybe merged commit 8b60612 into main Sep 22, 2022

dconeybe deleted the count branch September 22, 2022 00:19

This was referenced Sep 22, 2022

feat: Make count queries publicly available for use #1042

Merged

docs: AggregateQuery.java: describe why we ignore subsequent responses #1041

Merged

release-please bot mentioned this pull request Oct 4, 2022

chore(main): release 3.6.0 #1054

Merged

dconeybe mentioned this pull request Oct 12, 2022

feat: Add retry logic to COUNT queries #1062

Merged

cherylEnkidu pushed a commit that referenced this pull request Dec 11, 2023

feat: Count queries (not available for use yet) (#1033)

bc17281

release-please bot mentioned this pull request May 17, 2024

chore(main): release 3.21.4 #1689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Count queries (not available for use yet)#1033

feat: Count queries (not available for use yet)#1033
dconeybe merged 24 commits intomainfrom
count

dconeybe commented Sep 14, 2022 •

edited

Loading

Uh oh!

This comment was marked as outdated.

ehsannas left a comment

Uh oh!

ehsannas Sep 21, 2022

Uh oh!

dconeybe Sep 22, 2022

Uh oh!

ehsannas Sep 21, 2022

Uh oh!

dconeybe commented Sep 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dconeybe commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

ehsannas left a comment

Choose a reason for hiding this comment

Uh oh!

ehsannas Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

dconeybe Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

ehsannas Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

dconeybe commented Sep 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dconeybe commented Sep 14, 2022 •

edited

Loading