Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201610240516.u9O5GksE018824@ip-10-146-233-104.ec2.internal>
Date: Mon, 24 Oct 2016 05:16:46 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: Alex Behm <alex.behm@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3586=3A_Clean_up_union-node=2Eh/cc_to_enable_improvements=2E=0A?=
In-Reply-To: <gerrit.1477252019000.Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70@gerrit.cloudera.org>
References: <gerrit.1477252019000.Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Mon, 24 Oct 2016 05:16:52 -0000

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