Wes McKinney has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 4: (28 comments) http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/columnar-row-set.h File src/hs2client/columnar-row-set.h: Line 27: const void* nulls; `const uint8_t*` here Line 30: char c = (*((std::string*)nulls))[i / 8]; if you use `const uint8_t*` above, this is not needed. I don't think it's a good idea to use std::string in this manner anyhow (you may accidentally invoke constructor calls in some cases) http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/hs2client.cc File src/hs2client/hs2client.cc: Line 49: using apache::hive::service::cli::thrift::TStatusCode; You could instead use a namespace alias: namespace hs2 = apache::hive::service::cli::thrift; Line 58: using namespace std; Better to use qualified names Line 103: TFetchOrientation::type FetchOrientationToTFetchOrientation( Methods that are not exported should be static Line 132: unique_ptr<HS2Service>* out) { Need an HS2 protocol argument. We will eventually need to support non-columnar protocols (pre-V6). If the user passes < V6 protocol, return error Status Line 180: } It may be worth creating a macro like RETURN_NOT_OK but that captures and translates Thrift errors to hs2client::Status Line 194: TProtocolVersion hs2_protocol_version_; Struct data members do not have trailing underscores (https://google.github.io/styleguide/cppguide.html#Variable_Names). You may make this a class in the interest of readability Line 198: : impl_(new HS2SessionImpl()), rpc_(move(rpc)), open_(false) {} std::move is not necessary with std::shared_ptr Line 208: rpc_->client_->CloseSession(resp, req); Can this fail? Line 218: rpc_->client_->OpenSession(resp, req); Can this fail? Line 220: Status status = TStatusToStatus(resp.status); RETURN_NOT_OK(TStatusToStatus(resp.status)); Line 231: ExecuteStatementOperation(shared_ptr<ThriftRPC> rpc) : Operation(move(rpc)) {} const std::shared_ptr<ThriftRPC>&, and no std::move Line 233: Status Open(TSessionHandle sessionHandle, const string& statement, session_handle Line 241: rpc_->client_->ExecuteStatement(resp, req); failure possible? Line 255: const HS2ClientConfig& confOverlay, shared_ptr<Operation>* out) { conf_overlay Line 263: GetTablesOperation(shared_ptr<ThriftRPC> rpc) : Operation(move(rpc)) {} const sp<...>& and no std::move also Line 266: const std::string& tableName) { Please use Google C++ naming conventions Line 273: rpc_->client_->GetTables(resp, req); can this fail? Line 306: rpc_->client_->CancelOperation(resp, req); handle exception thrown Line 314: rpc_->client_->CloseOperation(resp, req); may throw exception Line 330: rpc_->client_->FetchResults(resp, req); may throw exception Line 331: Status status = TStatusToStatus(resp.status); RETURN_NOT_OK(...) ? Line 348: impl->column_ = row_set->columns[i]; I believe this performs a copy of the column in the row set, which must be avoided at all costs http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/hs2session.h File src/hs2client/hs2session.h: Line 61: Status GetFunctions(const std::string& schemaName, std::shared_ptr<Operation>* out); May prefer C++-style variable naming in this methods. Can you add documentation to explain what `schema name` means (i.e. schemas are databases here)? http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/operation.h File src/hs2client/operation.h: Line 46: static const int DEFAULT_MAX_ROWS; Is this intended to be configurable? If yes, state if the user is expected to modify Operation::DEFAULT_MAX_ROWS directly. If no, only declare this in the compilation unit. Line 52: Operation& operator=(Operation const&) = delete; You may want to pull in the gutil DISALLOW_COPY_AND_ASSIGN https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/macros.h#L22 (and put this in private: ) http://gerrit.cloudera.org:8080/#/c/2645/4/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 103: service->Close(); Is there concern about "zombie sessions"? We might be able to apply RAII-style to automatically close sessions in the event of exceptions / errors -- 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: 4 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
