Thomas Tauber-Marshall has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 15: (15 comments) http://gerrit.cloudera.org:8080/#/c/2645/15/src/hs2client/operation.cc File src/hs2client/operation.cc: Line 77: column_descs > call clear before this line Done Line 78: for (hs2::TColumnDesc tcolumn_desc: resp.schema.columns) { > const reference here as well Done http://gerrit.cloudera.org:8080/#/c/2645/15/src/hs2client/operation.h File src/hs2client/operation.h: Line 105: explicit > just curious, where did you find this to be necessary? Its just a general piece of advice to make single parameter constructors explicit. I suppose it doesn't really matter for this case. http://gerrit.cloudera.org:8080/#/c/2645/15/src/hs2client/thrift-internal.cc File src/hs2client/thrift-internal.cc: Line 166: return NULL; > let's not return pointers here. Even if we did though, the behavior of this Done Line 178: return new PrimitiveType > At first I thought this was going to be a memory leak, but I see this gets Done http://gerrit.cloudera.org:8080/#/c/2645/15/src/hs2client/types.h File src/hs2client/types.h: Line 40: int int_value; : std::string string_value; : : // True iff this qualifier represents an int value. : bool is_int; : // True iff this qualifier represents an string value. : bool is_string; > all can be const Done Line 89: PrimitiveType > what about just creating subclasses of PrimitiveType and nixing the qualifi Done Line 92: qualifiers > if we keep this, let's have this take a const ref map Done Line 95: TypeId > const Done Line 96: std::map<std::string, TypeQualifier> qualifiers() const { return qualifiers_; } > return a const reference Done Line 100: TypeId > const TypeId Done Line 101: std::map<std::string, TypeQualifier> qualifiers_ > const map Done Line 105: ColumnDesc > (Adding this comment for other readers) As Thomas and I discussed, we can e Done Line 107: ColumnDesc > Yeah, let's make this a class (consistent with ColumnType) then these can a Done Line 111: unique_ptr > I don't think this needs to hold memory allocated on the heap, i.e. this ca Done -- 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: 15 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
