Matthew Jacobs has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 13: (4 comments) I think there were a few more things that we chatted about after you posted rev #13. Here are a few brief comments on what's here (excluding what we already chatted about in person Fri afternoon). http://gerrit.cloudera.org:8080/#/c/2645/13/src/hs2client/columnar-row-set.h File src/hs2client/columnar-row-set.h: Line 67: // Returns the value for the i-th row within this set of data for this column. : T GetData(int i) const { return data()[i]; } Why add this now? I see why it could be useful, but I'm worried it could be misused with string data since this returns by value. Maybe return by const reference, but only if we need this fn. http://gerrit.cloudera.org:8080/#/c/2645/13/src/hs2client/operation.h File src/hs2client/operation.h: Line 67: State Here you might want to use the Operation namespace to be clear in the interface, e.g. "Operation::State* out". Callers that use this code from outside of Operation will need to use that. Line 112: // For access to the ThriftRPC for executing rpcs. I think we chatted about adding GetResultSetMetadata in this review. Then we can remove this I think? http://gerrit.cloudera.org:8080/#/c/2645/13/src/hs2client/thrift-internal.cc File src/hs2client/thrift-internal.cc: Line 104: case hs2::TOperationState::UKNOWN_STATE: : return Operation::State::UKNOWN; lol I love that hive has this spelled wrong, but let's fix it in our enum. -- 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: 13 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
