Skip to content

Commit

Permalink
Fix flaky spill stats check in aggregation test (facebookincubator#9459)
Browse files Browse the repository at this point in the history
Summary:
The flaky is due to spill check condition is fragile which is based on whether we have received
more than one input at partial aggregation. We shall change to final aggregation and given we
have four drivers so it is not guarantee one aggregation has received more than one input row.
Since we have recorded the spill injection count, then we just rely on this to check spill stats.

This PR also restrict the case that we trigger spill for output memory reservation by checking
if table is null or empty

Pull Request resolved: facebookincubator#9459

Reviewed By: mbasmanova

Differential Revision: D56043610

Pulled By: xiaoxmeng

fbshipit-source-id: 4deaec31e7097a4aa13b695e515ac6baecc4f3ce
  • Loading branch information
xiaoxmeng authored and facebook-github-bot committed Apr 12, 2024
1 parent b1252f2 commit 2b2ef52
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 3 deletions.
3 changes: 2 additions & 1 deletion velox/exec/GroupingSet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,8 @@ void GroupingSet::ensureOutputFits() {
// to reserve memory for the output as we can't reclaim much memory from this
// operator itself. The output processing can reclaim memory from the other
// operator or query through memory arbitration.
if (isPartial_ || spillConfig_ == nullptr || hasSpilled()) {
if (isPartial_ || spillConfig_ == nullptr || hasSpilled() ||
table_ == nullptr || table_->numDistinct() == 0) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ void AggregationTestBase::testAggregationsWithCompanion(
toPlanStats(task->taskStats()).at(partialNodeId).inputRows;
const auto finalInputRows =
toPlanStats(task->taskStats()).at(finalNodeId).inputRows;
if (partialInputRows > 1) {
if (exec::injectedSpillCount() > 0) {
EXPECT_LT(0, spilledBytes(*task))
<< "partial inputRows: " << partialInputRows
<< " final inputRows: " << finalInputRows
Expand Down Expand Up @@ -854,7 +854,7 @@ void AggregationTestBase::testAggregationsImpl(
toPlanStats(task->taskStats()).at(partialNodeId).inputRows;
const auto finalInputRows =
toPlanStats(task->taskStats()).at(finalNodeId).inputRows;
if (partialInputRows > 1) {
if (exec::injectedSpillCount() > 0) {
EXPECT_LT(0, spilledBytes(*task))
<< "partial inputRows: " << partialInputRows
<< " final inputRows: " << finalInputRows
Expand Down

0 comments on commit 2b2ef52

Please sign in to comment.