This repository has been archived by the owner on Dec 8, 2021. It is now read-only.
feat: add function to get a single row from a range #1074
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
googlebot
added
the
cla: yes
devjgm
changed the title
coryan
approved these changes
Nov 17, 2019
There was a problem hiding this comment.
I like it, and I cannot think of a better one.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved
Fixes: googleapis#386 This is just a convenience, but it seems nice in some cases. I found on example that could use this in our current code. I also have another one in an upcoming PR where I would like this.
devjgm
force-pushed
the
feat-next-row
branch
from
99a7b11 to
6ce93c6
Compare
Codecov Report
@@ Coverage Diff @@
## master #1074 +/- ##
==========================================
- Coverage 92% 91.56% -0.44%
==========================================
Files 162 163 +1
Lines 11807 11826 +19
==========================================
- Hits 10863 10829 -34
- Misses 944 997 +53
Continue to review full report at Codecov.
|
devjgm
added a commit
that referenced
this pull request
Nov 21, 2019
After some discussion with the local folks, we decided to change the semantics and name of GetCurrentRow that was added in #1074. Previously GetCurrentRow would return the first row in a range that might potentially contain multiple rows. The observation is that those may not be the best or safest semantics for callers. The use case we're trying to address is when a callers does a query/read and they know that only one row should be returned. Maybe they used LIMIT 1, or maybe they specified a primary key to guarantee this. In any case, if they know that only one result can be returned, it would be nice to let them conveniently access that one row without having to create a loop, break out, etc. The problem with GetCurrentRow was that if the row range happened to have more than one row, there would be no error and the user may never notice this. So it seems safer to clearly name this function and change its semantics so that it will produce an error if the given range does not contain exactly a single row. We also considered requiring the caller to pass an rvalue (i.e., std::move the range into the argument). The reason I don't enforce this is because that would then prevent callers for subsequently calling the RowStream::ReadTimestamp function. Adding @devbww as the reviewer since he initially raised some of these questions. And cc: @coryan and @mr-salty as potentially interested observers who are welcome to comment if they want.
devjgm
added a commit
to devjgm/google-cloud-cpp
that referenced
this pull request
May 7, 2020
…e-cloud-cpp-spanner#1074) * feat: add function to get a single row from a range Fixes: googleapis/google-cloud-cpp-spanner#386 This is just a convenience, but it seems nice in some cases. I found on example that could use this in our current code. I also have another one in an upcoming PR where I would like this.
devjgm
added a commit
to devjgm/google-cloud-cpp
that referenced
this pull request
May 7, 2020
…-cpp-spanner#1092) After some discussion with the local folks, we decided to change the semantics and name of GetCurrentRow that was added in googleapis/google-cloud-cpp-spanner#1074. Previously GetCurrentRow would return the first row in a range that might potentially contain multiple rows. The observation is that those may not be the best or safest semantics for callers. The use case we're trying to address is when a callers does a query/read and they know that only one row should be returned. Maybe they used LIMIT 1, or maybe they specified a primary key to guarantee this. In any case, if they know that only one result can be returned, it would be nice to let them conveniently access that one row without having to create a loop, break out, etc. The problem with GetCurrentRow was that if the row range happened to have more than one row, there would be no error and the user may never notice this. So it seems safer to clearly name this function and change its semantics so that it will produce an error if the given range does not contain exactly a single row. We also considered requiring the caller to pass an rvalue (i.e., std::move the range into the argument). The reason I don't enforce this is because that would then prevent callers for subsequently calling the RowStream::ReadTimestamp function. Adding @devbww as the reviewer since he initially raised some of these questions. And cc: @coryan and @mr-salty as potentially interested observers who are welcome to comment if they want.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #386
This is just a convenience, but it seems nice in some cases. I found on
example that could use this in our current code. I also have another one
in an upcoming PR where I would like this.
Do we like this? Or do we have a better API to consider?
This change is