Implement bigtable::RowSet #165
Conversation
googlebot
added
the
cla: yes
|
What's |
|
A typo. Fixed. |
|
Can we have a function that prunes out row keys less than or equal to a string? I plan to use it like so: DanielMahu@c887afd |
|
We do not have such a function yet. |
DanielMahu
left a comment
DanielMahu
left a comment
There was a problem hiding this comment.
I have a few non-critical comments, otherwise LGTM
bigtable/client/row_set.h
Outdated
| namespace bigtable { | ||
| inline namespace BIGTABLE_CLIENT_NS { | ||
| /** | ||
| * Represent (possibly non-continuous) set of row keys. |
There was a problem hiding this comment.
nit:
- Represent a (possibly non-continuous) set of row keys.
bigtable/client/row_set.h
Outdated
| } | ||
|
|
||
| /// Add @p row_key to the set. | ||
| void Append(std::string row_key) { |
There was a problem hiding this comment.
Append(std::string) and Append(char const*) are both made redundant by Append(absl::string_view), any reason to keep them?
There was a problem hiding this comment.
AFAIK, you cannot move from absl::string_view into a std::string so you must always copy. The two overloads allow you to eliminate copies when possible. If you think I am optimizing too early, happy to remove. Also reordered the functions to explain why.
| } | ||
|
|
||
| /// Add @p row_key to the set. | ||
| void Append(char const* row_key) { Append(absl::string_view(row_key)); } |
There was a problem hiding this comment.
This can also be removed together with Append(std::string).
bigtable/client/row_set.cc
Outdated
|
|
||
| namespace bigtable { | ||
| inline namespace BIGTABLE_CLIENT_NS { | ||
| namespace btproto = ::google::bigtable::v2; |
|
If @DanielMahu (or anybody else) thinks that the overloads are premature optimization I am happy to send a second PR to remove them. |
4 participants
This fixes #31. Implement a wrapper for
::google::bigtable::v2::RowSet.