[
https://issues.apache.org/jira/browse/HBASE-17283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15771295#comment-15771295
]
Enis Soztutar commented on HBASE-17283:
---------------------------------------
bq. Using a return by value will have an overhead of temporaries. Returning a
reference of local variable is not safe. Since we need the Cell instance to
live beyond the scope of the method we can create it on heap and we are using a
smart pointer i.e. unique_ptr in our case.
Thanks for the explanations above. I was asking about why are we returning
unique_ptr with copying the Cell, versus returning Cells uncopied with
shared_ptr's. I think Result should keep cells as vector<shared_ptr<Cell>> and
return vectors of shared_ptrs, so that there is no copying of the data when the
application reads back the cells from Result objects. Another alternative is
having everything return by reference to Cells or values, and tie the returned
stuff's lifetimes to that of the Result object. Not sure which one would be
better for consumers.
The first if is not needed here:
{code}
+ if (!cells.empty()) {
+ for (const auto &cell : cells) {
{code}
here as well:
{code}
+ if (!result_map_.empty()) {
+ if (!IsEmpty()) {
{code}
The iterators will not iterate anyway, no?
Result is a construct-once, and read-only object. Why are you checking the
row_.size here in the ctor? You can simply do the second line, no?
{code}
+ if (0 == row_.size()) {
+ row_ = (0 == cells_.size() ? "" : cells_[0]->Row());
+ }
{code}
This is a nit, but in java we use (x == 0) rather than (0 == x). We should use
that form unless it is more common in C++. See
https://en.wikipedia.org/wiki/Yoda_conditions.
The maps should not be using {{unsigned long}}, because the Timestamp is not a
{{long}} anymore. We are using int64_t, did you see the changes I've pushed in
HBASE-17220?
{code}
std::map<unsigned long, std::string, std::greater<unsigned long>>
{code}
Can you please run {{bin/format-code}} and also {{make lint}} for the incoming
patches. The cpplint is unfortunately will report existing issues as well
(which we do not need to address in this patch), but if there are issues that
are due to the new patch, we should address those.
This should be fine, you can remove the TODO (but keep the comment). We only
build the Result object once when reading back from an RPC. Partial results
might change that, but we can address those issues when we have that support
down the line.
{code}
+ //TODO Feedback needed.
+ // We create the map when cells are added. unlike java where map is
created when result.getMap() is called
{code}
The following is by-design. In most of the use cases, the client is only
interested in the latest version, and by default there is only 1 version
anyway. So this is a convenience method. Maybe add a javadoc in FamilyMap()
method saying something like {{ Returns map of qualifiers to values, only
includes latest values}}.
{code}
+ //TODO Feedback needed.
+ //We break after inserting the first value. Result.java takes
only the first value
{code}
> [C++] Result class
> ------------------
>
> Key: HBASE-17283
> URL: https://issues.apache.org/jira/browse/HBASE-17283
> Project: HBase
> Issue Type: Sub-task
> Reporter: Sudeep Sunthankar
> Assignee: Sudeep Sunthankar
> Attachments: HBASE-17283.HBASE-14850.v1.patch,
> HBASE-17283.HBASE-14850.v3.patch, hbase-17283_v2.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
