Matthew Jacobs has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 6: (26 comments) Coming together! First pass here, let me know if you have questions. http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/columnar-row-set.cc File src/hs2client/columnar-row-set.cc: Line 37: row_set->columns[i] this is copying columns[i] into column. Should this be a pointer? Line 38: impl For what we see here, I don't actually think you need ColumnImpl or Column::impl_ -- can you just have the XColumn constructors take ColumnarRowSetImpl* and int colIdx, then have those constructors grab what it needs? http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/columnar-row-set.h File src/hs2client/columnar-row-set.h: Line 31: length I think the value set here is wrong. See my comment in the sample-usage about this. http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2service-test.cc File src/hs2client/hs2service-test.cc: Line 45: coon connect Line 52: connection service? Line 55: EXPECT_OK(service->Close()); Why is Close() a no-op on this call but not on l58? http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2service.cc File src/hs2client/hs2service.cc: Line 66: try { : impl_->transport->close(); : } catch (TException& tx) {} just call Close()? Line 76: isOpen Are we sure this can't throw? http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2service.h File src/hs2client/hs2service.h: Line 76: Close What happens if you call this on a closed session -- no-op or error? Line 78: Reconnect Do we really need this functionality, or could we ask users to just create a new HS2Service via Connect? http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2session-test.cc File src/hs2client/hs2session-test.cc: Line 44: EXPECT_OK(session_ok->ExecuteStatement("select * from "+ TEST_TBL, &select_op)); nit: space Line 55: "+ nit space http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2session.cc File src/hs2client/hs2session.cc: Line 30: _ consistent naming with underscores please, I think we don't use them trailing in structs Line 40: maybe return immediately if !open_ ? http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/hs2session.h File src/hs2client/hs2session.h: Line 40: Close can be called twice? http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/macros.h File src/hs2client/macros.h: Line 19: DISALLOW_COPY_AND_ASSIGN we shouldn't let this macro leak into the public headers (this file is included by public headers) because this is a commonly defined macro. code that uses this library and gutil (or something else that defines a macro with this name) will fail to compile (macros can't be defined twice). For now maybe just prefix this with HS2CLIENT_. Line 24: if (!status.ok()) { \ : return status; \ : } This can introduce bugs. Can you copy the implementation of Impala's RETURN_IF_ERROR? (It's worth reading up a bit more about the perils of macros as well). http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 21: TCLIService why is this needed? this is a thrift header Line 50: CHECK_OK(status); I actually don't think we want to be using this macro everywhere (here and below). This sample use case should demonstrate that we need to clean up as well. E.g. check if there is an error, close the session, and then return. Line 56: CHECK_OK(status); Here you need to close the session. Line 60: CHECK_OK here you would close the execute_op and then the session. Line 66: int_col->length I think this is wrong. length seems to be the number of elements in the data array, which means its non-null elements. If you have a batch of all nulls, this will be 0. I assume you wanted length to be both nulls and non nulls? Of course that's the number of rows, which should be the same across all columns, and its unfortunate to have to have the same information duplicated on the columns as it's kind of confusing for the user to decide which to check and/or if they're different (I don't think they should be-- right?). Then for clarity in this sample usage code, I think it'd be better to always be using the length from the same column (but with an assert that int_col->length == string_col->length. That makes it more clear to the user. Line 68: auto would you mind using the real types here for clarity? Line 86: Fetch please make sure that the Fetch() API documents how this works with end-of-stream. Line 87: CHECK_OK same http://gerrit.cloudera.org:8080/#/c/2645/6/src/hs2client/test-util.h File src/hs2client/test-util.h: Line 37: Get I don't see a method "Get()" -- 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 <[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
