fix(spanner): fix Client::OverlayQueryOptions() to merge correctly #7118
Conversation
`Client::OverlayQueryOptions()` was misbehaving on two fronts:
- The environment option for `optimizer_statistics_package` was
using the value of `${SPANNER_OPTIMIZER_VERSION}` instead of
`${SPANNER_OPTIMIZER_STATISTICS_PACKAGE}`.
- It was not setting an output value for `request_priority` at all.
At least part of the reason for these errors was that the test:
- assumed the behavior could be changed by modifying the environment
whereas the implementation only read the environment once, and
- only a single `MockConnection`, with accumulated parameter matches,
was used across all test cases.
So, fix the implementation, and revamp the test to be more direct
(which eliminates the mocking) and to exercise RequestPriority.
Also add a note to `query_options.h` to update `OverlayQueryOptions()`
whenever new options are added.
Finally, add documentation for `${SPANNER_OPTIMIZER_STATISTICS_PACKAGE}`.
google-cla
bot
added
the
cla: yes
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7118 +/- ##
==========================================
- Coverage 94.49% 94.49% -0.01%
==========================================
Files 1309 1309
Lines 113039 113060 +21
==========================================
+ Hits 106820 106831 +11
- Misses 6219 6229 +10
Continue to review full report at Codecov.
|
google/cloud/spanner/client.h
Outdated
| @@ -643,6 +643,11 @@ class Client { | |||
| SqlStatement statement, QueryOptions const& opts = {}); | |||
|
|
|||
| private: | |||
| friend class OverlayQueryOptionsTester; | |||
| static QueryOptions OverlayQueryOptions( | |||
There was a problem hiding this comment.
Alternatively, put this static function in the spanner_internal namespace and you don't need the friend helper, your call.
google/cloud/spanner/query_options.h
Outdated
| @@ -30,6 +30,9 @@ inline namespace SPANNER_CLIENT_NS { | |||
| * These QueryOptions allow users to configure features about how their SQL | |||
| * queries executes on the server. | |||
| * | |||
| * Note: If you add an attribute here, remember to update the implementation | |||
There was a problem hiding this comment.
Ugh... that will make it to the public documentation too... Can you add the comment to a .cc file or to the beginning of the private: section? If not, can you add it after the Doxygen block as non-Doxygen comment?
There was a problem hiding this comment.
There is no .cc for this interface, so private: section it is.
google/cloud/spanner/client_test.cc
Outdated
| {{}, "client", "function", "function"}, | ||
| {"env", "client", "function", "function"}, | ||
| }; | ||
| constexpr auto OverlayQueryOptions = // NOLINT(readability-identifier-naming) |
There was a problem hiding this comment.
I am sure you understand the tradeoffs between testing the function that computes the query options vs. verifying the correct query options are sent to the SpannerConnection. This note is just a nudge to consider these tradeoffs one last time before you pull the trigger. I am good either way.
There was a problem hiding this comment.
There is a separate test, ConnectionImplTest.QueryOptions, for verifying how a final QueryOptions value is propagated into the protos. It is deficient in the request_priority department however, so I'll fix that in another PR.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Client::OverlayQueryOptions()was misbehaving on two fronts:optimizer_statistics_packagewasusing the value of
${SPANNER_OPTIMIZER_VERSION}instead of${SPANNER_OPTIMIZER_STATISTICS_PACKAGE}.request_priorityat all.At least part of the reason for these errors was that the test:
whereas the implementation only read the environment once, and
MockConnection, with accumulated parameter matches,was used across all test cases.
So, fix the implementation, and revamp the test to be more direct
(which eliminates the mocking) and to exercise RequestPriority.
Also add a note to
query_options.hto updateOverlayQueryOptions()whenever new options are added.
Finally, add documentation for
${SPANNER_OPTIMIZER_STATISTICS_PACKAGE}.This change is