Henry Robinson has posted comments on this change. Change subject: IMPALA-3742: partitions DMLs for Kudu tables ......................................................................
Patch Set 3: (17 comments) Few stylistic comments. http://gerrit.cloudera.org:8080/#/c/6037/4/be/src/runtime/data-stream-partitioner.cc File be/src/runtime/data-stream-partitioner.cc: PS4, Line 62: uduT nullptr Line 66: RETURN_IF_ERROR(CreateKuduClient(table_desc_->kudu_master_addresses(), &client_)); how about using kudu::client here, so you don't have to write it out a further four times? Line 80: for (int i = 0; i < partition_expr_ctxs_.size(); ++i) { override PS4, Line 88: t better var name than 'p' PS4, Line 102: : remove std:: PS4, Line 102: titioner(pool, row_ needs a comment, like all the other members. PS4, Line 104: Statu remove std:: PS4, Line 108: // We can't next_channel_ PS4, Line 119: for (ExprContext* ctx : partition_expr_ctxs_) { PS4, Line 135: remove std:: PS4, Line 144: rn Status::OK(); more idiomatic now to use Substitute() on line 152. PS4, Line 146: auto http://gerrit.cloudera.org:8080/#/c/6037/3/be/src/runtime/data-stream-partitioner.h File be/src/runtime/data-stream-partitioner.h: PS3, Line 38: virtual This is a v-call per row, which might be expensive. Is the idea that codegen will get rid of this eventually? http://gerrit.cloudera.org:8080/#/c/6037/4/be/src/runtime/data-stream-partitioner.h File be/src/runtime/data-stream-partitioner.h: PS4, Line 39: Does a virtual call per row have any measurable effect? Is the idea that codegen will eventually get rid of the overhead? http://gerrit.cloudera.org:8080/#/c/6037/4/be/src/runtime/data-stream-sender.cc File be/src/runtime/data-stream-sender.cc: PS4, Line 390: rtit probably easier to write if (partitioner_) Line 418: } one line Line 480: void DataStreamSender::Close(RuntimeState* state) { one line -- To view, visit http://gerrit.cloudera.org:8080/6037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic10b3295159354888efcde3df76b0edb24161515 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
