Matthew Jacobs has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 15: (14 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 column_descs->clear() Line 78: for (hs2::TColumnDesc tcolumn_desc: resp.schema.columns) { const reference here as well const hs2::TColumnDesc& 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? 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 I think isn't well defined. What will happen at runtime if this NULL ends up in a ColumnDesc? Consider just returning INVALID, or maybe making the return val an out parameter and returning a boolean/status. I think the former is fine, but make sure the fn documentation talks about this. Line 178: return new PrimitiveType At first I thought this was going to be a memory leak, but I see this gets dropped into a unique_ptr in a column_desc. let's not return a naked pointer here since this method could be called by anyone. I think it's scary to see a new where the resulting ptr gets passed around directly (i.e. not wrapped in a shared/unique ptr). I think its fine to return these by value since they're pretty small. http://gerrit.cloudera.org:8080/#/c/2645/15/src/hs2client/types.h File src/hs2client/types.h: Line 89: PrimitiveType what about just creating subclasses of PrimitiveType and nixing the qualifier maps? We should check with Wes if he thinks that's going to be a pain for a python wrapper, but I think it's more C++/object oriented. E.g. we could have 2 subclasses: one for the char/varchar (we can think of a name), and another for decimal. Each would just have a field for their extra info. Let's chat about this tomorrow. Line 92: qualifiers if we keep this, let's have this take a const ref map Line 95: TypeId const Line 96: std::map<std::string, TypeQualifier> qualifiers() const { return qualifiers_; } return a const reference Line 100: TypeId const TypeId Line 101: std::map<std::string, TypeQualifier> qualifiers_ const map Line 107: ColumnDesc can you give the parameters better names? Too hard to make sense of like this. Let's change the members to end in _ if you need to. Line 107: ColumnType* let's avoid this heap allocation Line 111: unique_ptr I don't think this needs to hold memory allocated on the heap, i.e. this can hold a "const ColumnType type;" -- 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
