[hs2client-CR] Implemented a framework for the C++ hiveserver2 client.

["Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>]

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 <tm...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Wes McKinney <we...@cloudera.com>
Gerrit-HasComments: Yes