feat!: support for select-star queries #976
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #976 +/- ##
==========================================
+ Coverage 94.12% 94.14% +0.01%
==========================================
Files 96 159 +63
Lines 4068 10640 +6572
==========================================
+ Hits 3829 10017 +6188
- Misses 239 623 +384
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
A few nits and suggestions in the comments.
I must confess that I cannot quite follow if move semantics are supported, so I will just ask: how do I write a query returning large strings or bytes such that those are not copied too many times?
Reviewed 24 of 24 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @devjgm and @mr-salty)
google/cloud/spanner/results.h, line 56 at r1 (raw file):
* A `QueryResult` object is itself a range defined by the [Input * Iterators][input-iterator] returned from `begin()` and `end(). Callers may * directly iterator a `QueryResult` instance, which will return a sequence of
nit: "directly iterator"? maybe "directly iterate"?
google/cloud/spanner/row.h, line 198 at r1 (raw file):
/** * Creates a `Row` with the specified column names and values that may be
Consider removing "that may be useful when testing" and adding something to the long description, such as " ... that goes in each column. This function in intended for application developers mocking the results of a ExecuteQuery call".
google/cloud/spanner/row.h, line 208 at r1 (raw file):
/** * Creates a `Row` with autogenerated column names that may be useful when * testing.
same suggestions as above regarding "may be useful".
google/cloud/spanner/row.h, line 405 at r1 (raw file):
* must be a range defined by `RowStreamIterator` objects. * * Ownership of the @p range is not transferred, so it must outlive the
That deserves a @note or even @warning.
There was a problem hiding this comment.
Unless there's a bug or an accidentally forgotten && somewhere, I think efficient moves are the normal behavior that users will naturally get. For example, I think all of our examples and samples do this correctly and have the least possible number of copies. Let me annotate an example with places where Values might be copied/moved:
QueryResult result = c.Read(...); // [1]
for (auto tup : StreamOf<RowType>(result)) { // [2], [3]
if (!tup) { // [4]
...
}
auto const& x = std::get<0>(*tup); // [5]
auto y = std::move(std::get<0>(*tup)); // [6]
auto z = std::get<0>(*tup); // [7]
}Here's what I think happens in that example:
- The Read() starts. Some Values may trickle in at this point (whatever was returned in the first response). Those are moved into the
PartialResultSetSource::buffer_vector. - As we iterate the
resultrange via itsRowStreamIteratorthe proto values in thebuffer_(mentioned in the previous step) are moved intospanner::Valueobjects to fill aRow. That vector ofspanner::Valueis then moved intoRow, oneRowfor each iteration. So far only moves, no copies. - Then
StreamOfconsumes and parses eachRow(from the previous step) into the specified tuple. It calls the&&-qualified overload ofRow::get<T>()to make sure the values are moved as efficiently as possible. Oops! Wait, no it doesn't. It was accidentally copying here. Fix included in this PR. Fixed. Now, the caller is catching the returned tuple by-value in this case (auto tup), but that's a caller choice. This would be good if they then want to move out of the tuple. - Actually, the rest of the numbered lines here are all just normal tuple stuff that I'm sure you know, so I'll stop here.
Reviewable status: 22 of 24 files reviewed, 1 unresolved discussion (waiting on @coryan and @mr-salty)
google/cloud/spanner/row.h, line 198 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Consider removing "that may be useful when testing" and adding something to the long description, such as " ... that goes in each column. This function in intended for application developers mocking the results of a ExecuteQuery call".
Good suggestion. Done here and for the next function too.
google/cloud/spanner/row.h, line 405 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
That deserves a
@noteor even@warning.
I added this is as a @note. I don't think this is a particularly sharp edge. Callers would have to go pretty far out of their way to do this wrong. The "easy" mistake might be to pass a temporary range argument, but there's a static_assert in the c'tor body to catch that and break the build.
Let me know if you think this still may warrant a @warning.
There was a problem hiding this comment.
Thanks for taking the time to explain this. I would not have been able to catch that missing move.
Reviewed 2 of 2 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @mr-salty)
google/cloud/spanner/row.h, line 405 at r1 (raw file):
Previously, devjgm (Greg Miller) wrote…
I added this is as a
@note. I don't think this is a particularly sharp edge. Callers would have to go pretty far out of their way to do this wrong. The "easy" mistake might be to pass a temporaryrangeargument, but there's a static_assert in the c'tor body to catch that and break the build.Let me know if you think this still may warrant a
@warning.
Looks good, thanks.
…panner#976) Fixes googleapis/google-cloud-cpp-spanner#387 BREAKING CHANGE: This PR changes QueryResult and ProfileQueryResult to themselves be ranges with begin() and end() member functions. Users can now directly use these objects in range-for loops. This gives users access to each row in a dynamic way allow them to inspect each row's values by column name, index, or iterate each value in the row. Users may also wrap these objects in a StreamOf<Tuple>, which will parse each row into the given Tuple. This PR also removes RowParser<T>
Fixes #387
BREAKING CHANGE: This PR changes
QueryResultandProfileQueryResultto themselves be ranges withbegin()andend()member functions. Users can now directly use these objects in range-for loops. This gives users access to each row in a dynamic way allow them to inspect each row's values by column name, index, or iterate each value in the row.Users may also wrap these objects in a
StreamOf<Tuple>, which will parse each row into the given Tuple.This PR also removes
RowParser<T>This change is