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
Collaborator

@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 Issues related to the googleapis/java-spanner API. label Mar 3, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 3, 2020
@olavloite olavloite requested a review from skuruppu March 3, 2020 17:40
@skuruppu skuruppu requested a review from hengfengli March 5, 2020 02:41
Copy link
Contributor

@skuruppu skuruppu left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +250 to +267
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

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
Collaborator Author

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

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
Collaborator Author

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
Collaborator Author

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 March 12, 2020 00:42
@skuruppu
Copy link
Contributor

@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 March 12, 2020 12:38
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
@olavloite olavloite deleted the query-options branch March 12, 2020 15:35
gcf-merge-on-green bot pushed a commit that referenced this pull request Mar 13, 2020
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
This PR was generated using Autosynth. 🌈


<details><summary>Log from Synthtool</summary>

```
2020-03-17 12:21:53,248 synthtool > Executing /tmpfs/src/git/autosynth/working_repo/synth.py.
.github/ISSUE_TEMPLATE/bug_report.md
.github/ISSUE_TEMPLATE/feature_request.md
.github/ISSUE_TEMPLATE/support_request.md
.github/PULL_REQUEST_TEMPLATE.md
.github/release-please.yml
.github/trusted-contribution.yml
.kokoro/build.bat
.kokoro/build.sh
.kokoro/coerce_logs.sh
.kokoro/common.cfg
.kokoro/continuous/common.cfg
.kokoro/continuous/dependencies.cfg
.kokoro/continuous/integration.cfg
.kokoro/continuous/java11.cfg
.kokoro/continuous/java7.cfg
.kokoro/continuous/java8-osx.cfg
.kokoro/continuous/java8-win.cfg
.kokoro/continuous/java8.cfg
.kokoro/continuous/lint.cfg
.kokoro/continuous/propose_release.cfg
.kokoro/continuous/samples.cfg
.kokoro/dependencies.sh
.kokoro/linkage-monitor.sh
.kokoro/nightly/common.cfg
.kokoro/nightly/dependencies.cfg
.kokoro/nightly/integration.cfg
.kokoro/nightly/java11.cfg
.kokoro/nightly/java7.cfg
.kokoro/nightly/java8-osx.cfg
.kokoro/nightly/java8-win.cfg
.kokoro/nightly/java8.cfg
.kokoro/nightly/lint.cfg
.kokoro/nightly/samples.cfg
.kokoro/presubmit/clirr.cfg
.kokoro/presubmit/common.cfg
.kokoro/presubmit/dependencies.cfg
.kokoro/presubmit/integration.cfg
.kokoro/presubmit/java11.cfg
.kokoro/presubmit/java7.cfg
.kokoro/presubmit/java8-osx.cfg
.kokoro/presubmit/java8-win.cfg
.kokoro/presubmit/java8.cfg
.kokoro/presubmit/linkage-monitor.cfg
.kokoro/presubmit/lint.cfg
.kokoro/presubmit/samples.cfg
.kokoro/release/bump_snapshot.cfg
.kokoro/release/common.cfg
.kokoro/release/common.sh
.kokoro/release/drop.cfg
.kokoro/release/drop.sh
.kokoro/release/promote.cfg
.kokoro/release/promote.sh
.kokoro/release/publish_javadoc.cfg
.kokoro/release/publish_javadoc.sh
.kokoro/release/snapshot.cfg
.kokoro/release/snapshot.sh
.kokoro/release/stage.cfg
.kokoro/release/stage.sh
.kokoro/trampoline.sh
CODE_OF_CONDUCT.md
CONTRIBUTING.md
LICENSE
README.md
codecov.yaml
java.header
license-checks.xml
renovate.json
samples/install-without-bom/pom.xml
samples/pom.xml
samples/snapshot/pom.xml
samples/snippets/pom.xml
2020-03-17 12:21:53,455 synthtool > Wrote metadata to synth.metadata.

```
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants