feat: implement query options versioning support - #30
Conversation
| self.assertEqual(client._query_options, expected_query_options) | ||
|
|
||
| @mock.patch("google.cloud.spanner_v1.client.os.getenv") | ||
| @mock.patch("google.cloud.spanner_v1.client._get_spanner_emulator_host") |
There was a problem hiding this comment.
Not sure why there's a diff for _get_spanner_emulator_host in this PR.
There was a problem hiding this comment.
Both emulator support and query options support rely on calling os.getenv which means it's more difficult to set up the mock. The os.getenv call for emulator is made in _get_spanner_emulator_host which means mocking that function instead is much more clear and avoids setting up theos.getenv mock.
| "without a scheme: ex %s=localhost:8080." | ||
| ) % ((EMULATOR_ENV_VAR,) * 3) | ||
| SPANNER_ADMIN_SCOPE = "https://www.googleapis.com/auth/spanner.admin" | ||
| OPTIMIZER_VERSION_VAR = "SPANNER_OPTIMIZER_VERSION" |
There was a problem hiding this comment.
probably use OPTIMIZER_VERSION_ENV_VAR
hengfengli
left a comment
There was a problem hiding this comment.
Major concerns:
- Does
query_optionssupport bothExecuteSqlRequest.QueryOptionsanddicteverywhere? It needs to be clear in the doc comment. - Does
partition_querysupportquery_optionsas well? - Nits: the merge function can be more generic for future extension. This can be done in the future when we add new fields in
query_options.
For the rest, LGTM, 👍
skuruppu
left a comment
There was a problem hiding this comment.
Just to confirm that we're going with SPANNER_OPTIMIZER_VERSION to be consistent with the query hint name. I think your implementation already uses this name but just wanted to make sure.
|
@hengfengli |
Actually, I mean this method |
49a422f to
4e16212
Compare
Adds support for QueryOptions.
The QueryOptions are set in one of three ways (in ascending order of precedence):
If the backend implements query hints, for example, to instruct the backend which query optimizer version to use, these query hints will also automatically be supported.