Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201701202106.v0KL6JpM024650@ip-10-146-233-104.ec2.internal>
Date: Fri, 20 Jan 2017 21:06:18 +0000
From: "Michael Ho (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Marcel Kornacker <marcel@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: kwho@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4192=3A_Disentangle_Expr_and_ExprContext=0A?=
In-Reply-To: <gerrit.1481587970000.Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3@gerrit.cloudera.org>
References: <gerrit.1481587970000.Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3@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: Fri, 20 Jan 2017 21:06:30 -0000

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5483

to look at the new patch set (#5).

Change subject: IMPALA-4192: Disentangle Expr and ExprContext
......................................................................

IMPALA-4192: Disentangle Expr and ExprContext

This change cleans up some entangled logic between Expr and
ExprContext's initialization. In general, we stored static
states of an expression in Expr and thread-private / evaluation
related states in ExprContext.

This change cleans up the followings:

1. FunctionContext should be managed by ExprContext. They reside
   in a vector inside ExprContext. When creating the expression tree,
   each sub-expression requiring a FunctionContext will be assigned
   an index into the vector. The index is stoed in the Expr object.
   ExprContext::Prepare() will allocate and prepare the FunctionContext
   for all sub-expressions in the expression tree.

2. Move the field 'output_scale_' to ExprContext. This field is needed
   to format the output in the table writer correctly for the case of
   RoundUpTo(). However, this field can only be derived much later in
   the initialization so in the spirit of not modifying the Expr after
   it has been initialized, let's move this field out to ExprContext
   until IMPALA-4743 and IMPALA-4720 are fixed.

3. Rename Expr::Prepare(), Expr::Open() and Expr::Close() to
   ExprContext class instead as they really operate solely on
   ExprContext but not Expr.

4. Expr::CreateExprTree() and friends return Expr instead of
   ExprContext.

This clean up is a step towards sharing expression states among plan
fragment instances.

This change also fixes a bug in Expr::GetFnContextError().
The old code only returned the error in the top level
FunctionContext, ignoring the error set by its descendants.

Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exec/aggregation-node.cc
M be/src/exec/analytic-eval-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/hash-join-node.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-table-sink.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/kudu-table-sink.cc
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/old-hash-table-test.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/plan-root-sink.cc
M be/src/exec/sort-exec-exprs.cc
M be/src/exec/sort-node.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node.cc
M be/src/exec/unnest-node.cc
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-context.cc
M be/src/exprs/expr-context.h
M be/src/exprs/expr-test.cc
M be/src/exprs/expr.cc
M be/src/exprs/expr.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/row-batch.h
M be/src/service/fe-support.cc
M be/src/service/impalad-main.cc
M common/thrift/ImpalaInternalService.thrift
50 files changed, 726 insertions(+), 617 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/5483/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5483
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>