impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <>
Subject [hs2client-CR] Implemented a framework for the C++ hiveserver2 client.
Date Fri, 01 Apr 2016 00:11:38 GMT
Thomas Tauber-Marshall has posted comments on this change.

Change subject: Implemented a framework for the C++ hiveserver2 client.

Patch Set 3:


As mentioned, the implementations of the service, session, operation, and columar row set
were moved into to facilitate passing Thrift objects between them.
Commit Message:

Line 7: Implemented a framework
> Maybe "Initial structure for the hiveserver2 client"?
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

Line 37: ColumnImpl
> Can you add the definition of this? It's hard to reason about the columns w
The definition is in
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

Line 38: map
> const map<>&

Line 83: .
> can you add something like: "Called by Connect() before returned to the use
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.
File src/hs2client/operation.h:

Line 80: int
> If this is a class constant typically its static const & the declaration go

Line 80: 10
> why 10?
Not sure what a good value here would be, though 10 is presumably too low. Any suggestions?
File src/hs2client/

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

Line 62: 1
> can you change the query to make it more clear that col 1 is a string, e.g.

Line 63: string
> const string& s:

Line 63:  
> nit: we don't use spaces here

Line 69: &op
> This might be confusing. Even though GetTables should reset the ptr, we sho

Line 72: row_set
> same for the row batch
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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7be82d5897ad6093116915b17924674c4ae508a0
Gerrit-PatchSet: 3
Gerrit-Project: hs2client
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Wes McKinney <>
Gerrit-HasComments: Yes

View raw message