impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [hs2client-CR] Initial structure for the C++ hiveserver2 client.
Date Mon, 09 May 2016 19:50:55 GMT
Matthew Jacobs has posted comments on this change.

Change subject: Initial structure for the C++ hiveserver2 client.
......................................................................


Patch Set 13:

(4 comments)

I think there were a few more things that we chatted about after you posted rev #13. Here
are a few brief comments on what's here (excluding what we already chatted about in person
Fri afternoon).

http://gerrit.cloudera.org:8080/#/c/2645/13/src/hs2client/columnar-row-set.h
File src/hs2client/columnar-row-set.h:

Line 67:   // Returns the value for the i-th row within this set of data for this column.
       :   T GetData(int i) const { return data()[i]; }
Why add this now? I see why it could be useful, but I'm worried it could be misused with string
data since this returns by value. Maybe return by const reference, but only if we need this
fn.


http://gerrit.cloudera.org:8080/#/c/2645/13/src/hs2client/operation.h
File src/hs2client/operation.h:

Line 67: State
Here you might want to use the Operation namespace to be clear in the interface, e.g. "Operation::State*
out". Callers that use this code from outside of Operation will need to use that.


Line 112:   // For access to the ThriftRPC for executing rpcs.
I think we chatted about adding GetResultSetMetadata in this review. Then we can remove this
I think?


http://gerrit.cloudera.org:8080/#/c/2645/13/src/hs2client/thrift-internal.cc
File src/hs2client/thrift-internal.cc:

Line 104:     case hs2::TOperationState::UKNOWN_STATE:
        :       return Operation::State::UKNOWN;
lol I love that hive has this spelled wrong, but let's fix it in our enum.


-- 
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: 13
Gerrit-Project: hs2client
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Wes McKinney <wes@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message