Thomas Tauber-Marshall has posted comments on this change. Change subject: Implemented a framework for the C++ hiveserver2 client. ......................................................................
Patch Set 3: (18 comments) As mentioned, the implementations of the service, session, operation, and columar row set were moved into hs2client.cc to facilitate passing Thrift objects between them. http://gerrit.cloudera.org:8080/#/c/2645/3//COMMIT_MSG Commit Message: Line 7: Implemented a framework > Maybe "Initial structure for the hiveserver2 client"? Done 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? Like I mentioned briefly, the idea here is that this is a bit array that represents which values for this column are null. This makes it easier to do things like differentiate between null strings and empty strings. The reason its a void* is because this is what Thrift actually gives us in the TColumn. I added an IsNull function that does the bit arithmetic. Line 28: protected > if we need non-public access then this should be a class rather than a stru Done Line 37: ColumnImpl > Can you add the definition of this? It's hard to reason about the columns w The definition is in hs2client.cc 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 v Done Line 38: map > const map<>& Done Line 83: . > can you add something like: "Called by Connect() before returned to the use Done http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/hs2session.h File src/hs2client/hs2session.h: Line 29: * the OpenSession RPC and uses it to create and return operations. > This should document the lifecycle, e.g. Done http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/operation.h File src/hs2client/operation.h: Line 80: int > If this is a class constant typically its static const & the declaration go Done Line 80: 10 > why 10? Not sure what a good value here would be, though 10 is presumably too low. Any suggestions? http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 26: CHECK_OK > why not just make a function? Defining macros for checking statuses like this is fairly common practice, for example I basically took this from Arrow's status implementation and Impala has something similar, but I don't have a strong opinion on whether this is better than just a function. Also, this is defined here and not in status.h because its not clear to me how to make it useful to users of this library, since I'm just printing to std::cout whereas a real application would presumably have its own logging infrastructure. For now, its just making this file a little cleaner. Line 62: > > most likely you'd want this to be the refernce, otherwise you're copying al Done Line 62: 1 > can you change the query to make it more clear that col 1 is a string, e.g. Done Line 63: string > const string& s: Done Line 63: > nit: we don't use spaces here Done Line 69: &op > This might be confusing. Even though GetTables should reset the ptr, we sho Done Line 72: row_set > same for the row batch Done http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/status.h File src/hs2client/status.h: Line 42: ToString > It's not clear if this is just the msg or also something else. I see in the Done -- 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
