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.

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: adds query optimizer statistics support #385

Merged
merged 9 commits into from Jun 5, 2021
Merged

feat: adds query optimizer statistics support #385

merged 9 commits into from Jun 5, 2021

Conversation

thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Aug 17, 2020 •
edited

Adds query optimizer statistics support. This will allow the user to set this option in 4 different levels:

  1. Through a statement hint (using DML).
  2. Through a query configuration (using Statement.Builder#withQueryOptions).
  3. Through an environment variable (SPANNER_OPTIMIZER_STATISTICS_PACKAGE).
  4. Through application configuration (using SpannerOptions and setting the default QueryOptions).

If packages are set in different levels, the precedence is from 1 to 4 (i.e. 1 is top priority).

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 17, 2020
thiagotnunes
thiagotnunes commented Aug 17, 2020
View reviewed changes
Copy link
Contributor Author

@thiagotnunes thiagotnunes left a comment •
edited

Choose a reason for hiding this comment

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

Note that the following functionality we get for free, by updating the proto files:

  1. Setting the (application level) default optimizer statistics package, through the SpannerOptions.Builder#setDefaultQueryOptions method.
  2. Setting the query level optimizer statistics package, through the Statement.Builder#setQueryOptions.
  3. QueryOptions precedence, already implemented on SpannerOptions constructor, AbstractReadContext#buildQueryOptions and StatementParser#mergeQueryOptions.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java Outdated Show resolved Hide resolved
...d-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutor.java Show resolved Hide resolved
...oud-spanner/src/main/resources/com/google/cloud/spanner/connection/ClientSideStatements.json Outdated Show resolved Hide resolved
...d-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutor.java Show resolved Hide resolved
...oud-spanner/src/main/resources/com/google/cloud/spanner/connection/ClientSideStatements.json Show resolved Hide resolved
...oud-spanner/src/main/resources/com/google/cloud/spanner/connection/ClientSideStatements.json Outdated Show resolved Hide resolved
thiagotnunes
thiagotnunes commented Aug 17, 2020
View reviewed changes
Copy link
Contributor Author

@thiagotnunes thiagotnunes left a comment •
edited

Choose a reason for hiding this comment

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

  • Check if the regex used to parse statistics package names is correct.
  • Check if we should add the capability of setting QueryOptions in the read* and write* methods.

olavloite
olavloite reviewed Aug 17, 2020
View reviewed changes
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

This looks generally good to me (with a couple of small questions). The PR also includes changes to the generated code. I assume that that will be moved to a separate PR?

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java Outdated Show resolved Hide resolved
.../src/test/resources/com/google/cloud/spanner/connection/ITSqlScriptTest_TestQueryOptions.sql Show resolved Hide resolved
proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/ExecuteSqlRequest.java Outdated Show resolved Hide resolved
@thiagotnunes
Copy link
Contributor Author

thiagotnunes commented Aug 17, 2020 •
edited

This looks generally good to me (with a couple of small questions). The PR also includes changes to the generated code. I assume that that will be moved to a separate PR?

Yes, I added them here, so that the tests would pass. Just to make sure we are talking about the same files, these are the ones that should not be included in the final PR: df26c0d

@thiagotnunes thiagotnunes added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 18, 2020
@thiagotnunes
Copy link
Contributor Author

We can only merge once we have the generated proto files in the main repository. We should then remove the generated code from this PR.

@olavloite olavloite self-requested a review August 19, 2020 09:42
olavloite
olavloite approved these changes Aug 19, 2020
View reviewed changes
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

LGTM, once the generated code can be removed from the PR.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Aug 21, 2020
@thiagotnunes thiagotnunes force-pushed the adds-query-optimizer-statistics-support branch from 69e8934 to d69f217 Compare June 5, 2021 05:28
@thiagotnunes thiagotnunes requested review from a team as code owners June 5, 2021 05:28
@generated-files-bot
Copy link

generated-files-bot bot commented Jun 5, 2021 •
edited

Warning: This pull request is touching the following templated files:

  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/ExecuteSqlRequest.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/SpannerProto.java

@google-cla
Copy link

google-cla bot commented Jun 5, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jun 5, 2021
@thiagotnunes thiagotnunes force-pushed the adds-query-optimizer-statistics-support branch from d69f217 to 69e8934 Compare June 5, 2021 05:34
@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 5, 2021
@thiagotnunes
feat: adds optimizer_statistics_package option
a12b81c 
Adds the possibility to set the optimizer statistics package when
executing queries. This option can be set in different levels as follows:
1. Through statement hints
2. Through query level configuration
3. Through an environment variable
4. Through application level configuration

If more than one package is set (in different levels) the precedence is
from 1 to 4 (1 has top priority).
@thiagotnunes
test: adds integration tests
c16a2dc 
Adds integration tests for setting the query options optimizer
statistics package.
@thiagotnunes
fix: addresses PR comments
2173166 
Fix missing value in the documentation of the optimizer statistics
package for the connection class.
@thiagotnunes
fix: adds tests for invalid stats packages
5a25b97 
Adds tests for invalid statistics packages (whitespace only ones).
@thiagotnunes
fix: adds integration tests for query options
6a48abe 
Adds an integration test to run the sql script with several expectations
for the query options. These are tests for the optimizer version and
optimizer statistics package.
@thiagotnunes
fix: formatting of ClientSideStatements.json
954dfc8 
This file is using a mix of tabs and spaces. For now I have formatted as
the other lines, so that the diff is clear. In a further PR I will
reformat the whole file to use only spaces.
@thiagotnunes thiagotnunes force-pushed the adds-query-optimizer-statistics-support branch from 69e8934 to 954dfc8 Compare June 5, 2021 05:36
@thiagotnunes
Merge branch 'master' of github.com:thiagotnunes/java-spanner into ad…
8cf5f10 
…ds-query-optimizer-statistics-support
@thiagotnunes
tests: fix Connection test
5d7350c 
Fixes connection test when using environment variables for retrieving
configurations. This only works when a first connection is created, so
we moved this specific test to its own subclass.
@thiagotnunes
fix: fix clirr checks
c3e365a 
Provides default interface implementations and fixes clirr checks
@thiagotnunes thiagotnunes removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 5, 2021
@thiagotnunes thiagotnunes merged commit e294532 into googleapis:master Jun 5, 2021
@thiagotnunes thiagotnunes deleted the adds-query-optimizer-statistics-support branch June 5, 2021 09:48
ansh0l pushed a commit to ansh0l/java-spanner that referenced this pull request Nov 10, 2022
@yoshi-automation
auto-regenerate .pb.go files (googleapis#385)
a5b850b 
This is an auto-generated regeneration of the .pb.go files by
cloud.google.com/go/internal/gapicgen. Once this PR is submitted, genmgr will
update the corresponding CL at gocloud to depend on the newer version of
go-genproto, and assign reviewers. Whilst this or any regen PR is open in
go-genproto, gapicgen will not create any more regeneration PRs or CLs. If all
regen PRs are closed, gapicgen will create a new set of regeneration PRs and
CLs once per night.

If you have been assigned to review this CL, please:

- Ensure that CI is passing. If it's failing, it requires your manual attention.
- Approve and submit this PR if you believe it's ready to ship. That will prompt
  genmgr to assign reviewers to the gocloud CL.

Corresponding gocloud CL: https://code-review.googlesource.com/c/gocloud/+/57330
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
@renovate-bot
chore(deps): update dependency com.google.cloud:libraries-bom to v19 (g…
cc0674d 
…oogleapis#385)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olavloite olavloite olavloite approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@thiagotnunes @olavloite