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