This is an automated email from the ASF dual-hosted git repository.
dbecker pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new 24179d46c IMPALA-13793: Always close expressions
24179d46c is described below
commit 24179d46cce2a1ff063f9ae37f0291f2dd23d8ed
Author: Daniel Becker <[email protected]>
AuthorDate: Fri Feb 21 09:50:33 2025 +0100
IMPALA-13793: Always close expressions
The Expr class has a Close() method, which is currently only used for
releasing 'cache_entry_', which is used for UDF or UDAF functions. If
Close() is not called for an Expr that uses 'cache_entry_', a DCHECK
fails in the destructor of Expr. However, if an Expr does not use
'cache_entry_' is not closed, nothing happens.
Classes holding Expr objects should always close
expressions because they may use 'cache_entry_'. However, if tests fail
to include cases where 'cache_entry_' is used, bugs may not be detected:
one such case was IMPALA-13770.
In debug builds, this change adds a new field, 'closed_', to the Expr
class, which is set to true in Close(). A DCHECK in the destructor fires
if Close() has not been called, regardless of whether 'cache_entry_' is
used. This makes it easier to detect cases where expressions are not
closed.
To avoid increasing the size of expressions, the 'closed_' field is not
added in release mode.
Existing cases where expressions are not closed are fixed in this
change.
Testing:
- tests are updated to close expressions
Change-Id: I9784652f5e135f1f8cd7f0fa6df471e64d27c477
Reviewed-on: http://gerrit.cloudera.org:8080/22528
Reviewed-by: Impala Public Jenkins <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
---
be/src/exec/nested-loop-join-builder.cc | 7 +++++++
be/src/exprs/expr-codegen-test.cc | 1 +
be/src/exprs/expr-test.cc | 7 +++++++
be/src/exprs/expr.cc | 6 ++++++
be/src/exprs/expr.h | 4 ++++
be/src/runtime/data-stream-test.cc | 1 +
be/src/util/tuple-row-compare-test.cc | 1 +
7 files changed, 27 insertions(+)
diff --git a/be/src/exec/nested-loop-join-builder.cc
b/be/src/exec/nested-loop-join-builder.cc
index d40ce225c..35b4c5515 100644
--- a/be/src/exec/nested-loop-join-builder.cc
+++ b/be/src/exec/nested-loop-join-builder.cc
@@ -244,6 +244,13 @@ void NljBuilder::Close(RuntimeState* state) {
if (ctx.expr_eval != nullptr) ctx.expr_eval->Close(state);
}
+ if (!is_separate_build_) {
+ // If we are using embedded mode, the sink config is created by
+ // NljBuilder::CreateEmbeddedBuilder, so we must close it ourselves.
+ // TODO: Remove const-cast.
+ const_cast<DataSinkConfig&>(sink_config_).Close();
+ }
+
DataSink::Close(state);
closed_ = true;
}
diff --git a/be/src/exprs/expr-codegen-test.cc
b/be/src/exprs/expr-codegen-test.cc
index 377740b7f..c4934a435 100644
--- a/be/src/exprs/expr-codegen-test.cc
+++ b/be/src/exprs/expr-codegen-test.cc
@@ -357,6 +357,7 @@ TEST_F(ExprCodegenTest, TestInlineConstFnAttrs) {
EXPECT_EQ(result.is_null, false);
EXPECT_EQ(result.val8, 100003);
CheckFnAttr();
+ expr->Close();
codegen->Close();
}
diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index b966abfa7..21be6c6a5 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -1524,6 +1524,7 @@ TEST_P(ExprTest, NullLiteral) {
EXPECT_OK(eval->Open(&state));
EXPECT_TRUE(eval->GetValue(nullptr) == nullptr);
eval->Close(&state);
+ expr.Close();
state.ReleaseResources();
}
}
@@ -9165,7 +9166,9 @@ TEST_P(ExprTest, ResultsLayoutTest) {
expected_offsets.clear();
for (const ColumnType& t: types) {
+ ScalarExpr::Close(exprs);
exprs.clear();
+
expected_offsets.clear();
// With one expr, all offsets should be 0.
expected_offsets[t.GetByteSize()] = set<int>({0});
@@ -9184,6 +9187,8 @@ TEST_P(ExprTest, ResultsLayoutTest) {
int expected_byte_size = 0;
int expected_var_begin = 0;
expected_offsets.clear();
+
+ ScalarExpr::Close(exprs);
exprs.clear();
// Test layout adding a bunch of exprs. Previously, this triggered padding.
@@ -9259,6 +9264,8 @@ TEST_P(ExprTest, ResultsLayoutTest) {
std::random_shuffle(exprs.begin(), exprs.end());
ValidateLayout(exprs, expected_byte_size, expected_var_begin,
expected_offsets);
}
+
+ ScalarExpr::Close(exprs);
}
// TODO: is there an easy way to templatize/parametrize these tests?
diff --git a/be/src/exprs/expr.cc b/be/src/exprs/expr.cc
index 68f1c4235..a591e0d1c 100644
--- a/be/src/exprs/expr.cc
+++ b/be/src/exprs/expr.cc
@@ -44,6 +44,9 @@ Expr::Expr(const TExprNode& node)
}
Expr::~Expr() {
+#ifndef NDEBUG
+ DCHECK(closed_);
+#endif
DCHECK(cache_entry_ == nullptr);
}
@@ -97,6 +100,9 @@ void Expr::Close() {
LibCache::instance()->DecrementUseCount(cache_entry_);
cache_entry_ = nullptr;
}
+#ifndef NDEBUG
+ closed_ = true;
+#endif
}
}
diff --git a/be/src/exprs/expr.h b/be/src/exprs/expr.h
index 5903cc6a1..65855486c 100644
--- a/be/src/exprs/expr.h
+++ b/be/src/exprs/expr.h
@@ -118,6 +118,10 @@ class Expr {
friend class ExprTest;
friend class ExprCodegenTest;
+#ifndef NDEBUG
+ bool closed_ = false;
+#endif
+
/// Creates an expression tree rooted at 'root' via depth-first traversal.
/// Called recursively to create children expr trees for sub-expressions.
///
diff --git a/be/src/runtime/data-stream-test.cc
b/be/src/runtime/data-stream-test.cc
index 1334f973a..d1ebc1050 100644
--- a/be/src/runtime/data-stream-test.cc
+++ b/be/src/runtime/data-stream-test.cc
@@ -674,6 +674,7 @@ class DataStreamTest : public testing::Test {
sender->Close(&state);
info->num_bytes_sent = static_cast<KrpcDataStreamSender*>(
sender.get())->GetNumDataBytesSent();
+ data_sink->Close();
batch->Reset();
state.ReleaseResources();
diff --git a/be/src/util/tuple-row-compare-test.cc
b/be/src/util/tuple-row-compare-test.cc
index df2e4b4bc..47882f310 100644
--- a/be/src/util/tuple-row-compare-test.cc
+++ b/be/src/util/tuple-row-compare-test.cc
@@ -90,6 +90,7 @@ class TupleRowCompareTest : public testing::Test {
template <typename... Types>
void CreateComperator(Types... types) {
+ ScalarExpr::Close(ordering_exprs_);
ordering_exprs_.clear();
LoadComperator(1, types...);
}
