impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wes McKinney (Code Review)" <>
Subject [hs2client-CR] Initial structure for the C++ hiveserver2 client.
Date Fri, 01 Apr 2016 06:42:16 GMT
Wes McKinney has posted comments on this change.

Change subject: Initial structure for the C++ hiveserver2 client.

Patch Set 4:

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)
File src/hs2client/

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 (
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);

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,

Line 241:     rpc_->client_->ExecuteStatement(resp, req);
failure possible?

Line 255:     const HS2ClientConfig& confOverlay, shared_ptr<Operation>* out) {

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

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
File src/hs2client/hs2session.h:

Line 61:   Status GetFunctions(const std::string& schemaName, std::shared_ptr<Operation>*
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)?
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
(and put this in private: )
File src/hs2client/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7be82d5897ad6093116915b17924674c4ae508a0
Gerrit-PatchSet: 4
Gerrit-Project: hs2client
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Wes McKinney <>
Gerrit-HasComments: Yes

View raw message