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 <[email protected]> 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 <[email protected]> Authored: Wed Nov 23 15:48:55 2016 -0800 Committer: Impala Public Jenkins <[email protected]> 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; }
