impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4883: Union Codegen
Date Fri, 31 Mar 2017 23:37:11 GMT
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 <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message