IMPALA-4525: fix crash when codegen mem limit exceeded The error path in OptimizeLlvmModule() has not worked correctly for a long time because various places in the code assume that codegen'd function pointers will be filled in (e.g. ScalarFnCall) . Since the recent change "IMPALA-4397,IMPALA-3259: reduce codegen time and memory" it is more likely to go down this path.
The cases when errors occur on this path: memory limit exceeded, internal codegen bugs, and corrupt IR UDFs, are all cases when it is not correct or safe to continue executing the query, so we should just fail the query. Testing: Add a test where codegen reliably fails with memory limit exceeded. Change-Id: Ib38d0a44b54c47617cad1b971244f477d344d505 Reviewed-on: http://gerrit.cloudera.org:8080/5211 Reviewed-by: Michael Ho <[email protected]> Reviewed-by: Alex Behm <[email protected]> Tested-by: Internal 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/16552f6e Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/16552f6e Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/16552f6e Branch: refs/heads/master Commit: 16552f6eda11c1c8587ae467b55247df246c019c Parents: 90ebecd Author: Tim Armstrong <[email protected]> Authored: Wed Nov 23 13:17:56 2016 -0800 Committer: Internal Jenkins <[email protected]> Committed: Thu Nov 24 08:03:39 2016 +0000 ---------------------------------------------------------------------- be/src/runtime/plan-fragment-executor.cc | 13 ++++--------- be/src/runtime/plan-fragment-executor.h | 3 ++- be/src/service/fe-support.cc | 9 ++++++++- .../queries/QueryTest/codegen-mem-limit.test | 10 ++++++++++ tests/query_test/test_query_mem_limit.py | 17 +++++++++++++++++ 5 files changed, 41 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/16552f6e/be/src/runtime/plan-fragment-executor.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/plan-fragment-executor.cc b/be/src/runtime/plan-fragment-executor.cc index ad781b5..cfa4863 100644 --- a/be/src/runtime/plan-fragment-executor.cc +++ b/be/src/runtime/plan-fragment-executor.cc @@ -261,16 +261,11 @@ Status PlanFragmentExecutor::PrepareInternal(const TExecPlanFragmentParams& requ return Status::OK(); } -void PlanFragmentExecutor::OptimizeLlvmModule() { - if (!runtime_state_->ShouldCodegen()) return; +Status PlanFragmentExecutor::OptimizeLlvmModule() { + if (!runtime_state_->ShouldCodegen()) return Status::OK(); LlvmCodeGen* codegen = runtime_state_->codegen(); DCHECK(codegen != NULL); - Status status = codegen->FinalizeModule(); - if (!status.ok()) { - stringstream ss; - ss << "Error with codegen for this query: " << status.GetDetail(); - runtime_state_->LogError(ErrorMsg(TErrorCode::GENERAL, ss.str())); - } + return codegen->FinalizeModule(); } void PlanFragmentExecutor::PrintVolumeIds( @@ -319,7 +314,7 @@ Status PlanFragmentExecutor::OpenInternal() { report_thread_started_cv_.wait(l); } - OptimizeLlvmModule(); + RETURN_IF_ERROR(OptimizeLlvmModule()); { SCOPED_TIMER(ADD_CHILD_TIMER(timings_profile_, "ExecTreeOpenTime", OPEN_TIMER_NAME)); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/16552f6e/be/src/runtime/plan-fragment-executor.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/plan-fragment-executor.h b/be/src/runtime/plan-fragment-executor.h index 708e979..22f2653 100644 --- a/be/src/runtime/plan-fragment-executor.h +++ b/be/src/runtime/plan-fragment-executor.h @@ -279,7 +279,8 @@ class PlanFragmentExecutor { /// Optimizes the code-generated functions in runtime_state_->llvm_codegen(). /// Must be called after exec_tree_->Prepare() and before exec_tree_->Open(). - void OptimizeLlvmModule(); + /// Returns error if LLVM optimization or compilation fails. + Status OptimizeLlvmModule(); /// Executes Open() logic and returns resulting status. Does not set status_. Status OpenInternal(); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/16552f6e/be/src/service/fe-support.cc ---------------------------------------------------------------------- diff --git a/be/src/service/fe-support.cc b/be/src/service/fe-support.cc index ef951d6..93eff4b 100644 --- a/be/src/service/fe-support.cc +++ b/be/src/service/fe-support.cc @@ -133,7 +133,14 @@ Java_org_apache_impala_service_FeSupport_NativeEvalConstExprs( DCHECK(codegen != NULL); state.CodegenScalarFns(); codegen->EnableOptimizations(false); - codegen->FinalizeModule(); + 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; + } } // Open() and evaluate the exprs. Always Close() the exprs even in case of errors. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/16552f6e/testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test b/testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test new file mode 100644 index 0000000..dea6066 --- /dev/null +++ b/testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test @@ -0,0 +1,10 @@ +===== +---- QUERY +set mem_limit=100k; +select * +from alltypes +where substr(string_col, 1) = ""; +---- CATCH +Codegen failed to reserve +===== + http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/16552f6e/tests/query_test/test_query_mem_limit.py ---------------------------------------------------------------------- diff --git a/tests/query_test/test_query_mem_limit.py b/tests/query_test/test_query_mem_limit.py index 88d9576..ce12e41 100644 --- a/tests/query_test/test_query_mem_limit.py +++ b/tests/query_test/test_query_mem_limit.py @@ -104,3 +104,20 @@ class TestQueryMemLimit(ImpalaTestSuite): assert should_succeed, "Query was expected to fail" except ImpalaBeeswaxException, e: assert not should_succeed, "Query should not have failed: %s" % e + + +class TestCodegenMemLimit(ImpalaTestSuite): + """Tests that memory limit applies to codegen """ + @classmethod + def get_workload(self): + return 'functional-query' + + @classmethod + def add_test_dimensions(cls): + super(TestCodegenMemLimit, cls).add_test_dimensions() + # Only run the query for parquet + cls.TestMatrix.add_constraint( + lambda v: v.get_value('table_format').file_format == 'parquet') + + def test_codegen_mem_limit(self, vector): + self.run_test_case('QueryTest/codegen-mem-limit', vector)
