Matthew Jacobs has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 16: (4 comments) Thanks! Almost there, after these small things I'm good to go. http://gerrit.cloudera.org:8080/#/c/2645/16/src/hs2client/thrift-internal.cc File src/hs2client/thrift-internal.cc: Line 178: ttype_desc.types[0].primitiveEntry.typeQualifiers.qualifiers maybe pull this out above this line to a local variable (by reference) to make this a bit more readable here and on l180. Line 178: at at() may throw an exception, which we don't do in our C++. Probably just assert/DCHECK before this and then use the [] operator to index in (which doesn't check/throw). same for l180 and l174. http://gerrit.cloudera.org:8080/#/c/2645/16/src/hs2client/thrift-internal.h File src/hs2client/thrift-internal.h: Line 58: // Converts a TTypeDesc to a ColumnType. Currently only primitive types are supported. please mention why this returns a heap allocated object (polymorphism). http://gerrit.cloudera.org:8080/#/c/2645/16/src/hs2client/types.h File src/hs2client/types.h: Line 61: : virtual bool IsCharacterType() const { return false; } : virtual bool IsDecimalType() const { return false; } : virtual bool IsPrimitiveType() const { return false; } I see why these are here, but I think we should avoid having to put things specific to child classes on the base classes when avoidable. I think it'd be cleaner to just have the DCHECKs that use these compare type_id() directly. We can always add more to the interface later if we find it useful... -- 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: 16 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
