Mailing-List: contact commits-help@impala.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: kwho@apache.org
To: commits@impala.incubator.apache.org
Date: Fri, 17 Nov 2017 02:55:46 -0000
Message-Id: <75bb4f15055646408839f1d50c524c4f@git.apache.org>
In-Reply-To: <6539d01721cd48338f88b0b7c0d3c224@git.apache.org>
References: <6539d01721cd48338f88b0b7c0d3c224@git.apache.org>
Subject: [3/3] incubator-impala git commit: IMPALA-6184: Clean up after
 ScalarExprEvaluator::Clone() fails
archived-at: Fri, 17 Nov 2017 02:55:50 -0000

IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails

When ScalarExprEvaluator::Clone() fails, the newly created evaluator was
not added to the output vector. This makes it impossible for callers to
close and clean up the evaluators afterwards. This change fixes this by
always adding the newly created evaluator to the output vector before
checking for the error status.

This path is only exercised in the scanner code. Two new tests are added
to exercise the failure paths.

Testing done: newly added tests in udf-errors.test

Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
Reviewed-on: http://gerrit.cloudera.org:8080/8572
Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/3ddafcd2
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/3ddafcd2
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/3ddafcd2

Branch: refs/heads/master
Commit: 3ddafcd29505614a01c8f4362396635c84ab4052
Parents: c886f21
Author: Michael Ho <kwho@cloudera.com>
Authored: Wed Nov 15 13:40:36 2017 -0800
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
Committed: Fri Nov 17 02:24:48 2017 +0000

----------------------------------------------------------------------
 be/src/exprs/scalar-expr-evaluator.cc           |  6 ++-
 be/src/testutil/test-udfs.cc                    | 49 ++++++++++++++++++++
 .../queries/QueryTest/udf-errors.test           | 36 ++++++++++++++
 3 files changed, 89 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3ddafcd2/be/src/exprs/scalar-expr-evaluator.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/scalar-expr-evaluator.cc b/be/src/exprs/scalar-expr-evaluator.cc
index b1ace96..0cadcc2 100644
--- a/be/src/exprs/scalar-expr-evaluator.cc
+++ b/be/src/exprs/scalar-expr-evaluator.cc
@@ -190,9 +190,11 @@ Status ScalarExprEvaluator::Clone(ObjectPool* pool, RuntimeState* state,
   DCHECK(cloned_evals->empty());
   for (int i = 0; i < evals.size(); ++i) {
     ScalarExprEvaluator* cloned_eval;
-    RETURN_IF_ERROR(
-        evals[i]->Clone(pool, state, expr_perm_pool, expr_results_pool, &cloned_eval));
+    Status status =
+        evals[i]->Clone(pool, state, expr_perm_pool, expr_results_pool, &cloned_eval);
+    // Always add the evaluator to the vector so it can be cleaned up.
     cloned_evals->push_back(cloned_eval);
+    RETURN_IF_ERROR(status);
   }
   return Status::OK();
 }

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3ddafcd2/be/src/testutil/test-udfs.cc
----------------------------------------------------------------------
diff --git a/be/src/testutil/test-udfs.cc b/be/src/testutil/test-udfs.cc
index 894ce61..a539c87 100644
--- a/be/src/testutil/test-udfs.cc
+++ b/be/src/testutil/test-udfs.cc
@@ -334,6 +334,55 @@ void ValidateOpenClose(
   }
 }
 
