feat(spanner): add initial PostgreSQL samples #8617
Conversation
product-auto-label
bot
added
api: spanner
|
Here is the summary of changes. You are about to add 5 region tags.
This comment is generated by snippet-bot.
|
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #8617 +/- ##
==========================================
- Coverage 93.79% 93.64% -0.15%
==========================================
Files 1449 1450 +1
Lines 123769 123968 +199
==========================================
+ Hits 116087 116092 +5
- Misses 7682 7876 +194
Continue to review full report at Codecov.
|
| @@ -16,7 +16,7 @@ | |||
|
|
|||
| function (spanner_client_define_samples) | |||
| set(spanner_client_integration_samples # cmake-format: sort | |||
| samples.cc) | |||
| pg_samples.cc samples.cc) | |||
There was a problem hiding this comment.
I think the file should have a more obvious name like postgresql_samples.cc or postgres_samples.cc.
There was a problem hiding this comment.
Meh. OK, postgresql_samples.cc it is.
| std::int64_t budget) { | ||
| auto sql = google::cloud::spanner::SqlStatement( | ||
| "UPDATE Albums SET MarketingBudget = $1" | ||
| " WHERE SingerId = $3 AND AlbumId = $2", |
There was a problem hiding this comment.
It doesn't look like there is a definitive source of what these samples are supposed to look like (just their names), but this one says it should be similar to the non-pg version, where these were ordered this way, although the non-pg versions uses names to bind instead of indices. I could imagine that explicitly using out-of-order indices is a teaching point, but probably not worth it. So, yes, swapped.
| return 0; | ||
| } | ||
|
|
||
| void SampleBanner(std::string const& name) { |
There was a problem hiding this comment.
We could factor this out as it appears in samples.cc as well. (We could also refactor the Command/make_command_entry stuff too). Probably not something we want to do in this PR. Maybe not something we ever want to do.
There was a problem hiding this comment.
I mentioned possible refactorings down the line in the original PR description, but, yes, we should certainly postpone that decision until we see where things fall. (At the moment it looks like it would be a net complication.)
| if (google::cloud::internal::GetEnv(auto_run).value_or("") == "yes") { | ||
| return RunAll(); | ||
| } | ||
| std::string program(ac ? (ac--, *av++) : "pg_samples"); |
There was a problem hiding this comment.
Yeah. It can be instructive to try executing common programs with no arguments and seeing how many survive the ordeal.
| }; | ||
| auto it = commands.find(argv[0]); | ||
| if (it == commands.end()) { | ||
| throw std::runtime_error(argv[0] + ": Unknown command"); |
There was a problem hiding this comment.
So how can the user learn which commands are available? Well they can look in the code, but I don't think they should have to. Can we print all commands here?
There was a problem hiding this comment.
I added a "help" command to list all the available commands, and mention using it in this error message.
| if (ac == 0) { | ||
| throw std::runtime_error("Usage: " + program + | ||
| " <command> [<argument> ...]"); | ||
| } |
There was a problem hiding this comment.
I think we also want to print the available commands in this failure case.
There was a problem hiding this comment.
I added a "help" command to list all the available commands, and mention using it in this error message.
These samples are sufficiently different from the "Google Standard" dialect ones that they deserve their own source file (not to mention that `samples.cc` is becoming unmanageable). If there ends up being any significant commonality between the samples implementations, we can factor it out later.
devbww
force-pushed
the
database-dialect-samples
branch
from
fc4fc84 to
5ac6fd3
Compare
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @dbolduc and @devbww)
google/cloud/spanner/samples/pg_samples.cc, line 177 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
It doesn't look like there is a definitive source of what these samples are supposed to look like (just their names), but this one says it should be similar to the non-pg version, where these were ordered this way, although the non-pg versions uses names to bind instead of indices. I could imagine that explicitly using out-of-order indices is a teaching point, but probably not worth it. So, yes, swapped.
Hm, for the record I was thinking:
auto sql = google::cloud::spanner::SqlStatement(
"UPDATE Albums SET MarketingBudget = $1"
" WHERE SingerId = $2 AND AlbumId = $3",
{{"p1", google::cloud::spanner::Value(budget)},
{"p2", google::cloud::spanner::Value(singer_id)},
{"p3", google::cloud::spanner::Value(album_id)}});
But this is all a nit anyway.
google/cloud/spanner/samples/pg_samples.cc, line 263 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
I added a "help" command to list all the available commands, and mention using it in this error message.
works for me
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @dbolduc)
google/cloud/spanner/samples/pg_samples.cc, line 177 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
Hm, for the record I was thinking:
auto sql = google::cloud::spanner::SqlStatement( "UPDATE Albums SET MarketingBudget = $1" " WHERE SingerId = $2 AND AlbumId = $3", {{"p1", google::cloud::spanner::Value(budget)}, {"p2", google::cloud::spanner::Value(singer_id)}, {"p3", google::cloud::spanner::Value(album_id)}});But this is all a nit anyway.
I was going to say that it makes a little more sense this way because the Albums key is (AlbumId, SingerId) (for better or worse), but it looks like its the other way around. I'm going to change that.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
These samples are sufficiently different from the "Google Standard"
dialect ones that they deserve their own source file (not to mention
that
samples.ccis becoming unmanageable). If there ends up beingany significant commonality between the samples implementations, we
can factor it out later.
This change is