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 Fri, 22 Apr 2016 22:49:52 GMT
Matthew Jacobs has posted comments on this change.

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


Patch Set 8:

(7 comments)

Not a full pass over this changeset, but I wanted to get the columnar row changes to you since
that's non-trivial.

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

Line 110: assert(impl);
any reason string is the only one to have this assert? also were you going to switch to using
DCHECKs?


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

Line 27:   
nit: no tab, here and below. I think you can remove this if you look at my gist linked below.


Line 61: class
Per our discussion, we can make the Column classes more concise by using templates. We should
also encapsulate the fields.

I started playing with this here: https://gist.github.com/mjacobs/126315408b2df201c24ee677ad8b3b08


Line 61: Column
Please add a good comment here explaining how this is used, including the data layout, and
give a very brief example.


Line 63: ~
FYI destructors need to be virtual if there will ever be subclasses. In this case I don't
think you need any destructor defined.


Line 72:  protected:
       :   // Hides Thrift objects from the header.
       :   struct ColumnImpl;
       : 
       :   // For access to the c'tor.
       :   friend class ColumnarRowSet;
No longer necessary


http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/hs2service.cc
File src/hs2client/hs2service.cc:

Line 23: logging
this file doesn't exist in the review


-- 
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: 8
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