feat!: Use ReadResult instead of ResultSet; remove ResultSet #935
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #935 +/- ##
==========================================
+ Coverage 94.12% 94.25% +0.13%
==========================================
Files 90 91 +1
Lines 3895 3897 +2
==========================================
+ Hits 3666 3673 +7
+ Misses 229 224 -5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
You may need to open some cleanup bugs to deal with the filenames and the lack of tests for ExecuteQueryResult bothers me a wee bit.
Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @devbww, @devjgm, @mr-salty, and @scotthart)
google/cloud/spanner/client.h, line 78 at r1 (raw file):
* @code * namespace cs = ::google::cloud::spanner; * using ::google::cloud::StatusOr;
nit: I think this is no longer used.
google/cloud/spanner/client.h, line 84 at r1 (raw file):
* auto client = cs::Client(conn); * * ReadResult result = client.Read(...);
I think that should becs::ReadResult maybe?
google/cloud/spanner/result_set_test.cc, line 36 at r1 (raw file):
using ::testing::Return; TEST(ReadResult, IterateNoRows) {
nit: the filename is a bit odd right now. And I do not see any tests for ExecuteQueryResult, both these things can be post-poned with a issue in GitHub.
google/cloud/spanner/samples/samples.cc, line 1192 at r1 (raw file):
auto result_set = client.ExecuteQuery(*partition); using RowType = spanner::Row<std::int64_t, std::string, std::string>; for (auto const& row : result_set.Rows<RowType>()) {
nit: I am not sure we want const here.
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @coryan, @devbww, @devjgm, and @mr-salty)
google/cloud/spanner/client.h, line 84 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I think that should be
cs::ReadResultmaybe?
Done.
google/cloud/spanner/result_set_test.cc, line 36 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
nit: the filename is a bit odd right now. And I do not see any tests for
ExecuteQueryResult, both these things can be post-poned with a issue in GitHub.
It's looking like ReadResult and ExecuteQueryResult are going to be merged. I'm open to suggestions for file names, and opinions on whether or not to keep all these "result" types in one file or split them up. And yes, testing needs to be added for the new types that are in a current state of flux.
google/cloud/spanner/samples/samples.cc, line 1192 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
nit: I am not sure we want
consthere.
Greg and I were talking earlier about what our "Best Practices" should be regarding the element type as we iterate though the collection returned from Rows(). I don't think we have a great answer yet.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @devbww, @devjgm, @mr-salty, and @scotthart)
google/cloud/spanner/result_set_test.cc, line 36 at r1 (raw file):
Previously, scotthart (Scott Hart) wrote…
It's looking like ReadResult and ExecuteQueryResult are going to be merged. I'm open to suggestions for file names, and opinions on whether or not to keep all these "result" types in one file or split them up. And yes, testing needs to be added for the new types that are in a current state of flux.
If we keep both types and we decide to keep them in the same file, then result_set.h is a good name. The alternative is to split each in its own file, does not matter much because we will force folks to include both anyway.
google/cloud/spanner/samples/samples.cc, line 1192 at r1 (raw file):
Previously, scotthart (Scott Hart) wrote…
Greg and I were talking earlier about what our "Best Practices" should be regarding the element type as we iterate though the collection returned from Rows(). I don't think we have a great answer yet.
"Almost Always Const" is becoming a thing... You do not always want auto const& because sometimes the values are large and you may want to move the values out of the object referenced by the iterator.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @devbww, @devjgm, and @mr-salty)
update documentation address review comments
scotthart
force-pushed
the
use_read_result
branch
from
816c9ba to
39bb685
Compare
scotthart
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
BREAKING CHANGE: Completes refactoring and removal of ResultSet in favor of more
specific result return types. Profiling methods and result types to be added in a subsequent
PR. Possible combining of ReadResult and ExecuteQueryResult types deferred to a future
PR.
This change is