impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (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 16:26:39 GMT
Matthew Jacobs 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:

(6 comments)

(Just a review of the C++ code)
Looks good. I think the refactoring makes sense, just a few small nits/questions. Thanks for
fixing the header guards.

http://gerrit.cloudera.org:8080/#/c/3296/2/src/hs2client/columnar-row-set.cc
File src/hs2client/columnar-row-set.cc:

PS2, Line 33: ATTR
nit: ATTR_NAME ?


Line 52: 
nit: #undef VALUE_GETTER


PS2, Line 56: columns[i]
It might be worth adding a DCHECK to make sure i < columns.size()


Line 73: 
nit: #undef TYPED_GETTER


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*?


PS2, Line 71: nulls_->c_str()
Are we sure this won't incur a virtual function call? May be unexpectedly expensive if IsNull()
is called in a loop.


-- 
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