Marcel Kornacker has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 12: (17 comments) http://gerrit.cloudera.org:8080/#/c/2645/12/src/hs2client/columnar-row-set.h File src/hs2client/columnar-row-set.h: Line 54: virtual const uint8_t* nulls() const = 0; let's stick with impala naming conventions Line 62: TypedColumn(const uint8_t* nulls, const std::vector<T>* data) does this need to be public? http://gerrit.cloudera.org:8080/#/c/2645/12/src/hs2client/fetch-results.h File src/hs2client/fetch-results.h: Line 25: // Row oriented results are not currently supported. do we plan on supporting this in the future? (and if so, why?) http://gerrit.cloudera.org:8080/#/c/2645/12/src/hs2client/hs2service.h File src/hs2client/hs2service.h: Line 65: // HS2Service objects are created using HS2Service::Connect. They must function references end in () Line 76: class HS2Service { what was the rationale for prefixing this with 'HS2' (and session) but not doing that for Operation (and both are in the hs2client namespace, so it's somewhat redundant anyway)? Line 109: Status OpenSession(const std::string& user, const HS2ClientConfig& config, is 'user' sufficient as an authentication mechanism? http://gerrit.cloudera.org:8080/#/c/2645/12/src/hs2client/hs2session.h File src/hs2client/hs2session.h: Line 48: std::shared_ptr<Operation>* operation) const; the class comment for Operation says it's not thread-safe, so why return this as a shared_ptr? Line 55: Status GetSchemas(std::shared_ptr<Operation>* operation) const; what exactly does this do? (what is role of operation?) Line 59: Status GetTables(std::shared_ptr<Operation>* operation) const; instead of returning the results of these metadata requests as generic operations, i think it's a lot easier if you created custom structs for these and converted the generic result format that comes back from hs2 into those structs. the reason is that the way it's not, a user also needs to understand the hs2 protocol in order to make sense of the returned data. you can leave that as a todo if this adds too much work now, but i think that would make this interface self-contained and a lot more usable. http://gerrit.cloudera.org:8080/#/c/2645/12/src/hs2client/operation.h File src/hs2client/operation.h: Line 40: enum class OperationState { move into class Operation and drop 'Operation' from name? Line 41: INITIALIZED_STATE, embedding the type name in the enum constants is common practice in c to disambiguate enums. for an enum class, that's not necessary anymore. Line 52: // and to retrieve its results. how do you get result set metadata? (the GetResultSetMetadata rpc) Line 73: // Fetches a batch of results, stores them in 'results', and sets has_more_rows. you should point out in the function comments whether a function blocks. i'd expect this one to do that. Line 104: // True if this operation has begun execution and has not been closed yet. how does this related to OperationState? http://gerrit.cloudera.org:8080/#/c/2645/12/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 85: total_retrieved += int_col->length(); it feels like the number of rows should also be part of FetchResults Line 87: int32_t int_val = int_col->data()[i]; why not also add a GetData(int), so you don't always have to deal with the array view of data? http://gerrit.cloudera.org:8080/#/c/2645/12/src/hs2client/util.cc File src/hs2client/util.cc: Line 101: hs2::TGetResultSetMetadataReq req; i think this part of hs2 functionality should be exposed in a consumable way by the public interface. -- 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: 12 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
