impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [hs2client-CR] Implemented a framework for the C++ hiveserver2 client.
Date Wed, 30 Mar 2016 05:44:26 GMT
Matthew Jacobs has posted comments on this change.

Change subject: Implemented a framework for the C++ hiveserver2 client.
......................................................................


Patch Set 3:

(9 comments)

Thanks! I still need to look a bit more in the morning.

http://gerrit.cloudera.org:8080/#/c/2645/3//COMMIT_MSG
Commit Message:

Line 7: Implemented a framework
Maybe "Initial structure for the hiveserver2 client"?


http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/columnar-row-set.h
File src/hs2client/columnar-row-set.h:

Line 26: const void* nulls
how does this work?


Line 28: protected
if we need non-public access then this should be a class rather than a struct


Line 37: ColumnImpl
Can you add the definition of this? It's hard to reason about the columns without the .cc
as well.


http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/hs2service.h
File src/hs2client/hs2service.h:

Line 36: GetOption
what happens if key isn't in the map? maybe better to return bool and the value as an out
parameter.


Line 38: map
const map<>&


Line 83: .
can you add something like: "Called by Connect() before returned to the user."?


http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/operation.h
File src/hs2client/operation.h:

Line 80: 10
why 10?


Line 80: int
If this is a class constant typically its static const & the declaration goes in the header
but the definition goes in the .cc, e.g.:

 
static const int DEFAULT_MAX_ROWS; // in the .h
 
Operation::DEFAULT_MAX_ROWS = 1024; // in the .cc


-- 
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: 3
Gerrit-Project: hs2client
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Wes McKinney <wes@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message