impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wes McKinney (Code Review)" <ger...@cloudera.org>
Subject [hs2client-CR] 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).
Date Fri, 03 Jun 2016 18:47:43 GMT
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 <wesm@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Wes McKinney <wesm@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message