[2/2] incubator-impala git commit: IMPALA-4525: follow-on: cleanup error handling

[ta...@apache.org]

IMPALA-4525: follow-on: cleanup error handling

Testing:
Ran exhaustive build. There is already some test coverage in
test_exprs.py for errors returned during constant expr evaluation by the
frontend.

Change-Id: Ifb2e532381c3e6c7b2e3ef37c2d91fbebb2db2d0
Reviewed-on: http://gerrit.cloudera.org:8080/5212
Reviewed-by: Tim Armstrong <ta...@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/d44df836
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/d44df836
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/d44df836

Branch: refs/heads/master
Commit: d44df83689f6cd0cb5c93d3893beaff4f7c9342a
Parents: 48983b3
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Wed Nov 23 15:48:55 2016 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Dec 2 11:37:14 2016 +0000

----------------------------------------------------------------------
 be/src/exprs/expr-context.cc |  4 +--
 be/src/exprs/expr-context.h  |  3 ++
 be/src/service/fe-support.cc | 67 +++++++++++++++++----------------------
 3 files changed, 34 insertions(+), 40 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d44df836/be/src/exprs/expr-context.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-context.cc b/be/src/exprs/expr-context.cc
index 27c61c1..94c652a 100644
--- a/be/src/exprs/expr-context.cc
+++ b/be/src/exprs/expr-context.cc
@@ -71,9 +71,9 @@ Status ExprContext::Open(RuntimeState* state) {
 }
 
 void ExprContext::Close(RuntimeState* state) {
-  DCHECK(!closed_);
+  if (closed_) return;
   FunctionContext::FunctionStateScope scope =
-      is_clone_? FunctionContext::THREAD_LOCAL : FunctionContext::FRAGMENT_LOCAL;
+      is_clone_ ? FunctionContext::THREAD_LOCAL : FunctionContext::FRAGMENT_LOCAL;
   root_->Close(state, this, scope);
 
   for (int i = 0; i < fn_contexts_.size(); ++i) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d44df836/be/src/exprs/expr-context.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-context.h b/be/src/exprs/expr-context.h
index e765b87..f7e1239 100644
--- a/be/src/exprs/expr-context.h
+++ b/be/src/exprs/expr-context.h
@@ -49,6 +49,8 @@ class ExprContext {
 
   /// Prepare expr tree for evaluation.
   /// Allocations from this context will be counted against 'tracker'.
+  /// If Prepare() is called, Close() must be called before destruction to release
+  /// resources, regardless of whether Prepare() succeeded.
   Status Prepare(RuntimeState* state, const RowDescriptor& row_desc,
                  MemTracker* tracker);
 
@@ -68,6 +70,7 @@ class ExprContext {
   Status Clone(RuntimeState* state, ExprContext** new_context);
 
   /// Closes all FunctionContexts. Must be called on every ExprContext, including clones.
+  /// Has no effect if already closed.
   void Close(RuntimeState* state);
 
   /// Calls the appropriate Get*Val() function on this context's expr tree and stores the

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d44df836/be/src/service/fe-support.cc
----------------------------------------------------------------------
diff --git a/be/src/service/fe-support.cc b/be/src/service/fe-support.cc
index 93eff4b..b3e3d0c 100644
--- a/be/src/service/fe-support.cc
+++ b/be/src/service/fe-support.cc
@@ -79,11 +79,13 @@ JNIEXPORT jbyteArray JNICALL
 Java_org_apache_impala_service_FeSupport_NativeEvalConstExprs(
     JNIEnv* env, jclass caller_class, jbyteArray thrift_expr_batch,
     jbyteArray thrift_query_ctx_bytes) {
+  Status status;
   jbyteArray result_bytes = NULL;
   TQueryCtx query_ctx;
   TExprBatch expr_batch;
   JniLocalFrame jni_frame;
   TResultRow expr_results;
+  vector<TColumnValue> results;
   ObjectPool obj_pool;
 
   DeserializeThriftMsg(env, thrift_expr_batch, &expr_batch);
@@ -113,71 +115,60 @@ Java_org_apache_impala_service_FeSupport_NativeEvalConstExprs(
   vector<ExprContext*> expr_ctxs;
   for (const TExpr& texpr : texprs) {
     ExprContext* ctx;
-    THROW_IF_ERROR_RET(Expr::CreateExprTree(&obj_pool, texpr, &ctx), env,
-        JniUtil::internal_exc_class(), result_bytes);
-    Status prepare_status =
-        ctx->Prepare(&state, RowDescriptor(), state.query_mem_tracker());
+    status = Expr::CreateExprTree(&obj_pool, texpr, &ctx);
+    if (!status.ok()) goto error;
+
+    // Add 'ctx' to vector so it will be closed if Prepare() fails.
     expr_ctxs.push_back(ctx);
-    if (!prepare_status.ok()) {
-      for (ExprContext* ctx: expr_ctxs) ctx->Close(&state);
-      (env)->ThrowNew(JniUtil::internal_exc_class(), prepare_status.GetDetail().c_str());
-      return result_bytes;
-    }
+    status = ctx->Prepare(&state, RowDescriptor(), state.query_mem_tracker());
+    if (!status.ok()) goto error;
   }
 
   // UDFs which cannot be interpreted need to be handled by codegen.
   if (state.ScalarFnNeedsCodegen()) {
-    THROW_IF_ERROR_RET(
-        state.CreateCodegen(), env, JniUtil::internal_exc_class(), result_bytes);
+    status = state.CreateCodegen();
+    if (!status.ok()) goto error;
     LlvmCodeGen* codegen = state.codegen();
     DCHECK(codegen != NULL);
     state.CodegenScalarFns();
     codegen->EnableOptimizations(false);
     Status status = codegen->FinalizeModule();
-    if (!status.ok()) {
-      for (int i = 0; i < expr_ctxs.size(); ++i) {
-        expr_ctxs[i]->Close(&state);
-      }
-      (env)->ThrowNew(JniUtil::internal_exc_class(), status.GetDetail().c_str());
-      return result_bytes;
-    }
+    if (!status.ok()) goto error;
   }
 
   // Open() and evaluate the exprs. Always Close() the exprs even in case of errors.
-  vector<TColumnValue> results;
-  Status status;
-  int i;
-  for (i = 0; i < expr_ctxs.size(); ++i) {
-    status = expr_ctxs[i]->Open(&state);
-    if (!status.ok()) break;
+  for (ExprContext* expr_ctx : expr_ctxs) {
+    status = expr_ctx->Open(&state);
+    if (!status.ok()) goto error;
 
     TColumnValue val;
-    expr_ctxs[i]->GetConstantValue(&val);
-    status = expr_ctxs[i]->root()->GetFnContextError(expr_ctxs[i]);
-    if (!status.ok()) break;
+    expr_ctx->GetConstantValue(&val);
+    status = expr_ctx->root()->GetFnContextError(expr_ctx);
+    if (!status.ok()) goto error;
 
     // Check for mem limit exceeded.
     status = state.CheckQueryState();
-    if (!status.ok()) break;
+    if (!status.ok()) goto error;
     // Check for warnings registered in the runtime state.
     if (state.HasErrors()) {
       status = Status(state.ErrorLog());
-      break;
+      goto error;
     }
 
-    expr_ctxs[i]->Close(&state);
+    expr_ctx->Close(&state);
     results.push_back(val);
   }
-  // Convert status to exception. Close all remaining expr contexts.
-  if (!status.ok()) {
-    for (int j = i; j < expr_ctxs.size(); ++j) expr_ctxs[j]->Close(&state);
-    (env)->ThrowNew(JniUtil::internal_exc_class(), status.GetDetail().c_str());
-    return result_bytes;
-  }
 
   expr_results.__set_colVals(results);
-  THROW_IF_ERROR_RET(SerializeThriftMsg(env, &expr_results, &result_bytes), env,
-                     JniUtil::internal_exc_class(), result_bytes);
+  status = SerializeThriftMsg(env, &expr_results, &result_bytes);
+  if (!status.ok()) goto error;
+  return result_bytes;
+
+error:
+  DCHECK(!status.ok());
+  // Convert status to exception. Close all remaining expr contexts.
+  for (ExprContext* expr_ctx : expr_ctxs) expr_ctx->Close(&state);
+  (env)->ThrowNew(JniUtil::internal_exc_class(), status.GetDetail().c_str());
   return result_bytes;
 }