feat: add backend query options by olavloite · Pull Request #90 · googleapis/java-spanner ·

Adds the ability to set QueryOptions when running Cloud Spanner queries. For now, only setting the query_optimizer_version is added.

QueryOptions can be configured through the following mechanisms:

Through the SPANNER_OPTIMIZER_VERSION environment variable.
At Spanner level using SpannerOptions.newBuilder().setDefaultQueryOptions(DatabaseId, QueryOptions).
At query level using Statement.newBuilder(String).withQueryOptions(QueryOptions).

If the options are configured through multiple mechanisms then:

Options set at an environment variable level will override options configured at the SpannerOptions level.
Options set at a query-level will override options set at either the SpannerOptions or environment variable level.

If no options are set, the optimizer version will default to:

The optimizer version the database is pinned to.
If the database is not pinned to a specific version, then the Cloud Spanner backend will use the "latest" version.

LGTM

A couple of requests:

I would like to verify that both ExecuteSql and ExecuteSql requests support query options. I think it does from what I understand of the implementation but thought I'd check.
If you could update the description of this PR to contain the information about the env variable and the precedence, that would be great. Example PR

skuruppu · 2020-03-09T00:36:54Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java

+MultiUseReadOnlyTransaction(Builder builder) {
+super(builder);
 checkArgument(
-bound.getMode() != TimestampBound.Mode.MAX_STALENESS
-&& bound.getMode() != TimestampBound.Mode.MIN_READ_TIMESTAMP,
-"Bounded staleness mode %s is not supported for multi-use read-only transactions."
-+ " Create a single-use read or read-only transaction instead.",
-bound.getMode());
-this.bound = bound;
-}
-
-MultiUseReadOnlyTransaction(
-SessionImpl session,
-ByteString transactionId,
-Timestamp timestamp,
-SpannerRpc rpc,
-int defaultPrefetchChunks) {
-super(session, rpc, defaultPrefetchChunks);
-this.transactionId = transactionId;
-this.timestamp = timestamp;
+!(builder.bound != null && builder.transactionId != null)
+&& !(builder.bound == null && builder.transactionId == null),
+"Either TimestampBound or TransactionId must be specified");
+if (builder.bound != null) {
+checkArgument(
+builder.bound.getMode() != TimestampBound.Mode.MAX_STALENESS
+&& builder.bound.getMode() != TimestampBound.Mode.MIN_READ_TIMESTAMP,
+"Bounded staleness mode %s is not supported for multi-use read-only transactions."
+ " Create a single-use read or read-only transaction instead.",
+builder.bound.getMode());
+this.bound = builder.bound;
+} else {
+this.timestamp = builder.timestamp;
+this.transactionId = builder.transactionId;
+}


Since most of this refactoring and extra checks for arguments is not related to the query options work, would it be ok to move this to a separate PR?

I would prefer not to. The reason for introducing this builder pattern in this PR is that this PR requires an additional object to be passed in to the different transaction classes. This would add another parameter to the constructors of these classes, bringing the total number to 6 in this specific case. As a general rule of thumb, a method (or constructor) in Java should not take more than 4 arguments: https://rules.sonarsource.com/java/RSPEC-107

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java

google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java

One thing I notice is that PartitionedDML also uses ExecuteSqlRequest, which needs to support QueryOptions as well.

final ExecuteSqlRequest.Builder builder =
ExecuteSqlRequest.newBuilder()
.setSql(statement.getSql())
.setQueryMode(QueryMode.NORMAL)
.setSession(session.getName())
.setTransaction(TransactionSelector.newBuilder().setId(transactionId).build());

LGTM
A couple of requests:
I would like to verify that both ExecuteSql and ExecuteSql requests support query options. I think it does from what I understand of the implementation but thought I'd check.
If you could update the description of this PR to contain the information about the env variable and the precedence, that would be great. Example PR

I've added update statements to ITQueryOptionsTest. All update statements are executed by the client library using the ExecuteSql RPC. (All queries use ExecuteSql). This verifies that QueryOptions work for both RPCs.
Done.

One thing I notice is that PartitionedDML also uses ExecuteSqlRequest, which needs to support QueryOptions as well.

I've added a PartitionedDML statement to ITQueryOptionsTest to verify that QueryOptions are supported (or actually; currently ignored) for DML and PartitionedDML statements.

@olavloite

@olavloite you are welcome to merge this in as I've now merged in the generated code.

Adds support for setting QueryOptions that will be used by the backend to execute queries.

QueryOptions should be an option on a Statement instead of a parameter to the executeQuery method. By setting these options on a Statement, it is possible to use it with analyzeQuery as well.

olavloite added the api: spannerIssues related to the googleapis/java-spanner API.label Mar 3, 2020

googlebot added the cla: yesThis human has signed the Contributor License Agreement.label Mar 3, 2020

olavloite requested a review from skuruppu March 3, 2020 17:40

skuruppu requested a review from hengfengli March 5, 2020 02:41

skuruppu approved these changes Mar 9, 2020
View reviewed changes

skuruppu force-pushed the autosynth branch 4 times, most recently from 9130062 to 59d2f8d Compare March 12, 2020 00:42

yoshi-automation force-pushed the autosynth branch from 59d2f8d to 2c7df30 Compare March 12, 2020 09:07

olavloite changed the base branch from autosynth to master March 12, 2020 12:38

olavloite added 4 commits March 12, 2020 13:39

feat: add backend query options

0b1a529
Adds support for setting QueryOptions that will be used by the backend to execute queries.

fix: set QueryOptions on Statement

160970b
QueryOptions should be an option on a Statement instead of a parameter to the executeQuery method. By setting these options on a Statement, it is possible to use it with analyzeQuery as well.

feat: add toBuilder() method to Statement

7e6b799

fix: code review comments

d27b3a7

olavloite force-pushed the query-options branch from 624f0e1 to d27b3a7 Compare March 12, 2020 12:53

fix: remove unused interface and class

096e3fb

olavloite merged commit e96e172 into master Mar 12, 2020

olavloite deleted the query-options branch March 12, 2020 15:35

Uh oh!

Uh oh!

Uh oh!

skuruppu Mar 9, 2020

Uh oh!

olavloite Mar 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skuruppu Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

olavloite Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!