Alex Behm has posted comments on this change. Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements. ......................................................................
Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/4817/1/be/src/exec/union-node.cc File be/src/exec/union-node.cc: PS1, Line 72: for (int i = 0; i < const_expr_lists_.size(); ++i) { : RETURN_IF_ERROR(Expr::Prepare( : const_expr_lists_[i], state, row_desc(), expr_mem_tracker())); : AddExprCtxsToFree(const_expr_lists_[i]); : DCHECK_EQ(const_expr_lists_[i].size(), tuple_desc_->slots().size()); : } : : // Prepare result expr lists. : for (int i = 0; i < child_expr_lists_.size(); ++i) { : RETURN_IF_ERROR(Expr::Prepare( : child_expr_lists_[i], state, child(i)->row_desc(), expr_mem_tracker())); : AddExprCtxsToFree(child_expr_lists_[i]); : DCHECK_EQ(child_expr_lists_[i].size(), tuple_desc_->slots().size()); : } > ranged-for loops as well? Done for the first loop. The second loop needs child(i)->row_desc(). PS1, Line 220: for (int i = 0; i < const_expr_lists_.size(); ++i) { : Expr::Close(const_expr_lists_[i], state); : } : for (int i = 0; i < child_expr_lists_.size(); ++i) { : Expr::Close(child_expr_lists_[i], state); : } > While you're here, can you rewrite as two ranged-for loops? Done http://gerrit.cloudera.org:8080/#/c/4817/1/be/src/exec/union-node.h File be/src/exec/union-node.h: Line 58: /// Const exprs materialized by this node. These exprs don't refer to any children. > Add that these are only materialized by the first fragment instance to avoi Done -- To view, visit http://gerrit.cloudera.org:8080/4817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-HasComments: Yes
