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.

Sign up for GitHub

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

fix(spanner): fix Client::OverlayQueryOptions() to merge correctly #7118

Merged
merged 2 commits into from
Aug 5, 2021
Merged

fix(spanner): fix Client::OverlayQueryOptions() to merge correctly #7118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8228326
fix(spanner): fix Client::OverlayQueryOptions() to merge correctly
devbww Aug 5, 2021
430f920
Address review comments.
devbww Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 36 additions & 18 deletions google/cloud/spanner/client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,29 +301,21 @@ StatusOr<PartitionedDmlResult> 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<std::string> const& optimizer_version_env,
absl::optional<std::string> 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 (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.
Expand All @@ -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<spanner::Connection> MakeConnection(spanner::Database const& db,
Options opts) {
internal::CheckExpectedOptions<
Expand Down
5 changes: 5 additions & 0 deletions google/cloud/spanner/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,11 @@ class Client {
SqlStatement statement, QueryOptions const& opts = {});

private:
friend class OverlayQueryOptionsTester;
static QueryOptions OverlayQueryOptions(
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, put this static function in the spanner_internal namespace and you don't need the friend helper, your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

QueryOptions const& preferred, QueryOptions const& fallback,
absl::optional<std::string> const& optimizer_version_env,
absl::optional<std::string> const& optimizer_statistics_package_env);
QueryOptions OverlayQueryOptions(QueryOptions const&);

std::shared_ptr<Connection> conn_;
Expand Down
149 changes: 92 additions & 57 deletions google/cloud/spanner/client_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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<std::string> const& optimizer_version_env,
absl::optional<std::string> const& optimizer_statistics_package_env) {
return Client::OverlayQueryOptions(preferred, fallback,
optimizer_version_env,
optimizer_statistics_package_env);
}
};

namespace {

namespace spanner_proto = ::google::spanner::v1;
Expand All @@ -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;
Expand Down Expand Up @@ -1026,59 +1034,86 @@ TEST(ClientTest, ProfileQueryWithOptionsSuccess) {
}

TEST(ClientTest, QueryOptionsOverlayPrecedence) {
struct Levels {
absl::optional<std::string> env;
absl::optional<std::string> client;
absl::optional<std::string> function;
absl::optional<std::string> expected;
};
std::vector<Levels> 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

OverlayQueryOptionsTester::OverlayQueryOptions;

// Check optimizer_version.
{
QueryOptions preferred;
preferred.set_optimizer_version("preferred");
QueryOptions fallback;
fallback.set_optimizer_version("fallback");
absl::optional<std::string> 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<MockConnection>();
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<std::string> 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);
}
}

Expand Down
3 changes: 3 additions & 0 deletions google/cloud/spanner/doc/spanner-main.dox
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions google/cloud/spanner/query_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no .cc for this interface, so private: section it is.

* of Client::OverlayQueryOptions().
*
* @see https://cloud.google.com/spanner/docs/reference/rest/v1/QueryOptions
* @see http://cloud/spanner/docs/query-optimizer/manage-query-optimizer
*/
Expand Down