impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Date Mon, 24 Oct 2016 05:16:46 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................


Patch Set 1: Code-Review+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?


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?


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 avoid duplication.


-- 
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.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message