Skip to content
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

feat: Query Profiling #1609

Merged
merged 42 commits into from
Mar 26, 2024
Merged

feat: Query Profiling #1609

merged 42 commits into from
Mar 26, 2024

Conversation

ehsannas
Copy link
Contributor

No description provided.

ehsannas and others added 30 commits October 17, 2023 15:40
@ehsannas
Update protos
7978c13
@ehsannas
Query profiling APIs.
093c230
@ehsannas
Add explain() and explainAnalyze() implementation for Query.
509891b
@ehsannas
WIP: Add explain() and explainAnalyze() for aggregate queries.
e1a2cd4
@ehsannas
Fix aggregate query profiling.
9527010
@ehsannas
format.
6fcc3fe
@ehsannas
Update the test.
83e68b3
@ehsannas
Don't use .* imports.
2638b3e
@ehsannas
decodeStruct should take Nullable value.
803e17e
@ehsannas
Address comments.
8b09a3e
@ehsannas
Format.
6c18db9
@ehsannas
introduce QueryPlan class to make it possible to add node-level info …
ec1ec7f 
…in the future.
@ehsannas
don't make QueryPlan c'tor public.
d38d889
@ehsannas
Remove print statement.
8c2de21
@ehsannas
undo manual proto changes.
67bfe83
@ehsannas
Merge remote-tracking branch 'origin/main' into query_profiling_5_new
2b0b099
@ehsannas
Address feedback.
61bb62a
@ehsannas
address feedback.
8f584ff
@gcf-owl-bot
🦉 Updates from OwlBot post-processor
0b81cfd 
See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md
@ehsannas
address feedback.
32b6003
@ehsannas
v2.
c15d575
@ehsannas
Add ExplainMetrics. Rename Plan to PlanSummary.
803c7ff
@ehsannas
Update tests.
8617ea3
@ehsannas
Fixes for Query and AggregateQuery explain methods.
c547b4b
@ehsannas
fix up the implementation based on the new protos.
f9220c7
@gcf-owl-bot @ehsannas
🦉 Updates from OwlBot post-processor
9c87e97 
See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md
@ehsannas
Fix: explain a query with empty result set.
544d1c0
@gcf-owl-bot @ehsannas
🦉 Updates from OwlBot post-processor
2373b4e 
See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md
@ehsannas
Add check for the regular query flow as well.
abf9c72
@ehsannas
Add explainStream API.
029017d
@ehsannas ehsannas assigned tom-andersen and unassigned ehsannas Mar 18, 2024
@ehsannas ehsannas requested a review from a team as a code owner March 18, 2024 07:20
tom-andersen
tom-andersen requested changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

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

There are a few points where the observer pattern implementation breaks down in my opinion. Otherwise, looks good!

google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/ExplainMetrics.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/PlanSummary.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Outdated Show resolved Hide resolved
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Mar 21, 2024
ehsannas
ehsannas commented Mar 21, 2024
View reviewed changes
google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/PlanSummary.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/ExplainMetrics.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/ExplainMetrics.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Outdated Show resolved Hide resolved
@ehsannas ehsannas assigned tom-andersen and unassigned ehsannas Mar 21, 2024
@ehsannas ehsannas added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2024
@ehsannas ehsannas added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 25, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 25, 2024
tom-andersen
tom-andersen requested changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

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

I will need some more time, but here is a preliminary review.

google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Outdated Show resolved Hide resolved
tom-andersen
tom-andersen approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

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

Looks good. Consider anything remaining as nits.

Beyond this PR, this library needs better logging. I would like to see that every usage of SettableApiFuture also include conditional logging if set or setException returns false. These represent unexpected situations that we are simply ignoring right now.

google-cloud-firestore/src/main/java/com/google/cloud/firestore/AggregateQuery.java Show resolved Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/ExplainMetrics.java Outdated Show resolved Hide resolved
@ehsannas ehsannas merged commit 55b1a49 into main Mar 26, 2024
24 checks passed
@ehsannas ehsannas deleted the ehsann/query_profiling_impl_v2_with_new_protos_6 branch March 26, 2024 16:40
@release-please release-please bot mentioned this pull request Mar 26, 2024
Merged
@release-please release-please bot mentioned this pull request May 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-andersen tom-andersen tom-andersen approved these changes

Assignees

@tom-andersen tom-andersen

Labels
api: firestore Issues related to the googleapis/java-firestore API. size: xl Pull request size is extra large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ehsannas @tom-andersen @yoshi-kokoro