Skip to content

Commit

Permalink
Fix SelectivityVector::resize (facebookincubator#1303)
Browse files Browse the repository at this point in the history
Summary:
I noticed that sometimes the number of output rows for streaming aggregation is
greater than number of input rows. This should never be the case as aggregation
must not increase cardinality. Turns out there was a bug in
SelectivityVector::resize() which is used in GroupingSet::addRemainingInput().

SelectivityVector::resize() when called on a vector whose size is a
multiple of 64 incorrectly marked last 64 bits as "selected" before resizing.

This commit fixes SelectivityVector::resize() and updates
GroupingSet::addRemainingInput() to call resize() before cleanAll() "just in case".

Pull Request resolved: facebookincubator#1303

Reviewed By: kgpai

Differential Revision: D35205252

Pulled By: mbasmanova

fbshipit-source-id: a74226813039c3aa28b430a060fdb4101226754f
  • Loading branch information
mbasmanova authored and artem.malyshev committed May 31, 2022
1 parent eb5545f commit 0758019
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 6 deletions.
2 changes: 1 addition & 1 deletion velox/exec/GroupingSet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ void GroupingSet::addInputForActiveRows(
}

void GroupingSet::addRemainingInput() {
activeRows_.clearAll();
activeRows_.resize(remainingInput_->size());
activeRows_.clearAll();
activeRows_.setValidRange(firstRemainingRow_, remainingInput_->size(), true);
activeRows_.updateBounds();

Expand Down
5 changes: 4 additions & 1 deletion velox/vector/SelectivityVector.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ class SelectivityVector {
auto numWords = bits::nwords(size);
// Set bits from size_ to end of the word.
if (size > size_ && !bits_.empty()) {
bits::fillBits(&bits_.back(), size_ % 64, 64, value);
const auto start = size_ % 64;
if (start) {
bits::fillBits(&bits_.back(), start, 64, value);
}
}

bits_.resize(numWords, value ? -1 : 0);
Expand Down
10 changes: 6 additions & 4 deletions velox/vector/tests/SelectivityVectorTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,11 @@ void assertIsValid(
int to,
const SelectivityVector& vector,
bool value) {
for (int i = 0; i < from; i++) {
EXPECT_EQ(!value, vector.isValid(i)) << "at " << i;
}
for (int i = from; i < to; i++) {
ASSERT_EQ(value, vector.isValid(i));
EXPECT_EQ(value, vector.isValid(i)) << "at " << i;
}
}

Expand Down Expand Up @@ -398,8 +401,7 @@ TEST(SelectivityVectorTest, resize) {
SelectivityVector larger(53, true);
assertIsValid(0, 53, larger, true);
larger.resize(656, true);
assertIsValid(53, 64, larger, true);
assertIsValid(640, 656, larger, true);
assertIsValid(0, 656, larger, true);

// Check for word length reduction
larger.resize(53);
Expand All @@ -420,7 +422,7 @@ TEST(SelectivityVectorTest, select) {
a.resize(33, true);
SelectivityVector b(32, false);
b.select(a);
assertIsValid(17, 32, b, true);
assertIsValid(16, 32, b, true);

SelectivityVector empty(0);
empty.select(first);
Expand Down

0 comments on commit 0758019

Please sign in to comment.