[jira] [Commented] (HBASE-17283) [C++] Result class

["Enis Soztutar (JIRA)" <ji...@apache.org>]

    [ 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)