impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4883: Implement Codegen for Union
Date Thu, 23 Mar 2017 20:12:24 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-4883: Implement Codegen for Union

Patch Set 1:

File be/src/exec/

PS1, Line 28: TupleRow* dst_row = dst_batch->GetRow(dst_batch->AddRow());
Can this be not cross-compiled and we can make the caller pass dst_row directly instead ?

PS1, Line 32: dst_batch->tuple_data_pool()
Can the caller pass dst_batch->tuple_data_pool() to this function instead ? This avoid
compiling code which won't provide much performance boost.

Line 34:   dst_batch->CommitLastRow();
Also don't seem to be strictly needed to be cross-compiled. Please correct me if I misunderstood
File be/src/exec/

PS1, Line 40: NULL

PS1, Line 108: DCHECK(codegen_status.ok());
Don't think it's safe to always assume codegen succeeded. It can fail for legitimate reason
(e.g. we don't handle TYPE_CHAR). Please consider skipping this child expression if codegen
for it failed.

PS1, Line 114: NULL

PS1, Line 231: IsNodeCodegenDisabled()
Wouldn't that be implied by codegen_union_materialize_expr_fns_.size() ==0 ?

PS1, Line 233:[child_idx_] == NULL
Isn't this the only check we care about ? Are the other two conditions needed ?

PS1, Line 237:[child_idx_]

To view, visit
To unsubscribe, visit

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

View raw message