impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
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:

File be/src/exec/

Line 19: #include "runtime/tuple.h"
> Do we need tuple.h? I don't think I see any references to Tuple* in here.

Line 21: #include "util/runtime-profile-counters.h"
> Is this needed still?

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.
File be/src/exec/

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
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

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.

PS5, Line 128: row_batch
> dst_batch.

Line 136:   void IR_ALWAYS_INLINE MaterializeExprs(const std::vector<ExprContext*>&
> Move this to the 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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4107d27582ff5416172810364a6e76d3d93c439
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message