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 <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Wes McKinney <[email protected]> Gerrit-HasComments: Yes
