From 822832602774fba8adc8cf8533be14e911ff7f3e Mon Sep 17 00:00:00 2001 From: Bradley White <14679271+devbww@users.noreply.github.com> Date: Thu, 5 Aug 2021 04:19:13 -0400 Subject: [PATCH 1/2] fix(spanner): fix Client::OverlayQueryOptions() to merge correctly `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/cloud/spanner/client.cc | 54 +++++--- google/cloud/spanner/client.h | 5 + google/cloud/spanner/client_test.cc | 149 +++++++++++++--------- google/cloud/spanner/doc/spanner-main.dox | 3 + google/cloud/spanner/query_options.h | 3 + 5 files changed, 139 insertions(+), 75 deletions(-) diff --git a/google/cloud/spanner/client.cc b/google/cloud/spanner/client.cc index 70ecd48305a1b..988e017cb71ba 100644 --- a/google/cloud/spanner/client.cc +++ b/google/cloud/spanner/client.cc @@ -301,20 +301,12 @@ StatusOr Client::ExecutePartitionedDml( {std::move(statement), OverlayQueryOptions(opts)}); } -// Returns a QueryOptions struct that has each field set according to the -// hierarchy that options specified as to the function call (i.e., `preferred`) -// are preferred, followed by options set at the Client level, followed by an -// environment variable. If none are set, the field's optional will be unset -// and nothing will be included in the proto sent to Spanner, in which case, -// the Database default will be used. -QueryOptions Client::OverlayQueryOptions(QueryOptions const& preferred) { - // GetEnv() is not super fast, so we look it up once and cache it. - static auto const* const kOptimizerVersionEnvValue = - new auto(google::cloud::internal::GetEnv("SPANNER_OPTIMIZER_VERSION")); - static auto const* const kOptimizerStatisticsPackageEnvValue = new auto( - google::cloud::internal::GetEnv("SPANNER_OPTIMIZER_STATISTICS_PACKAGE")); - - QueryOptions const& fallback = opts_.query_options(); +// Returns a QueryOptions that has each attribute set according to the +// preferred/fallback/environment hierarchy. +QueryOptions Client::OverlayQueryOptions( + QueryOptions const& preferred, QueryOptions const& fallback, + absl::optional const& optimizer_version_env, + absl::optional const& optimizer_statistics_package_env) { QueryOptions opts; // Choose the `optimizer_version` option. @@ -322,8 +314,8 @@ QueryOptions Client::OverlayQueryOptions(QueryOptions const& preferred) { opts.set_optimizer_version(preferred.optimizer_version()); } else if (fallback.optimizer_version().has_value()) { opts.set_optimizer_version(fallback.optimizer_version()); - } else if (kOptimizerVersionEnvValue->has_value()) { - opts.set_optimizer_version(*kOptimizerVersionEnvValue); + } else if (optimizer_version_env.has_value()) { + opts.set_optimizer_version(*optimizer_version_env); } // Choose the `optimizer_statistics_package` option. @@ -333,13 +325,39 @@ QueryOptions Client::OverlayQueryOptions(QueryOptions const& preferred) { } else if (fallback.optimizer_statistics_package().has_value()) { opts.set_optimizer_statistics_package( fallback.optimizer_statistics_package()); - } else if (kOptimizerStatisticsPackageEnvValue->has_value()) { - opts.set_optimizer_statistics_package(*kOptimizerVersionEnvValue); + } else if (optimizer_statistics_package_env.has_value()) { + opts.set_optimizer_statistics_package(*optimizer_statistics_package_env); + } + + // Choose the `request_priority` option. + if (preferred.request_priority().has_value()) { + opts.set_request_priority(preferred.request_priority()); + } else if (fallback.request_priority().has_value()) { + opts.set_request_priority(fallback.request_priority()); } return opts; } +// Returns a QueryOptions that has each attribute set according to +// the hierarchy that options specified at the function call (i.e., +// `preferred`) are preferred, followed by options set at the Client +// level (i.e., `opts_.query_options()`), followed by some environment +// variables. If none are set, the attribute's optional will be unset +// and nothing will be included in the proto sent to Spanner, in which +// case the Database default will be used. +QueryOptions Client::OverlayQueryOptions(QueryOptions const& preferred) { + // GetEnv() is not super fast, so we look it up once and cache it. + static auto const* const kOptimizerVersionEnvValue = + new auto(google::cloud::internal::GetEnv("SPANNER_OPTIMIZER_VERSION")); + static auto const* const kOptimizerStatisticsPackageEnvValue = new auto( + google::cloud::internal::GetEnv("SPANNER_OPTIMIZER_STATISTICS_PACKAGE")); + + return OverlayQueryOptions(preferred, opts_.query_options(), + *kOptimizerVersionEnvValue, + *kOptimizerStatisticsPackageEnvValue); +} + std::shared_ptr MakeConnection(spanner::Database const& db, Options opts) { internal::CheckExpectedOptions< diff --git a/google/cloud/spanner/client.h b/google/cloud/spanner/client.h index 53c533beed36f..158e36a8d1ff2 100644 --- a/google/cloud/spanner/client.h +++ b/google/cloud/spanner/client.h @@ -643,6 +643,11 @@ class Client { SqlStatement statement, QueryOptions const& opts = {}); private: + friend class OverlayQueryOptionsTester; + static QueryOptions OverlayQueryOptions( + QueryOptions const& preferred, QueryOptions const& fallback, + absl::optional const& optimizer_version_env, + absl::optional const& optimizer_statistics_package_env); QueryOptions OverlayQueryOptions(QueryOptions const&); std::shared_ptr conn_; diff --git a/google/cloud/spanner/client_test.cc b/google/cloud/spanner/client_test.cc index f227a07c88e4a..f6de08f15b662 100644 --- a/google/cloud/spanner/client_test.cc +++ b/google/cloud/spanner/client_test.cc @@ -19,9 +19,7 @@ #include "google/cloud/spanner/results.h" #include "google/cloud/spanner/timestamp.h" #include "google/cloud/spanner/value.h" -#include "google/cloud/internal/setenv.h" #include "google/cloud/testing_util/is_proto_equal.h" -#include "google/cloud/testing_util/scoped_environment.h" #include "google/cloud/testing_util/status_matchers.h" #include "absl/memory/memory.h" #include "absl/types/optional.h" @@ -36,6 +34,19 @@ namespace google { namespace cloud { namespace spanner { inline namespace SPANNER_CLIENT_NS { + +class OverlayQueryOptionsTester { + public: + static QueryOptions OverlayQueryOptions( + QueryOptions const& preferred, QueryOptions const& fallback, + absl::optional const& optimizer_version_env, + absl::optional const& optimizer_statistics_package_env) { + return Client::OverlayQueryOptions(preferred, fallback, + optimizer_version_env, + optimizer_statistics_package_env); + } +}; + namespace { namespace spanner_proto = ::google::spanner::v1; @@ -45,12 +56,9 @@ using ::google::cloud::spanner_mocks::MockResultSetSource; using ::google::cloud::testing_util::IsProtoEqual; using ::google::cloud::testing_util::StatusIs; using ::google::protobuf::TextFormat; -using ::testing::AnyNumber; using ::testing::ByMove; using ::testing::DoAll; using ::testing::ElementsAre; -using ::testing::Eq; -using ::testing::Field; using ::testing::HasSubstr; using ::testing::Return; using ::testing::SaveArg; @@ -1026,59 +1034,86 @@ TEST(ClientTest, ProfileQueryWithOptionsSuccess) { } TEST(ClientTest, QueryOptionsOverlayPrecedence) { - struct Levels { - absl::optional env; - absl::optional client; - absl::optional function; - absl::optional expected; - }; - std::vector levels = { - {{}, {}, {}, {}}, - {"env", {}, {}, "env"}, - {{}, "client", {}, "client"}, - {{}, {}, "function", "function"}, - {"env", "client", {}, "client"}, - {"env", {}, "function", "function"}, - {{}, "client", "function", "function"}, - {"env", "client", "function", "function"}, - }; + constexpr auto OverlayQueryOptions = // NOLINT(readability-identifier-naming) + OverlayQueryOptionsTester::OverlayQueryOptions; + + // Check optimizer_version. + { + QueryOptions preferred; + preferred.set_optimizer_version("preferred"); + QueryOptions fallback; + fallback.set_optimizer_version("fallback"); + absl::optional optimizer_version_env = "environment"; + EXPECT_EQ(OverlayQueryOptions(preferred, fallback, optimizer_version_env, + absl::nullopt) + .optimizer_version(), + "preferred"); + preferred.set_optimizer_version(absl::nullopt); + EXPECT_EQ(OverlayQueryOptions(preferred, fallback, optimizer_version_env, + absl::nullopt) + .optimizer_version(), + "fallback"); + fallback.set_optimizer_version(absl::nullopt); + EXPECT_EQ(OverlayQueryOptions(preferred, fallback, optimizer_version_env, + absl::nullopt) + .optimizer_version(), + "environment"); + optimizer_version_env = absl::nullopt; + EXPECT_EQ(OverlayQueryOptions(preferred, fallback, optimizer_version_env, + absl::nullopt) + .optimizer_version(), + absl::nullopt); + } - auto constexpr kQueryOptionsField = &Connection::SqlParams::query_options; - auto conn = std::make_shared(); - for (auto const& level : levels) { - google::cloud::testing_util::ScopedEnvironment opt_ver_env( - "SPANNER_OPTIMIZER_VERSION", level.env); - google::cloud::testing_util::ScopedEnvironment opt_stats_env( - "SPANNER_OPTIMIZER_STATISTICS_PACKAGE", level.env); - auto client_qo = QueryOptions() - .set_optimizer_version(level.client) - .set_optimizer_statistics_package(level.client); - Client client(conn, ClientOptions().set_query_options(client_qo)); - auto expected = QueryOptions() - .set_optimizer_version(level.expected) - .set_optimizer_statistics_package(level.expected); - EXPECT_CALL(*conn, ExecuteQuery(Field(kQueryOptionsField, Eq(expected)))) - .Times(AnyNumber()); - - auto const qo = QueryOptions() - .set_optimizer_version(level.function) - .set_optimizer_statistics_package(level.function); - auto const ro = Transaction::ReadOnlyOptions{}; - auto const su = Transaction::SingleUseOptions{ro}; - - // Call all the overloads that accept QueryOptions to ensure they all work. - client.ExecuteQuery(SqlStatement{}, qo); - client.ExecuteQuery(su, SqlStatement{}, qo); - client.ExecuteQuery(Transaction{ro}, SqlStatement{}, qo); - client.ExecuteQuery(QueryPartition{}, qo); - - client.ProfileQuery(SqlStatement{}, qo); - client.ProfileQuery(su, SqlStatement{}, qo); - client.ProfileQuery(Transaction{ro}, SqlStatement{}, qo); - - client.ExecuteDml(Transaction{ro}, SqlStatement{}, qo); - client.ProfileDml(Transaction{ro}, SqlStatement{}, qo); - client.AnalyzeSql(Transaction{ro}, SqlStatement{}, qo); + // Check optimizer_statistics_package. + { + QueryOptions preferred; + preferred.set_optimizer_statistics_package("preferred"); + QueryOptions fallback; + fallback.set_optimizer_statistics_package("fallback"); + absl::optional optimizer_statistics_package_env = + "environment"; + EXPECT_EQ(OverlayQueryOptions(preferred, fallback, absl::nullopt, + optimizer_statistics_package_env) + .optimizer_statistics_package(), + "preferred"); + preferred.set_optimizer_statistics_package(absl::nullopt); + EXPECT_EQ(OverlayQueryOptions(preferred, fallback, absl::nullopt, + optimizer_statistics_package_env) + .optimizer_statistics_package(), + "fallback"); + fallback.set_optimizer_statistics_package(absl::nullopt); + EXPECT_EQ(OverlayQueryOptions(preferred, fallback, absl::nullopt, + optimizer_statistics_package_env) + .optimizer_statistics_package(), + "environment"); + optimizer_statistics_package_env = absl::nullopt; + EXPECT_EQ(OverlayQueryOptions(preferred, fallback, absl::nullopt, + optimizer_statistics_package_env) + .optimizer_statistics_package(), + absl::nullopt); + } + + // Check request_priority. + { + QueryOptions preferred; + preferred.set_request_priority(RequestPriority::kHigh); + QueryOptions fallback; + fallback.set_request_priority(RequestPriority::kLow); + EXPECT_EQ( + OverlayQueryOptions(preferred, fallback, absl::nullopt, absl::nullopt) + .request_priority(), + RequestPriority::kHigh); + preferred.set_request_priority(absl::nullopt); + EXPECT_EQ( + OverlayQueryOptions(preferred, fallback, absl::nullopt, absl::nullopt) + .request_priority(), + RequestPriority::kLow); + fallback.set_request_priority(absl::nullopt); + EXPECT_EQ( + OverlayQueryOptions(preferred, fallback, absl::nullopt, absl::nullopt) + .request_priority(), + absl::nullopt); } } diff --git a/google/cloud/spanner/doc/spanner-main.dox b/google/cloud/spanner/doc/spanner-main.dox index 103b9c618d731..3bcbb4c56750e 100644 --- a/google/cloud/spanner/doc/spanner-main.dox +++ b/google/cloud/spanner/doc/spanner-main.dox @@ -89,6 +89,9 @@ behaviors in the library. - `SPANNER_OPTIMIZER_VERSION=n` sets the default query optimizer version to use in `Client::ExecuteQuery()` calls. +- `SPANNER_OPTIMIZER_STATISTICS_PACKAGE=...` specifies a statistics package + for the query optimizer to use when compiling a SQL query. + - `GOOGLE_CLOUD_CPP_ENABLE_CLOG=yes` turns on logging in the library, basically the library always "logs" but the logging infrastructure has no backend to actually print anything until the application sets a backend or they set this diff --git a/google/cloud/spanner/query_options.h b/google/cloud/spanner/query_options.h index 16e5cd96fd4eb..c683030e3c73b 100644 --- a/google/cloud/spanner/query_options.h +++ b/google/cloud/spanner/query_options.h @@ -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 + * of Client::OverlayQueryOptions(). + * * @see https://cloud.google.com/spanner/docs/reference/rest/v1/QueryOptions * @see http://cloud/spanner/docs/query-optimizer/manage-query-optimizer */ From 430f920d635e6eecfa6b1da084bcebc783082044 Mon Sep 17 00:00:00 2001 From: Bradley White <14679271+devbww@users.noreply.github.com> Date: Thu, 5 Aug 2021 13:39:28 -0400 Subject: [PATCH 2/2] Address review comments. --- google/cloud/spanner/client.cc | 89 +++++++++++++++------------- google/cloud/spanner/client.h | 17 ++++-- google/cloud/spanner/client_test.cc | 76 ++++++++++-------------- google/cloud/spanner/query_options.h | 5 +- 4 files changed, 94 insertions(+), 93 deletions(-) diff --git a/google/cloud/spanner/client.cc b/google/cloud/spanner/client.cc index 988e017cb71ba..cac591f2734e8 100644 --- a/google/cloud/spanner/client.cc +++ b/google/cloud/spanner/client.cc @@ -301,44 +301,6 @@ StatusOr Client::ExecutePartitionedDml( {std::move(statement), OverlayQueryOptions(opts)}); } -// Returns a QueryOptions that has each attribute set according to the -// preferred/fallback/environment hierarchy. -QueryOptions Client::OverlayQueryOptions( - QueryOptions const& preferred, QueryOptions const& fallback, - absl::optional const& optimizer_version_env, - absl::optional const& optimizer_statistics_package_env) { - QueryOptions opts; - - // Choose the `optimizer_version` option. - if (preferred.optimizer_version().has_value()) { - opts.set_optimizer_version(preferred.optimizer_version()); - } else if (fallback.optimizer_version().has_value()) { - opts.set_optimizer_version(fallback.optimizer_version()); - } else if (optimizer_version_env.has_value()) { - opts.set_optimizer_version(*optimizer_version_env); - } - - // Choose the `optimizer_statistics_package` option. - if (preferred.optimizer_statistics_package().has_value()) { - opts.set_optimizer_statistics_package( - preferred.optimizer_statistics_package()); - } else if (fallback.optimizer_statistics_package().has_value()) { - opts.set_optimizer_statistics_package( - fallback.optimizer_statistics_package()); - } else if (optimizer_statistics_package_env.has_value()) { - opts.set_optimizer_statistics_package(*optimizer_statistics_package_env); - } - - // Choose the `request_priority` option. - if (preferred.request_priority().has_value()) { - opts.set_request_priority(preferred.request_priority()); - } else if (fallback.request_priority().has_value()) { - opts.set_request_priority(fallback.request_priority()); - } - - return opts; -} - // Returns a QueryOptions that has each attribute set according to // the hierarchy that options specified at the function call (i.e., // `preferred`) are preferred, followed by options set at the Client @@ -353,9 +315,9 @@ QueryOptions Client::OverlayQueryOptions(QueryOptions const& preferred) { static auto const* const kOptimizerStatisticsPackageEnvValue = new auto( google::cloud::internal::GetEnv("SPANNER_OPTIMIZER_STATISTICS_PACKAGE")); - return OverlayQueryOptions(preferred, opts_.query_options(), - *kOptimizerVersionEnvValue, - *kOptimizerStatisticsPackageEnvValue); + return spanner_internal::OverlayQueryOptions( + preferred, opts_.query_options(), *kOptimizerVersionEnvValue, + *kOptimizerStatisticsPackageEnvValue); } std::shared_ptr MakeConnection(spanner::Database const& db, @@ -400,5 +362,50 @@ std::shared_ptr MakeConnection( } // namespace SPANNER_CLIENT_NS } // namespace spanner + +namespace spanner_internal { +inline namespace SPANNER_CLIENT_NS { + +// Returns a QueryOptions that has each attribute set according to the +// preferred/fallback/environment hierarchy. +spanner::QueryOptions OverlayQueryOptions( + spanner::QueryOptions const& preferred, + spanner::QueryOptions const& fallback, + absl::optional const& optimizer_version_env, + absl::optional const& optimizer_statistics_package_env) { + spanner::QueryOptions opts; + + // Choose the `optimizer_version` option. + if (preferred.optimizer_version().has_value()) { + opts.set_optimizer_version(preferred.optimizer_version()); + } else if (fallback.optimizer_version().has_value()) { + opts.set_optimizer_version(fallback.optimizer_version()); + } else if (optimizer_version_env.has_value()) { + opts.set_optimizer_version(*optimizer_version_env); + } + + // Choose the `optimizer_statistics_package` option. + if (preferred.optimizer_statistics_package().has_value()) { + opts.set_optimizer_statistics_package( + preferred.optimizer_statistics_package()); + } else if (fallback.optimizer_statistics_package().has_value()) { + opts.set_optimizer_statistics_package( + fallback.optimizer_statistics_package()); + } else if (optimizer_statistics_package_env.has_value()) { + opts.set_optimizer_statistics_package(*optimizer_statistics_package_env); + } + + // Choose the `request_priority` option. + if (preferred.request_priority().has_value()) { + opts.set_request_priority(preferred.request_priority()); + } else if (fallback.request_priority().has_value()) { + opts.set_request_priority(fallback.request_priority()); + } + + return opts; +} + +} // namespace SPANNER_CLIENT_NS +} // namespace spanner_internal } // namespace cloud } // namespace google diff --git a/google/cloud/spanner/client.h b/google/cloud/spanner/client.h index 158e36a8d1ff2..f6cbbb717d94c 100644 --- a/google/cloud/spanner/client.h +++ b/google/cloud/spanner/client.h @@ -643,11 +643,6 @@ class Client { SqlStatement statement, QueryOptions const& opts = {}); private: - friend class OverlayQueryOptionsTester; - static QueryOptions OverlayQueryOptions( - QueryOptions const& preferred, QueryOptions const& fallback, - absl::optional const& optimizer_version_env, - absl::optional const& optimizer_statistics_package_env); QueryOptions OverlayQueryOptions(QueryOptions const&); std::shared_ptr conn_; @@ -727,6 +722,18 @@ std::shared_ptr MakeConnection( } // namespace SPANNER_CLIENT_NS } // namespace spanner + +namespace spanner_internal { +inline namespace SPANNER_CLIENT_NS { + +spanner::QueryOptions OverlayQueryOptions( + spanner::QueryOptions const& preferred, + spanner::QueryOptions const& fallback, + absl::optional const& optimizer_version_env, + absl::optional const& optimizer_statistics_package_env); + +} // namespace SPANNER_CLIENT_NS +} // namespace spanner_internal } // namespace cloud } // namespace google diff --git a/google/cloud/spanner/client_test.cc b/google/cloud/spanner/client_test.cc index f6de08f15b662..d53dcc3f34a8e 100644 --- a/google/cloud/spanner/client_test.cc +++ b/google/cloud/spanner/client_test.cc @@ -34,19 +34,6 @@ namespace google { namespace cloud { namespace spanner { inline namespace SPANNER_CLIENT_NS { - -class OverlayQueryOptionsTester { - public: - static QueryOptions OverlayQueryOptions( - QueryOptions const& preferred, QueryOptions const& fallback, - absl::optional const& optimizer_version_env, - absl::optional const& optimizer_statistics_package_env) { - return Client::OverlayQueryOptions(preferred, fallback, - optimizer_version_env, - optimizer_statistics_package_env); - } -}; - namespace { namespace spanner_proto = ::google::spanner::v1; @@ -1034,9 +1021,6 @@ TEST(ClientTest, ProfileQueryWithOptionsSuccess) { } TEST(ClientTest, QueryOptionsOverlayPrecedence) { - constexpr auto OverlayQueryOptions = // NOLINT(readability-identifier-naming) - OverlayQueryOptionsTester::OverlayQueryOptions; - // Check optimizer_version. { QueryOptions preferred; @@ -1044,23 +1028,23 @@ TEST(ClientTest, QueryOptionsOverlayPrecedence) { QueryOptions fallback; fallback.set_optimizer_version("fallback"); absl::optional optimizer_version_env = "environment"; - EXPECT_EQ(OverlayQueryOptions(preferred, fallback, optimizer_version_env, - absl::nullopt) + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, optimizer_version_env, absl::nullopt) .optimizer_version(), "preferred"); preferred.set_optimizer_version(absl::nullopt); - EXPECT_EQ(OverlayQueryOptions(preferred, fallback, optimizer_version_env, - absl::nullopt) + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, optimizer_version_env, absl::nullopt) .optimizer_version(), "fallback"); fallback.set_optimizer_version(absl::nullopt); - EXPECT_EQ(OverlayQueryOptions(preferred, fallback, optimizer_version_env, - absl::nullopt) + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, optimizer_version_env, absl::nullopt) .optimizer_version(), "environment"); optimizer_version_env = absl::nullopt; - EXPECT_EQ(OverlayQueryOptions(preferred, fallback, optimizer_version_env, - absl::nullopt) + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, optimizer_version_env, absl::nullopt) .optimizer_version(), absl::nullopt); } @@ -1073,23 +1057,27 @@ TEST(ClientTest, QueryOptionsOverlayPrecedence) { fallback.set_optimizer_statistics_package("fallback"); absl::optional optimizer_statistics_package_env = "environment"; - EXPECT_EQ(OverlayQueryOptions(preferred, fallback, absl::nullopt, - optimizer_statistics_package_env) + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, absl::nullopt, + optimizer_statistics_package_env) .optimizer_statistics_package(), "preferred"); preferred.set_optimizer_statistics_package(absl::nullopt); - EXPECT_EQ(OverlayQueryOptions(preferred, fallback, absl::nullopt, - optimizer_statistics_package_env) + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, absl::nullopt, + optimizer_statistics_package_env) .optimizer_statistics_package(), "fallback"); fallback.set_optimizer_statistics_package(absl::nullopt); - EXPECT_EQ(OverlayQueryOptions(preferred, fallback, absl::nullopt, - optimizer_statistics_package_env) + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, absl::nullopt, + optimizer_statistics_package_env) .optimizer_statistics_package(), "environment"); optimizer_statistics_package_env = absl::nullopt; - EXPECT_EQ(OverlayQueryOptions(preferred, fallback, absl::nullopt, - optimizer_statistics_package_env) + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, absl::nullopt, + optimizer_statistics_package_env) .optimizer_statistics_package(), absl::nullopt); } @@ -1100,20 +1088,20 @@ TEST(ClientTest, QueryOptionsOverlayPrecedence) { preferred.set_request_priority(RequestPriority::kHigh); QueryOptions fallback; fallback.set_request_priority(RequestPriority::kLow); - EXPECT_EQ( - OverlayQueryOptions(preferred, fallback, absl::nullopt, absl::nullopt) - .request_priority(), - RequestPriority::kHigh); + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, absl::nullopt, absl::nullopt) + .request_priority(), + RequestPriority::kHigh); preferred.set_request_priority(absl::nullopt); - EXPECT_EQ( - OverlayQueryOptions(preferred, fallback, absl::nullopt, absl::nullopt) - .request_priority(), - RequestPriority::kLow); + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, absl::nullopt, absl::nullopt) + .request_priority(), + RequestPriority::kLow); fallback.set_request_priority(absl::nullopt); - EXPECT_EQ( - OverlayQueryOptions(preferred, fallback, absl::nullopt, absl::nullopt) - .request_priority(), - absl::nullopt); + EXPECT_EQ(spanner_internal::OverlayQueryOptions( + preferred, fallback, absl::nullopt, absl::nullopt) + .request_priority(), + absl::nullopt); } } diff --git a/google/cloud/spanner/query_options.h b/google/cloud/spanner/query_options.h index c683030e3c73b..88fad29e0e130 100644 --- a/google/cloud/spanner/query_options.h +++ b/google/cloud/spanner/query_options.h @@ -30,9 +30,6 @@ 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 - * of Client::OverlayQueryOptions(). - * * @see https://cloud.google.com/spanner/docs/reference/rest/v1/QueryOptions * @see http://cloud/spanner/docs/query-optimizer/manage-query-optimizer */ @@ -96,6 +93,8 @@ class QueryOptions { } private: + // Note: If you add an attribute here, remember to update the implementation + // of Client::OverlayQueryOptions(). absl::optional optimizer_version_; absl::optional optimizer_statistics_package_; absl::optional request_priority_;