Wes McKinney has posted comments on this change. Change subject: Rough draft of Cython interface to libhs2client and start of DB API 2.0 compliance layer (for compatibility with other systems that expect the standard Python database driver interface). ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/3296/2/src/hs2client/columnar-row-set.cc File src/hs2client/columnar-row-set.cc: Line 52: > nit: #undef VALUE_GETTER Done (is this generally preferred even when defines have no visibility outside a particular .cc file?) PS2, Line 56: columns[i] > It might be worth adding a DCHECK to make sure i < columns.size() Done Line 73: > nit: #undef TYPED_GETTER Done http://gerrit.cloudera.org:8080/#/c/3296/2/src/hs2client/columnar-row-set.h File src/hs2client/columnar-row-set.h: PS2, Line 56: std::string* > I'm fine with this, but just curious why switch from uint8_t*? See https://github.com/cloudera/hs2client/issues/16. Some versions of Hive truncate the bitmap, so there may be fewer bits available than there are values. So we will need the bitmap size to be able to work around this problem. PS2, Line 71: nulls_->c_str() > Are we sure this won't incur a virtual function call? May be unexpectedly e Reverting to uint8_t* for nulls (otherwise you have to do the reinterpret_cast everywhere you want the bitmap as const uint8_t*) and also adding a member for the length to make things more explicit (+ a comment explaining). -- To view, visit http://gerrit.cloudera.org:8080/3296 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c26be5932548d096088aa62bde0fe03e85a9a1e Gerrit-PatchSet: 2 Gerrit-Project: hs2client Gerrit-Branch: master Gerrit-Owner: Wes McKinney <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Wes McKinney <[email protected]> Gerrit-HasComments: Yes
