Skip to content

Commit

Permalink
Fix a corner case in EliasFanoReader::previousValue
Browse files Browse the repository at this point in the history
Summary:
`previousValue` makes the incorrect assumption that `outer_`
is a multiple of the word size. This is incorrect because `skipToNext`
can reposition at an arbitrary point.

The bug is very rare because it can only happen if there is a gap
larger than the skip quantum after the first element in the upper
bits.

Reviewed By: philippv

Differential Revision: D4607270

fbshipit-source-id: ff016f09f3f1f87314b68370e3dc137b82694f45
  • Loading branch information
ot authored and facebook-github-bot committed Feb 24, 2017
1 parent 3900132 commit e639eac
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 2 deletions.
4 changes: 2 additions & 2 deletions folly/experimental/EliasFanoCoding.h
Original file line number Diff line number Diff line change
Expand Up @@ -473,8 +473,8 @@ class UpperBitsReader {
block &= (block_t(1) << inner_) - 1;

while (UNLIKELY(block == 0)) {
DCHECK_GE(outer, sizeof(block_t));
outer -= sizeof(block_t);
DCHECK_GT(outer, 0);
outer -= std::min(sizeof(block_t), outer);
block = folly::loadUnaligned<block_t>(start_ + outer);
}

Expand Down
22 changes: 22 additions & 0 deletions folly/experimental/test/EliasFanoCodingTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,28 @@ TEST_F(EliasFanoCodingTest, Select64) {
}
}

TEST_F(EliasFanoCodingTest, BugLargeGapInUpperBits) { // t16274876
typedef EliasFanoEncoderV2<uint32_t, uint32_t, 2, 2> Encoder;
typedef EliasFanoReader<Encoder, instructions::EF_TEST_ARCH> Reader;
constexpr uint32_t kLargeValue = 127;

// Build a list where the upper bits have a large gap after the
// first element, so that we need to reposition in the upper bits
// using skips to position the iterator on the second element.
std::vector<uint32_t> data = {0, kLargeValue};
for (uint32_t i = 0; i < kLargeValue; ++i) {
data.push_back(data.back() + 1);
}
auto list = Encoder::encode(data.begin(), data.end());

{
Reader reader(list);
ASSERT_TRUE(reader.skipTo(kLargeValue - 1));
ASSERT_EQ(kLargeValue, reader.value());
ASSERT_EQ(0, reader.previousValue());
}
}

namespace bm {

typedef EliasFanoEncoderV2<uint32_t, uint32_t, 128, 128> Encoder;
Expand Down

0 comments on commit e639eac

Please sign in to comment.