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 support for QueryOptions #846

Merged
merged 7 commits into from
Mar 12, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Mar 4, 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 Database level using spanner.instance('instance-name').database('database-name', sessionPoolOptions, queryOptions).
  3. At query level using ExecuteSqlRequest.queryOptions.

If the options are configured through multiple mechanisms then:

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

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

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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 4, 2020
@olavloite olavloite requested a review from skuruppu March 4, 2020 19:08
@skuruppu skuruppu requested a review from hengfengli March 5, 2020 02:28
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

If you can update the PR description to:

  1. List the precedence
  2. Mention the env var name
    that would be great.

@@ -1146,16 +1146,19 @@ export namespace google {
paramTypes?: ({ [k: string]: google.spanner.v1.IType }|null);

/** ExecuteSqlRequest resumeToken */
resumeToken?: (Uint8Array|null);
resumeToken?: (Uint8Array|string|null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these are auto-generated changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, although it could be that this change is only 'generated' because of the way I manually generated the proto changes. It should however not make any difference for this PR once the actual proto change has been merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the generated code change is on master so I think you should be able to revert this file.

@olavloite
Copy link
Contributor Author

LGTM

If you can update the PR description to:

  1. List the precedence
  2. Mention the env var name
    that would be great.

Done.

@skuruppu
Copy link
Contributor

Thanks for the changes. One more request @olavloite, would you be able to upload any samples you have? I'm not sure whether it should be in this PR or in a separate one but I'll let you decide.

@olavloite
Copy link
Contributor Author

Thanks for the changes. One more request @olavloite, would you be able to upload any samples you have? I'm not sure whether it should be in this PR or in a separate one but I'll let you decide.

I've added the samples to this PR.

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.

Thanks @olavloite. Please merge in the change after you've resolved the merge conflicts.

@olavloite olavloite merged commit c1098c5 into googleapis:master Mar 12, 2020
@olavloite olavloite deleted the query-options branch March 12, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants