Wes McKinney has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 5: (3 comments) I'll let Matt take over the CR when he gets back. It's hard to see how we can proceed further with any implementation work without creating a comprehensive set of tests. We can scrutinize the implementation details, but ultimately the proof is in the pudding (passing tests, especially in the error handling part of things). It would be much more productive to flip between test cases and implementation and figure out what is being tested well and what's not being tested. http://gerrit.cloudera.org:8080/#/c/2645/5/src/hs2client/columnar-row-set.cc File src/hs2client/columnar-row-set.cc: Line 37: impl->column = row_set->columns[i]; I think this is still copying data. Line 77: nulls = &impl_->column.boolVal.nulls; You want here: reinterpret_cast<const uint8_t*>( impl_->column.boolVal.nulls.c_str()) http://gerrit.cloudera.org:8080/#/c/2645/5/src/hs2client/operation.cc File src/hs2client/operation.cc: Line 30: Operation::Operation(shared_ptr<ThriftRPC> rpc) I would use const std::shared_ptr<ThriftRPC>& rpc here and skip the std::move (both options increment perform the same atomic reference count actions IIUC) as a matter of consistency (otherwise the ownership-acquisition is happening in the pass-by-value rather than in the assignment of rpc_). I can let Matt opine further as I don't know enough to say which is better in general. -- To view, visit http://gerrit.cloudera.org:8080/2645 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7be82d5897ad6093116915b17924674c4ae508a0 Gerrit-PatchSet: 5 Gerrit-Project: hs2client Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Wes McKinney <[email protected]> Gerrit-HasComments: Yes
