Michael Ho has posted comments on this change. Change subject: IMPALA-4883: Implement Codegen for Union ......................................................................
Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/6459/1/be/src/exec/union-node-ir.cc File be/src/exec/union-node-ir.cc: 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 anything. http://gerrit.cloudera.org:8080/#/c/6459/1/be/src/exec/union-node.cc File be/src/exec/union-node.cc: PS1, Line 40: NULL nullptr 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 nullptr PS1, Line 231: IsNodeCodegenDisabled() Wouldn't that be implied by codegen_union_materialize_expr_fns_.size() ==0 ? PS1, Line 233: codegend_union_materialize_exprs_fns_.data()[child_idx_] == NULL Isn't this the only check we care about ? Are the other two conditions needed ? PS1, Line 237: codegend_union_materialize_exprs_fns_.data()[child_idx_] codegend_union_materialize_exprs_fns_[child_idx] -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