+// This prepare function always fails to make sure clean up is done afterwards.
+void BadExprPrepare(FunctionContext* context, FunctionContext::FunctionStateScope scope) {
+  if (scope == FunctionContext::FRAGMENT_LOCAL) {
+    int32_t* state = reinterpret_cast<int32_t*>(context->Allocate(sizeof(int32_t)));
+    *state = 0xf001cafe;
+    context->SetFunctionState(scope, state);
+  }
+  context->SetError("BadExpr prepare error");
+}
+
+// This prepare function always fails for cloned evaluators to exercise IMPALA-6184.
+// It does so by detecting whether the caller is a cloned evaluator and inserts an error
+// in FunctionContext if that's the case.
+void BadExpr2Prepare(FunctionContext* context,
+    FunctionContext::FunctionStateScope scope) {
+  if (scope == FunctionContext::FRAGMENT_LOCAL) {
+    int32_t* state = reinterpret_cast<int32_t*>(context->Allocate(sizeof(int32_t)));
+    *state = 0xf001cafe;
+    context->SetFunctionState(scope, state);
+    // Set the thread local state too to differentiate from cloned evaluators.
+    context->SetFunctionState(FunctionContext::THREAD_LOCAL, state);
+  } else {
+    if (context->GetFunctionState(FunctionContext::THREAD_LOCAL) == nullptr) {
+      context->SetError("BadExpr2 prepare error");
+    }
+  }
+}
+
+// Used by both BadExprPrepare() and BadExpr2Prepare() above.
+BooleanVal BadExpr(FunctionContext* context, const DoubleVal& slot) {
+  static int32_t count = 0;
+  if (slot.is_null) return BooleanVal(false);
+  if (++count > 100) {
+    context->SetError("BadExpr error");
+    return BooleanVal(false);
+  }
+  return BooleanVal(true);
+}
+
+// Used by both BadExprPrepare() and BadExpr2Prepare() above.
+void BadExprClose(FunctionContext* context, FunctionContext::FunctionStateScope scope) {
+  if (scope == FunctionContext::FRAGMENT_LOCAL) {
+    int32_t* state = reinterpret_cast<int32_t*>(context->GetFunctionState(scope));
+    assert(*state == 0xf001cafe);
+    context->Free(reinterpret_cast<uint8_t*>(state));
+    context->SetFunctionState(scope, nullptr);
+  }
+}
+
 // MemTest UDF: "Allocates" the specified number of bytes per call.
 void MemTestPrepare(FunctionContext* context, FunctionContext::FunctionStateScope scope) {
   if (scope == FunctionContext::THREAD_LOCAL) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3ddafcd2/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test b/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
index 73aee7e..9698717 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
@@ -63,6 +63,40 @@ select nine_args_ir(1,2,3,4,5,6,7,8,9);
 Cannot interpret LLVM IR UDF 'nine_args_ir': Codegen is needed. Please set DISABLE_CODEGEN to false.
 ====
 ---- QUERY
+create function if not exists bad_expr(double) returns boolean
+location '$FILESYSTEM_PREFIX/test-warehouse/libTestUdfs.so'
+symbol='BadExpr' prepare_fn='BadExprPrepare' close_fn='BadExprClose';
+---- RESULTS
+====
+---- QUERY
+create function if not exists bad_expr2(double) returns boolean
+location '$FILESYSTEM_PREFIX/test-warehouse/libTestUdfs.so'
+symbol='BadExpr' prepare_fn='BadExpr2Prepare' close_fn='BadExprClose';
+---- RESULTS
+====
+---- QUERY
+select count(t1.int_col) from functional.alltypes t1 join functional.alltypes t2
+on (bad_expr(rand()) = (t2.bool_col && t1.bool_col));
+---- CATCH
+BadExpr prepare error
+====
+---- QUERY
+select count(t1.int_col) from functional.alltypes t1 join functional.alltypes t2
+on (bad_expr2(rand()) = (t2.bool_col && t1.bool_col));
+---- CATCH
+BadExpr error
+====
+---- QUERY
+select count(int_col) from functional.alltypes where bad_expr(rand());
+---- CATCH
+BadExpr prepare error
+====
+---- QUERY
+select count(int_col) from functional.alltypes where bad_expr2(rand());
+---- CATCH
+BadExpr2 prepare error
+====
+---- QUERY
 use default;
 drop database $DATABASE;
 ---- CATCH
@@ -76,5 +110,7 @@ drop function twenty_args(int, int, int, int, int, int, int, int,
 drop function twenty_one_args(int, int, int, int, int, int, int, int,
     int, int, int, int, int, int, int, int, int, int, int, int, int);
 drop function nine_args_ir(int, int, int, int, int, int, int, int, int);
+drop function bad_expr(double);
+drop function bad_expr2(double);
 ---- RESULTS
 ====