Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201605091950.u49JotWF015375@ip-10-146-233-104.ec2.internal>
Date: Mon, 9 May 2016 19:50:55 +0000
From: "Matthew Jacobs (Code Review)" <gerrit@cloudera.org>
To: Thomas Tauber-Marshall <tmarshall@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Marcel Kornacker <marcel@cloudera.com>, Wes McKinney <wes@cloudera.com>
Reply-To: mj@cloudera.com
Subject: =?UTF-8?Q?[hs2client-CR]_Initial_structure_for_the_C++_hiveserver2_client.=0A?=
In-Reply-To: <gerrit.1459192143000.I7be82d5897ad6093116915b17924674c4ae508a0@gerrit.cloudera.org>
References: <gerrit.1459192143000.I7be82d5897ad6093116915b17924674c4ae508a0@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.10-rc0
archived-at: Mon, 09 May 2016 19:51:02 -0000

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