Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-4883: Implement Codegen for Union ......................................................................
Patch Set 1: (10 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 dire Done PS1, Line 32: dst_batch->tuple_data_pool() > Can the caller pass dst_batch->tuple_data_pool() to this function instead ? Done Line 34: dst_batch->CommitLastRow(); > Also don't seem to be strictly needed to be cross-compiled. Please correct Done 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 Done PS1, Line 108: DCHECK(codegen_status.ok()); > Don't think it's safe to always assume codegen succeeded. It can fail for l Done PS1, Line 114: NULL > nullptr Done PS1, Line 231: IsNodeCodegenDisabled() > Wouldn't that be implied by codegen_union_materialize_expr_fns_.size() ==0 Done 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 need I rewrote this a little bit. I think we need at least 2 checks: that the size is not zero and that the current child is not null. PS1, Line 237: codegend_union_materialize_exprs_fns_.data()[child_idx_] > codegend_union_materialize_exprs_fns_[child_idx] Done http://gerrit.cloudera.org:8080/#/c/6459/1/be/src/exec/union-node.h File be/src/exec/union-node.h: PS1, Line 71: tuple_pool_; > FWIW, this is probably not needed in the future as we avoid baking pointers Ok, this can be removed in the future. -- 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: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
