Wes McKinney has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/2645/6/CMakeLists.txt File CMakeLists.txt: Line 253: src/hs2client/operation-test.cc I don't think you need to put these here -- the test files are compiled as part of ADD_HS2CLIENT_TEST http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/columnar-row-set.h File src/hs2client/columnar-row-set.h: Line 35: return (*(nulls + (i / 8)) & (1 << (i % 8))) != 0; nulls[i / 8] http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2service-test.cc File src/hs2client/hs2service-test.cc: Line 32: ProtocolVersion protocol_version = ProtocolVersion::HS2CLIENT_PROTOCOL_V7; We'll want to test more protocols eventually http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2service.cc File src/hs2client/hs2service.cc: Line 56: assert(!IsConnected()); Maybe DCHECK here? You can lift the simplified (do not depend on glog) DCHECK macros here if you want: https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/logging.h http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2session.h File src/hs2client/hs2session.h: Line 48: Status GetSchemas(const std::string& schema_name, std::shared_ptr<Operation>* out); In this new function, schema_name can be a wildcard, is that right? If so, can you add a comment and change the argument name? Line 59: Status GetFunctions(const std::string& schema_name, std::shared_ptr<Operation>* out); Same question with these others. Do they accept wildcards (e.g. "foo_*") http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/macros.h File src/hs2client/macros.h: Line 24: if (!status.ok()) { \ : return status; \ : } > This can introduce bugs. Can you copy the implementation of Impala's RETURN Yeah, in general you should wrap all multi-line macros in do {... } while(0). This has the dual effect of making macros function-like (you can always put a semicolon after) and isolating scope effects of variables used inside the macro. I can't remember the technical term for this methodology. http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/operation.cc File src/hs2client/operation.cc: Line 31: using std::vector; I think it's better to use std members explicitly most of the time (unlike Java-ish languages, where you import everything you use at the top). Lifting shared_ptr and unique_ptr is okay but I would rather avoid using declarations for the others unless it's a) test code or b) eliminating a huge amount of verbosity Line 112: PrintResults > Let's see if we can move PrintResults to test code or at least some utility +1 http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/operation.h File src/hs2client/operation.h: Line 84: PrintResults > Is this for debug/testing purposes? It may be hard to ensure this is genera Agree -- I think we can leave this to users of the library to implement themselves, or offer it for debugging purposes (with no guarantee of API stability or behavior stability). We don't want people depending on the output of this function. If we're going to put it in the public API, it would be better to pass in an std::ostream& -- returning std::string makes the API somewhat rigid. http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 21: TCLIService > why is this needed? this is a thrift header I would even go so far as to add a unit test exhibiting that Thrift is not included by accident in the public API. See: https://github.com/apache/parquet-cpp/blob/master/src/parquet/public-api-test.cc#L26 http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/thrift-internal.h File src/hs2client/thrift-internal.h: Line 74: > extra space I can add automated clang-tidy style cleaning in a follow up patch. -- 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: 6 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
