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] Initial structure for the C++ hiveserver2 client.
Date Mon, 18 Apr 2016 22:28:13 GMT
Matthew Jacobs has posted comments on this change.

Change subject: Initial structure for the C++ hiveserver2 client.
......................................................................


Patch Set 6:

(12 comments)

Here's a bit more and I have a few more files to review later.

http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/operation-test.cc
File src/hs2client/operation-test.cc:

Line 60:   EXPECT_TRUE(FindRow(1, "a", int_col.get(), string_col.get()));
       :   EXPECT_TRUE(FindRow(2, "b", int_col.get(), string_col.get()));
       :   EXPECT_FALSE(FindRow(3, int_col.get()));
       :   EXPECT_FALSE(FindRow(4, int_col.get()));
This is fine, but the results are ordered so you should be able to check exactly the right
position.


http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/operation.cc
File src/hs2client/operation.cc:

Line 36: 512
is this from somewhere else? in our code we often pick 1024 so if this is just random lets
stick with 1024.


Line 90: row_set_impl->row_set = resp.results
This involves a memory copy of the data. A few ways around this but maybe the easiest is to
put a hs2::TFetchResultsResp on  ColumnarRowSetImpl and then have FetchResults take that resp.
You have to do the new  ColRowSetImpl above l85 of course.


Line 112: PrintResults
Let's see if we can move PrintResults to test code or at least some utility class.


Line 113: namespace {
?


http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/operation.h
File src/hs2client/operation.h:

Line 28: enum
I think we can use "enum class" here.


Line 28: enum FetchOrientation {
       :   FETCH_NEXT,
       :   FETCH_PRIOR,
       :   FETCH_RELATIVE,
       :   FETCH_ABSOLUTE,
       :   FETCH_FIRST,
       :   FETCH_LAST
       : };
       : 
       : enum OperationState {
       :   INITIALIZED_STATE,
       :   RUNNING_STATE,
       :   FINISHED_STATE,
       :   CANCELED_STATE,
       :   CLOSED_STATE,
       :   ERROR_STATE,
       :   UKNOWN_STATE,
       :   PENDING_STATE,
       : };
Are these just mirroring the definitions in hs2? Can you add a comment. Can you also note
(inline) which ones are supported and which ones are not?


Line 59: GetState
Brief comment. When is this valid to call?


Line 63: Wait
Is this necessary? Seems like a useful function for testing, but I don't know if we need to
provide this in the public API.


Line 66: GetLog
Brief comment, when is this valid to call?


Line 68: GetProfile
same here and the other actions below


Line 84: PrintResults
Is this for debug/testing purposes? It may be hard to ensure this is generally useful as it's
not clear how this will be formatted. Also what if the data set is large? Maybe we can move
this and other utility-like functions to test code or if we want it public then perhaps a
utility class.


-- 
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: 6
Gerrit-Project: hs2client
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Wes McKinney <wes@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message