Skip to content

Commit 00d9594

Browse files
tanjialiangfacebook-github-bot
authored andcommitted
fix: Fix crash from hash aggregation row container cleanup due to abort (facebookincubator#13979)
Summary: Pull Request resolved: facebookincubator#13979 Aggregates could be uninitialized when destroying, causing process to crash. Avoid destroying uninitialized aggregates. Reviewed By: skyelves, kewang1024 Differential Revision: D77632466 fbshipit-source-id: 155cef4a7dd2a9cc09b831218a1e5ee31bd2198c
1 parent e4c97dc commit 00d9594

File tree

3 files changed

+65
-0
lines changed

3 files changed

+65
-0
lines changed

velox/exec/DistinctAggregations.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ class TypedDistinctAggregations : public DistinctAggregations {
4949
},
5050
[this](folly::Range<char**> groups) {
5151
for (auto* group : groups) {
52+
if (!isInitialized(group)) {
53+
continue;
54+
}
5255
auto* accumulator =
5356
reinterpret_cast<AccumulatorType*>(group + offset_);
5457
accumulator->free(*allocator_);

velox/exec/DistinctAggregations.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,10 @@ class DistinctAggregations {
101101
char** groups,
102102
folly::Range<const vector_size_t*> indices) = 0;
103103

104+
bool isInitialized(char* group) const {
105+
return group[initializedByte_] & initializedMask_;
106+
}
107+
104108
HashStringAllocator* allocator_;
105109
int32_t offset_;
106110
int32_t nullByte_;

velox/exec/tests/AggregationTest.cpp

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4030,6 +4030,64 @@ TEST_F(AggregationTest, destroyAfterPartialInitialization) {
40304030
ASSERT_TRUE(agg.destroyCalled);
40314031
}
40324032

4033+
DEBUG_ONLY_TEST_F(
4034+
AggregationTest,
4035+
uninitializedDistinctAggrWithExternalMemAggrDuringAbort) {
4036+
const auto createInput =
4037+
[&](int32_t startKey, uint32_t numGroups, uint32_t numElementsPerGroup) {
4038+
return makeRowVector({
4039+
makeFlatVector<int32_t>([&]() {
4040+
std::vector<int32_t> keys;
4041+
for (auto i = 0; i < numGroups; ++i) {
4042+
for (auto j = 0; j < numElementsPerGroup; ++j) {
4043+
keys.push_back(startKey + i);
4044+
}
4045+
}
4046+
return keys;
4047+
}()),
4048+
makeFlatVector<int32_t>(
4049+
numGroups * numElementsPerGroup,
4050+
[&](auto row) { return startKey; }),
4051+
});
4052+
};
4053+
4054+
std::vector<RowVectorPtr> inputs;
4055+
inputs.emplace_back(createInput(0, 10000, 10));
4056+
createDuckDbTable(inputs);
4057+
4058+
GroupingSet* groupingSet{nullptr};
4059+
4060+
std::atomic_bool groupingSetExtracted{false};
4061+
SCOPED_TESTVALUE_SET(
4062+
"facebook::velox::exec::GroupingSet::addInputForActiveRows",
4063+
std::function<void(GroupingSet*)>([&](GroupingSet* _groupingSet) {
4064+
if (!groupingSetExtracted.exchange(true)) {
4065+
groupingSet = _groupingSet;
4066+
}
4067+
}));
4068+
4069+
SCOPED_TESTVALUE_SET(
4070+
"facebook::velox::memory::MemoryPoolImpl::reserveThreadSafe",
4071+
std::function<void(void*)>([&](void* /*unused*/) {
4072+
if (groupingSet == nullptr) {
4073+
return;
4074+
}
4075+
if (groupingSet->numRows() > 0) {
4076+
VELOX_FAIL("Inject allocation failure.");
4077+
}
4078+
}));
4079+
4080+
auto plan = PlanBuilder()
4081+
.values(inputs)
4082+
.singleAggregation({"c0"}, {"array_agg(distinct c1)"})
4083+
.planNode();
4084+
4085+
VELOX_ASSERT_THROW(
4086+
assertQuery(
4087+
plan, "SELECT c0, array_agg(distinct c1) FROM tmp GROUP BY c0"),
4088+
"Inject allocation failure.");
4089+
}
4090+
40334091
TEST_F(AggregationTest, nanKeys) {
40344092
// Some keys are NaNs.
40354093
auto kNaN = std::numeric_limits<double>::quiet_NaN();

0 commit comments

Comments
 (0)