Skip to content

DO NOT MERGE: Prototype COUNT using the RunAggregationQuery RPC #3784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 16 commits into from
Closed

DO NOT MERGE: Prototype COUNT using the RunAggregationQuery RPC #3784

wants to merge 16 commits into from

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jun 4, 2022

Note: This new RunAggregationQuery RPC is only available in the Firestore emulator (i.e. it's not available in prod). And even the emulator's implementation is experimental. This PR is only for a proof of concept and should not be merged.

@dconeybe dconeybe self-assigned this Jun 4, 2022
@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-firestore_ktx:
error: Added method com.google.firebase.firestore.ktx.FirestoreKt.getField(com.google.firebase.firestore.AggregateSnapshot,com.google.firebase.firestore.AggregateField) [AddedMethod]
error: Added method com.google.firebase.firestore.ktx.FirestoreKt.getField(error.NonExistentClass,String) [AddedMethod]
error: Added method com.google.firebase.firestore.ktx.FirestoreKt.getField(error.NonExistentClass,com.google.firebase.firestore.FieldPath) [AddedMethod]
error: Added method com.google.firebase.firestore.ktx.FirestoreKt.getField(com.google.firebase.firestore.AggregateSnapshot,com.google.firebase.firestore.AggregateField,com.google.firebase.firestore.DocumentSnapshot.ServerTimestampBehavior) [AddedMethod]
error: Added method com.google.firebase.firestore.ktx.FirestoreKt.getField(error.NonExistentClass,String,com.google.firebase.firestore.DocumentSnapshot.ServerTimestampBehavior) [AddedMethod]
error: Added method com.google.firebase.firestore.ktx.FirestoreKt.getField(error.NonExistentClass,com.google.firebase.firestore.FieldPath,com.google.firebase.firestore.DocumentSnapshot.ServerTimestampBehavior) [AddedMethod]

The public api surface has changed for the subproject firebase-firestore:
error: Added class com.google.firebase.firestore.AggregateField [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuery [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuerySnapshot [AddedClass]
error: Added class com.google.firebase.firestore.AggregateSnapshot [AddedClass]
error: Added class com.google.firebase.firestore.AggregateSource [AddedClass]
error: Added method com.google.firebase.firestore.Query.count() [AddedMethod]

The public api surface has changed for the subproject firebase-firestore_ktx_api.txt:

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-firestore:
error: Added class com.google.firebase.firestore.AggregateField [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuery [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuerySnapshot [AddedClass]
error: Added class com.google.firebase.firestore.AggregateSnapshot [AddedClass]
error: Added class com.google.firebase.firestore.AggregateSource [AddedClass]
error: Added method com.google.firebase.firestore.Query.count() [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 4, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-firestore

    Overall coverage changed from 46.21% (37f0fe7) to 44.64% (03c42d9) by -1.57%.

    18 individual files with coverage change

    FilenameBase (37f0fe7)Merge (03c42d9)Diff
    AggregateField.java?0.00%?
    AggregateQuery.java?0.00%?
    AggregateQuerySnapshot.java?0.00%?
    AggregationResult.java?0.00%?
    AggregationResultOrBuilder.java?0.00%?
    AggregationResultProto.java?0.00%?
    Datastore.java32.14%23.28%-8.87%
    FirestoreClient.java38.24%37.96%-0.28%
    FirestoreGrpc.java2.62%2.46%-0.15%
    PatchMutation.java100.00%98.39%-1.61%
    Query.java3.65%3.64%-0.01%
    RunAggregationQueryRequest.java?0.00%?
    RunAggregationQueryRequestOrBuilder.java?0.00%?
    RunAggregationQueryResponse.java?0.00%?
    RunAggregationQueryResponseOrBuilder.java?0.00%?
    SetMutation.java97.22%94.44%-2.78%
    StructuredAggregationQuery.java?0.00%?
    StructuredAggregationQueryOrBuilder.java?0.00%?

Test Logs

Notes

  • Commit (03c42d9) is created by Prow via merging PR base commit (37f0fe7) and head commit (df05424).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/Ay03LND6ml.html

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-firestore:
error: Added class com.google.firebase.firestore.AggregateField [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuery [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuerySnapshot [AddedClass]
error: Added method com.google.firebase.firestore.Query.count() [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

1 similar comment
@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-firestore:
error: Added class com.google.firebase.firestore.AggregateField [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuery [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuerySnapshot [AddedClass]
error: Added method com.google.firebase.firestore.Query.count() [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 4, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-firestore

    TypeBase (37f0fe7)Merge (03c42d9)Diff
    aar1.26 MB1.30 MB+46.1 kB (+3.7%)
    apk (aggressive)489 kB491 kB+2.00 kB (+0.4%)
    apk (release)3.39 MB3.40 MB+7.19 kB (+0.2%)

Test Logs

Notes

  • Commit (03c42d9) is created by Prow via merging PR base commit (37f0fe7) and head commit (df05424).

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/KZWwG39Uvj.html

dconeybe added 16 commits June 17, 2022 14:58
@dconeybe
Update protos to googleapis HEAD at googleapis/googleapis@13f9b89
20a7e10
@dconeybe
write.proto: add back string verify = 5
92c5218
@dconeybe
Update protos to googleapis HEAD of the preview branch at googleapis/…
5130d16 
…googleapis@77a0a97
@dconeybe
write.proto: add back string verify = 5
9c3a336
@dconeybe
Add API skeleton to aggregate queries and COUNT
74e6d14
@dconeybe
Datastore.java: runCountQuery() added
4155ac6
@dconeybe
wire up count queries
6b34e44
@dconeybe
delete group-by stuff, since it's not needed for this experiment
8dded2c
@dconeybe
CountTest.java added
12b5cd1
@dconeybe
Revert changes to Firestore.kt, since this prototype only uses java
77beb18
@dconeybe
AggregateField.java: remove everything except COUNT
bcf0bcf
@dconeybe
AggregateSnapshot.java deleted, since it's not used in this prototype
9aef43a
@dconeybe
revert a bunch of other unnecssary changes
01420e4
@dconeybe
Collapse CountQuery.java into AggregateQuery.java for simplicity
965144b
@dconeybe
/gradlew :firebase-firestore:googleJavaFormat
4aa3ceb
@dconeybe
AggregateField.java: remove CountAggregateField.upTo, since it's neve…
df05424 
…r used in this prototype
@dconeybe dconeybe force-pushed the dconeybe/CountFromDFE branch from e95066c to df05424 Compare June 17, 2022 18:58
@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-firestore:
error: Added class com.google.firebase.firestore.AggregateField [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuery [AddedClass]
error: Added class com.google.firebase.firestore.AggregateQuerySnapshot [AddedClass]
error: Added method com.google.firebase.firestore.Query.count() [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

@dconeybe: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
api-information df05424 link /test api-information
device-check-changed df05424 link /test device-check-changed

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dconeybe
Copy link
Contributor Author

This feature was implemented in another PR: #3847. I'm closing this one.

@dconeybe dconeybe closed this Sep 22, 2022
@dconeybe dconeybe deleted the dconeybe/CountFromDFE branch September 22, 2022 05:26
@firebase firebase locked and limited conversation to collaborators Oct 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@dconeybe dconeybe

Labels
api: firestore do-not-merge/work-in-progress size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dconeybe @google-oss-bot