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: add backend query options #90

Merged
merged 5 commits into from Mar 12, 2020
Merged

feat: add backend query options #90

merged 5 commits into from Mar 12, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Mar 3, 2020

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:

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

If the options are configured through multiple mechanisms then:

  1. Options set at an environment variable level will override options configured at the SpannerOptions level.
  2. 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:

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

@olavloite olavloite added the api: spanner label Mar 3, 2020
@googlebot googlebot added the cla: yes label Mar 3, 2020
@olavloite olavloite requested a review from skuruppu Mar 3, 2020
@skuruppu skuruppu requested a review from hengfengli Mar 5, 2020
Copy link
Contributor

@skuruppu skuruppu left a comment

LGTM

A couple of requests:

  1. I would like to verify that both ExecuteSql and ExecuteStreamingSql requests support query options. I think it does from what I understand of the implementation but thought I'd check.
  2. 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

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;
}
Copy link
Contributor

@skuruppu skuruppu Mar 9, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@olavloite olavloite Mar 10, 2020

Choose a reason for hiding this comment

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

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

@hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Mar 9, 2020

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());

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Mar 10, 2020

LGTM

A couple of requests:

  1. I would like to verify that both ExecuteSql and ExecuteStreamingSql requests support query options. I think it does from what I understand of the implementation but thought I'd check.
  2. 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
  1. I've added update statements to ITQueryOptionsTest. All update statements are executed by the client library using the ExecuteSql RPC. (All queries use ExecuteStreamingSql). This verifies that QueryOptions work for both RPCs.
  2. Done.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Mar 10, 2020

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.

@skuruppu skuruppu force-pushed the autosynth branch 4 times, most recently from 9130062 to 59d2f8d Compare Mar 12, 2020
@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Mar 12, 2020

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

@olavloite olavloite changed the base branch from autosynth to master Mar 12, 2020
olavloite added 4 commits Mar 12, 2020
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 olavloite merged commit e96e172 into master Mar 12, 2020
13 checks passed
@olavloite olavloite deleted the query-options branch Mar 12, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 13, 2020
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

4 participants