Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-4883: Union Codegen ......................................................................
Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/6459/5/be/src/exec/union-node-ir.cc File be/src/exec/union-node-ir.cc: Line 19: #include "runtime/tuple.h" > Do we need tuple.h? I don't think I see any references to Tuple* in here. Done Line 21: #include "util/runtime-profile-counters.h" > Is this needed still? Done Line 35: while (!dst_batch->AtCapacity() && child_row_idx < child_batch->num_rows()) { > Nice! We can maybe avoid a few more loads and stores via the child_batch an Great suggestions. Done. Line 46: if (limit_ != -1 && num_rows_returned_ + dst_batch->num_rows() > limit_) { > We don't need to cross-compile this logic. Let's move it into the caller an Good point. Moved it out of here. http://gerrit.cloudera.org:8080/#/c/6459/5/be/src/exec/union-node.cc File be/src/exec/union-node.cc: Line 168: if (limit_ != -1 && num_rows_returned_ + row_batch->num_rows() > limit_) { > How about we move this logic around num_rows_returned_ and limit_ into GetN Done http://gerrit.cloudera.org:8080/#/c/6459/5/be/src/exec/union-node.h File be/src/exec/union-node.h: Line 28: #include "runtime/tuple.h" > Do we need the tuple.h and tuple-row.h imports? Oh I guess for the inline M Done Line 72: /// each GetNext() call. > We should add a TODO to remove this. Maybe Michael knows if there's a JIRA Added a todo, couldn't find the relevant JIRA. PS5, Line 99: Null > NULL here and below, just for consistency with other comments. Done PS5, Line 128: row_batch > dst_batch. Done Line 136: void IR_ALWAYS_INLINE MaterializeExprs(const std::vector<ExprContext*>& exprs, > Move this to the -ir.cc file? I don't think there's a reason we need to def Ok, moved it. Line 148: bool inline IsChildPassthrough(int child_idx) const { > I don't think any of the "inline" specifiers here and below do anything - i Makes sense, removed all of them. -- To view, visit http://gerrit.cloudera.org:8080/6459 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib4107d27582ff5416172810364a6e76d3d93c439 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
