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

feat: adds query optimizer statistics support #385

Conversation

thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Aug 17, 2020

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 label Aug 17, 2020
Copy link
Contributor Author

@thiagotnunes thiagotnunes left a comment

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.

Copy link
Contributor Author

@thiagotnunes thiagotnunes left a comment

  • 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.

Copy link
Contributor

@olavloite olavloite left a comment

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?

@thiagotnunes
Copy link
Contributor Author

@thiagotnunes thiagotnunes commented Aug 17, 2020

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 label Aug 18, 2020
@thiagotnunes
Copy link
Contributor Author

@thiagotnunes thiagotnunes commented Aug 18, 2020

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 Aug 19, 2020
Copy link
Contributor

@olavloite olavloite left a comment

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

@product-auto-label product-auto-label bot added the api: spanner label Aug 21, 2020
@thiagotnunes thiagotnunes force-pushed the adds-query-optimizer-statistics-support branch from 69e8934 to d69f217 Compare Jun 5, 2021
@thiagotnunes thiagotnunes requested review from as code owners Jun 5, 2021
@generated-files-bot
Copy link

@generated-files-bot generated-files-bot bot commented Jun 5, 2021

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 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 and removed cla: yes labels Jun 5, 2021
@thiagotnunes thiagotnunes force-pushed the adds-query-optimizer-statistics-support branch from d69f217 to 69e8934 Compare Jun 5, 2021
@google-cla google-cla bot added cla: yes and removed cla: no labels Jun 5, 2021
thiagotnunes added 6 commits Jun 5, 2021
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).
Adds integration tests for setting the query options optimizer
statistics package.
Fix missing value in the documentation of the optimizer statistics
package for the connection class.
Adds tests for invalid statistics packages (whitespace only ones).
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.
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 Jun 5, 2021
thiagotnunes added 3 commits Jun 5, 2021
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.
Provides default interface implementations and fixes clirr checks
@thiagotnunes thiagotnunes removed the do not merge label Jun 5, 2021
@thiagotnunes thiagotnunes merged commit e294532 into googleapis:master Jun 5, 2021
16 checks passed
@thiagotnunes thiagotnunes deleted the adds-query-optimizer-statistics-support branch Jun 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants