Skip to content
This repository was archived by the owner on Mar 4, 2026. It is now read-only.

feat: add support for QueryOptions#846

Merged
olavloite merged 7 commits intogoogleapis:masterfrom
olavloite:query-options
Mar 12, 2020
Merged

feat: add support for QueryOptions#846
olavloite merged 7 commits intogoogleapis:masterfrom
olavloite:query-options

Conversation

@olavloite
Copy link
Copy Markdown
Contributor

@olavloite olavloite commented Mar 4, 2020 •
edited
Loading

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
skuruppu
skuruppu approved these changes Mar 9, 2020
View reviewed changes
Copy link
Copy Markdown
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.

proto/spanner.d.ts Outdated

/** ExecuteSqlRequest resumeToken */
resumeToken?: (Uint8Array|null);
resumeToken?: (Uint8Array|string|null);
Copy link
Copy Markdown
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.

I assume these are auto-generated changes?

Copy link
Copy Markdown
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.

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
Copy Markdown
Contributor

@skuruppu skuruppu Mar 12, 2020

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
Copy Markdown
Contributor Author

olavloite commented Mar 10, 2020

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
Copy Markdown
Contributor

skuruppu commented Mar 11, 2020

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
Copy Markdown
Contributor Author

olavloite commented Mar 11, 2020

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.

skuruppu
skuruppu approved these changes Mar 12, 2020
View reviewed changes
Copy link
Copy Markdown
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
@skuruppu skuruppu mentioned this pull request Aug 27, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hengfengli hengfengli Awaiting requested review from hengfengli

1 more reviewer

@skuruppu skuruppu skuruppu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@olavloite @skuruppu @googlebot