docs: updated snippets and samples for profile and analyze methods #1364

scotthart · 2020-03-12T21:23:35Z

This change is

docs: updated snippets and samples for profile and analyze methods.

codecov · 2020-03-12T22:02:36Z

Codecov Report

Merging #1364 into master will increase coverage by <.01%.
The diff coverage is 95.91%.

@@            Coverage Diff             @@
##           master    #1364      +/-   ##
==========================================
+ Coverage   94.53%   94.54%   +<.01%     
==========================================
  Files         186      184       -2     
  Lines       15135    15214      +79     
==========================================
+ Hits        14308    14384      +76     
- Misses        827      830       +3

Impacted Files	Coverage Δ
google/cloud/spanner/client.h	`100% <ø> (ø)`	⬆️
google/cloud/spanner/results.h	`95.83% <ø> (-4.17%)`	⬇️
google/cloud/spanner/samples/samples.cc	`88.23% <95.91%> (+0.24%)`	⬆️
google/cloud/spanner/internal/log_wrapper.h	`71.42% <0%> (-13.19%)`	⬇️
...ud/spanner/integration_tests/client_stress_test.cc	`80.55% <0%> (-1.86%)`	⬇️
...loud/spanner/internal/partial_result_set_source.cc	`93.24% <0%> (-1.36%)`	⬇️
google/cloud/spanner/value.h	`92.19% <0%> (-0.71%)`	⬇️
google/cloud/spanner/sql_statement.cc	`100% <0%> (ø)`	⬆️
google/cloud/spanner/internal/session.h	`100% <0%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88adb64...e1b5202. Read the comment docs.

google/cloud/spanner/samples/samples.cc

coryan

A few suggestions below, let me know if they do not make sense.

Reviewed 2 of 3 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @devbww, @devjgm, and @scotthart)

google/cloud/spanner/samples/samples.cc, line 1257 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

A general question that I have also been struggling with: Is it better for the snippet to include any necessary using declarations and namespace alias declarations, or to omit them? That is, is the brevity worth the incompleteness?

I would prefer if we kept them, they avoid questions around "What is this spanner:: namespace? The audience for these snippets are folks that may have never used our libraries before.

google/cloud/spanner/samples/samples.cc, line 1270 at r3 (raw file):

    if (!row) throw std::runtime_error(row.status().message());
    std::cout << "AlbumId: " << std::get<0>(*row) << "\t";
    std::cout << "AlbumTitle: " << std::get<1>(*row) << "\t";

nit: consider just discarding all the results, with a comment:

// Discard query values to keep this example brief.

google/cloud/spanner/samples/samples.cc, line 1432 at r3 (raw file):

  using ::google::cloud::StatusOr;
  namespace spanner = ::google::cloud::spanner;
  //! [execute-dml]

See above, please move this up, in fact, consider including the whole function so folks don't wonder where client came from.

google/cloud/spanner/samples/samples.cc, line 2177 at r3 (raw file):

          "SELECT SingerId, FirstName, LastName FROM Singers"));
  if (!plan) throw std::runtime_error(plan.status().message());
  if (plan->plan_nodes(0).kind() != google::spanner::v1::PlanNode::RELATIONAL &&

What do you think of wrapping these conditionals in a lambda that examples what they do?

auto is_partitionable_ = [](/*replace with actual type*/auto const& plan) {
  if (plan.plan_nodes().empty()) return false;
  if (plan.plan_nodes(0).kind() != google::spanner::v1::PlanNode::RELATIONAL) {
    return false;
  }
  if (plan.plan_nodes(0).display_name() != "Distributed Union") {
    // maybe even print why here...
    return false;
  }
  return true;
};

if (!is_partionable(*plan)) throw std::runtime_error(...);

devjgm

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @coryan, @devbww, and @scotthart)

google/cloud/spanner/samples/samples.cc, line 1501 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

Can this test ever fail?

If not, then maybe the type of dml_result should drop the StatusOr part to indicate that it can never fail.

google/cloud/spanner/samples/samples.cc, line 1263 at r3 (raw file):

      {{"start_title", spanner::Value("Aardvark")},
       {"end_title", spanner::Value("Goo")}});

In order to keep the samples as as focused as possible on the thing they're trying to demonstrate (in this case, profiling), I suggest removing the query params, and just inline the values into the select statement string.

google/cloud/spanner/samples/samples.cc, line 1270 at r3 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

nit: consider just discarding all the results, with a comment:

// Discard query values to keep this example brief.

FWIW, I think this is a good idea. It would also allow the example to get rid of the RowType alias, since we won't need to use StreamOf

google/cloud/spanner/samples/samples.cc, line 1442 at r3 (raw file):

{}

Since you're in here, you can remove the empty braces for the empty params.

google/cloud/spanner/samples/samples.cc, line 1491 at r3 (raw file):

                "UPDATE Albums SET MarketingBudget = MarketingBudget * 2"
                " WHERE SingerId = 1 AND AlbumId = 1",
                {}));

Please remove the empty params braces.

scotthart

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @coryan and @devbww)

google/cloud/spanner/samples/samples.cc, line 1257 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

I would prefer if we kept them, they avoid questions around "What is this spanner:: namespace? The audience for these snippets are folks that may have never used our libraries before.

Done.

google/cloud/spanner/samples/samples.cc, line 1501 at r1 (raw file):

Previously, devjgm (Greg Miller) wrote…

If not, then maybe the type of dml_result should drop the StatusOr part to indicate that it can never fail.

If the user chooses to call ProfileDml without using a Commit "transaction runner", then it could fail. For this particular implementation, it likely will not ever fail, but as this is example code, I figured checking was the thorough thing to do.

google/cloud/spanner/samples/samples.cc, line 1432 at r3 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

See above, please move this up, in fact, consider including the whole function so folks don't wonder where client came from.

I think the client is ubiquitous in all snippets as to not cause confusion.

google/cloud/spanner/samples/samples.cc, line 1491 at r3 (raw file):

Previously, devjgm (Greg Miller) wrote…

Please remove the empty params braces.

Removed them in all places in the file where calls to ExecuteDml and ProfileDml was including them.

google/cloud/spanner/samples/samples.cc, line 2177 at r3 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

What do you think of wrapping these conditionals in a lambda that examples what they do?

auto is_partitionable_ = [](/*replace with actual type*/auto const& plan) {
  if (plan.plan_nodes().empty()) return false;
  if (plan.plan_nodes(0).kind() != google::spanner::v1::PlanNode::RELATIONAL) {
    return false;
  }
  if (plan.plan_nodes(0).display_name() != "Distributed Union") {
    // maybe even print why here...
    return false;
  }
  return true;
};

if (!is_partionable(*plan)) throw std::runtime_error(...);

Done.

coryan

One item for discussion below, let me know what you think, I could be convinced that I am wrong.

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @devbww and @scotthart)

google/cloud/spanner/samples/samples.cc, line 1432 at r3 (raw file):

Previously, scotthart (Scott Hart) wrote…

I think the client is ubiquitous in all snippets as to not cause confusion.

FYI, and this is just a thought, but you know that, I know that, and a user that is reading many samples knows that. If this is the first sample they read (or they have not read them in a while) that would come as a complete surprise. I think each sample should stand on its own, but I could be convinced otherwise.

google/cloud/spanner/samples/samples.cc, line 2156 at r4 (raw file):

  // `ExecutionPlan` can be partitioned.
  auto is_partitionable = [](spanner::ExecutionPlan const& plan) {
    return (plan.plan_nodes(0).kind() ==

nit: the parenthesis are not needed in this return.

devbww

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @coryan, @devbww, @devjgm, and @scotthart)

google/cloud/spanner/samples/samples.cc, line 1501 at r1 (raw file):

Previously, scotthart (Scott Hart) wrote…

If the user chooses to call ProfileDml without using a Commit "transaction runner", then it could fail. For this particular implementation, it likely will not ever fail, but as this is example code, I figured checking was the thorough thing to do.

The observation is not about update, the result of ProfileDml(), but dml_result, the output of the mutator, which is only assigned when the ProfileDml() (and then the mutator) succeeds.

scotthart

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @coryan, @devbww, and @scotthart)

google/cloud/spanner/samples/samples.cc, line 1501 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

The observation is not about update, the result of ProfileDml(), but dml_result, the output of the mutator, which is only assigned when the ProfileDml() (and then the mutator) succeeds.

Done.

google/cloud/spanner/samples/samples.cc, line 2156 at r4 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

nit: the parenthesis are not needed in this return.

Agreed, I thought it might improve the readability of this multiline, compound conditional.

devbww

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @coryan and @scotthart)

coryan

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scotthart)

google/cloud/spanner/samples/samples.cc, line 2156 at r4 (raw file):

Previously, scotthart (Scott Hart) wrote…

Agreed, I thought it might improve the readability of this multiline, compound conditional.

Hmmm... Okay.

google/cloud/spanner/samples/samples.cc, line 2154 at r5 (raw file):

  // `ExecutionPlan` can be partitioned.
  auto is_partitionable = [](spanner::ExecutionPlan const& plan) {
    return (plan.plan_nodes(0).kind() ==

What if plan_nodes() is empty?

scotthart

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @coryan)

google/cloud/spanner/samples/samples.cc, line 1432 at r3 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

FYI, and this is just a thought, but you know that, I know that, and a user that is reading many samples knows that. If this is the first sample they read (or they have not read them in a while) that would come as a complete surprise. I think each sample should stand on its own, but I could be convinced otherwise.

Seeing as there are both flavors of snippets in this file, can we decide one way or the other if we include the entire function or not? Also, how should we handle situations where one function contains multiple snippets (maybe overlapping maybe not)?

google/cloud/spanner/samples/samples.cc, line 2154 at r5 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

What if plan_nodes() is empty?

Done.

coryan

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

google/cloud/spanner/samples/samples.cc, line 1432 at r3 (raw file):

Previously, scotthart (Scott Hart) wrote…

Seeing as there are both flavors of snippets in this file, can we decide one way or the other if we include the entire function or not? Also, how should we handle situations where one function contains multiple snippets (maybe overlapping maybe not)?

For me is whether the snippet is self-contained, not whether it is a complete function or not. Though most of the time "whole function" == "self-contained". Having types, functions, or objects appear in a snippet without context (telling you at least what it is) can only lead to confusion.

Let's move this discussion elsewhere, we are just dragging here.

…oogleapis/google-cloud-cpp-spanner#1364) * docs: add snippets for AnalyzeSql and ExecutionPlan docs: updated snippets and samples for profile and analyze methods. * debug query plan * debug query plan * debug query plan * address review comments * address review comments * address review comments * address review comments

scotthart requested review from coryan, devjgm and devbww March 12, 2020 21:23

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 12, 2020

docs: add snippets for AnalyzeSql and ExecutionPlan

88db8d7

docs: updated snippets and samples for profile and analyze methods.

scotthart force-pushed the update_code_examples branch from a7a05a9 to 88db8d7 Compare March 12, 2020 21:26

debug query plan

80e64fe

devbww reviewed Mar 12, 2020

View reviewed changes

google/cloud/spanner/samples/samples.cc Show resolved Hide resolved

google/cloud/spanner/samples/samples.cc Outdated Show resolved Hide resolved

google/cloud/spanner/samples/samples.cc Outdated Show resolved Hide resolved

scotthart added 2 commits March 12, 2020 18:51

debug query plan

3e008d9

debug query plan

e6ef15f

coryan suggested changes Mar 13, 2020

View reviewed changes

devjgm reviewed Mar 13, 2020

View reviewed changes

address review comments

7eddd83

scotthart commented Mar 13, 2020

View reviewed changes

coryan suggested changes Mar 13, 2020

View reviewed changes

address review comments

9898ee8

devbww suggested changes Mar 13, 2020

View reviewed changes

address review comments

d89f301

scotthart commented Mar 13, 2020

View reviewed changes

devbww reviewed Mar 13, 2020

View reviewed changes

coryan suggested changes Mar 13, 2020

View reviewed changes

address review comments

e1b5202

scotthart commented Mar 13, 2020

View reviewed changes

coryan approved these changes Mar 13, 2020

View reviewed changes

devbww approved these changes Mar 13, 2020

View reviewed changes

scotthart merged commit 58b119d into googleapis:master Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: updated snippets and samples for profile and analyze methods #1364

docs: updated snippets and samples for profile and analyze methods #1364

scotthart commented Mar 12, 2020 •

edited by coryan

Loading

codecov bot commented Mar 12, 2020 •

edited

Loading

coryan left a comment

devjgm left a comment

scotthart left a comment

coryan left a comment

devbww left a comment

scotthart left a comment

devbww left a comment

coryan left a comment

scotthart left a comment

coryan left a comment

docs: updated snippets and samples for profile and analyze methods #1364

docs: updated snippets and samples for profile and analyze methods #1364

Conversation

scotthart commented Mar 12, 2020 • edited by coryan Loading

codecov bot commented Mar 12, 2020 • edited Loading

Codecov Report

coryan left a comment

Choose a reason for hiding this comment

devjgm left a comment

Choose a reason for hiding this comment

scotthart left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

devbww left a comment

Choose a reason for hiding this comment

scotthart left a comment

Choose a reason for hiding this comment

devbww left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

scotthart left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

scotthart commented Mar 12, 2020 •

edited by coryan

Loading

codecov bot commented Mar 12, 2020 •

edited

Loading