Skip to content

Use ByteString instead of std::vector in the BloomFilter #11038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 30, 2023
Merged

Use ByteString instead of std::vector in the BloomFilter #11038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe8e32b
Convert BloomFilter to use ByteString instead of std::vector
dconeybe Mar 29, 2023
541683b
FSTSpecTests.mm: std::vector<uint8_t> -> ByteString
dconeybe Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ - (SnapshotVersion)parseVersion:(NSNumber *_Nullable)version {
NSString *bitmapEncoded = bitsData[@"bitmap"];
std::string bitmapDecoded;
absl::Base64Unescape([bitmapEncoded cStringUsingEncoding:NSASCIIStringEncoding], &bitmapDecoded);
std::vector<uint8_t> bitmap(bitmapDecoded.begin(), bitmapDecoded.end());
ByteString bitmap(bitmapDecoded);

// If not specified in proto, default padding and hashCount to 0.
int32_t padding = [bitsData[@"padding"] intValue];
Expand Down
15 changes: 7 additions & 8 deletions Firestore/core/src/remote/bloom_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ namespace firebase {
namespace firestore {
namespace remote {

using nanopb::ByteString;
using util::Status;
using util::StatusOr;

Expand All @@ -38,9 +39,9 @@ bool HasSameBits(const BloomFilter& lhs, const BloomFilter& rhs) {
return true;
}

const std::vector<uint8_t>& bitmap1 = lhs.bitmap();
const std::vector<uint8_t>& bitmap2 = rhs.bitmap();
const auto byte_count = static_cast<int32_t>(bitmap1.size());
const auto byte_count = static_cast<int32_t>(lhs.bitmap().size());
Copy link
Contributor

@milaGGL milaGGL Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i asked the same question before, but why auto? since we know its type will be int32_t

Copy link
Contributor Author

@dconeybe dconeybe Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint suggests (requires?) using auto when the type is explicitly mentioned on the right-hand side of the equal sign. By mentioning it twice it actually opens up the opportunity for an unintended implicit narrowing conversion if there is a typo on one side of the equal sign.

const uint8_t* bitmap1 = lhs.bitmap().data();
const uint8_t* bitmap2 = rhs.bitmap().data();

// Compare all bytes from the bitmap, except for the last byte.
for (int32_t i = 0; i < byte_count - 1; ++i) {
Expand Down Expand Up @@ -81,14 +82,12 @@ int32_t BloomFilter::GetBitIndex(const Hash& hash, int32_t hash_index) const {
}

bool BloomFilter::IsBitSet(int32_t index) const {
uint8_t byte_at_index = bitmap_[index / 8];
uint8_t byte_at_index = bitmap_.data()[index / 8];
int32_t offset = index % 8;
return (byte_at_index & (static_cast<uint8_t>(0x01) << offset)) != 0;
}

BloomFilter::BloomFilter(std::vector<uint8_t> bitmap,
int32_t padding,
int32_t hash_count)
BloomFilter::BloomFilter(ByteString bitmap, int32_t padding, int32_t hash_count)
: bit_count_(static_cast<int32_t>(bitmap.size()) * 8 - padding),
hash_count_(hash_count),
bitmap_(std::move(bitmap)) {
Expand All @@ -101,7 +100,7 @@ BloomFilter::BloomFilter(std::vector<uint8_t> bitmap,
HARD_ASSERT(bit_count_ >= 0);
}

StatusOr<BloomFilter> BloomFilter::Create(std::vector<uint8_t> bitmap,
StatusOr<BloomFilter> BloomFilter::Create(ByteString bitmap,
int32_t padding,
int32_t hash_count) {
if (padding < 0 || padding >= 8) {
Expand Down
10 changes: 5 additions & 5 deletions Firestore/core/src/remote/bloom_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#define FIRESTORE_CORE_SRC_REMOTE_BLOOM_FILTER_H_

#include <string>
#include <vector>
#include "Firestore/core/src/nanopb/byte_string.h"
#include "Firestore/core/src/util/statusor.h"
#include "absl/strings/string_view.h"

Expand All @@ -28,7 +28,7 @@ namespace remote {

class BloomFilter final {
public:
BloomFilter(std::vector<uint8_t> bitmap, int32_t padding, int32_t hash_count);
BloomFilter(nanopb::ByteString bitmap, int32_t padding, int32_t hash_count);

// Copyable & movable.
BloomFilter(const BloomFilter&) = default;
Expand All @@ -42,7 +42,7 @@ class BloomFilter final {
* @return a new BloomFilter if the inputs are valid, otherwise returns a not
* `ok()` status.
*/
static util::StatusOr<BloomFilter> Create(std::vector<uint8_t> bitmap,
static util::StatusOr<BloomFilter> Create(nanopb::ByteString bitmap,
int32_t padding,
int32_t hash_count);

Expand Down Expand Up @@ -75,7 +75,7 @@ class BloomFilter final {
}

/** Bloom filter's bitmap. */
const std::vector<uint8_t>& bitmap() const {
const nanopb::ByteString& bitmap() const {
return bitmap_;
}

Expand Down Expand Up @@ -108,7 +108,7 @@ class BloomFilter final {

int32_t hash_count_ = 0;

std::vector<uint8_t> bitmap_;
nanopb::ByteString bitmap_;
};

bool operator==(const BloomFilter& lhs, const BloomFilter& rhs);
Expand Down
4 changes: 2 additions & 2 deletions Firestore/core/src/remote/existence_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,16 @@
#define FIRESTORE_CORE_SRC_REMOTE_EXISTENCE_FILTER_H_

#include <utility>
#include <vector>

#include "Firestore/core/src/nanopb/byte_string.h"
#include "Firestore/core/src/remote/bloom_filter.h"

namespace firebase {
namespace firestore {
namespace remote {

struct BloomFilterParameters {
std::vector<uint8_t> bitmap;
nanopb::ByteString bitmap;
int32_t padding;
int32_t hash_count;
};
Expand Down
12 changes: 5 additions & 7 deletions Firestore/core/src/remote/serializer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1456,15 +1456,13 @@ ExistenceFilter Serializer::DecodeExistenceFilter(

int32_t hash_count = filter.unchanged_names.hash_count;
int32_t padding = 0;
std::vector<uint8_t> bitmap;
ByteString bitmap;
if (filter.unchanged_names.has_bits) {
padding = filter.unchanged_names.bits.padding;

pb_bytes_array_t* bitmap_ptr = filter.unchanged_names.bits.bitmap;
if (bitmap_ptr != nullptr) {
bitmap = std::vector<uint8_t>(bitmap_ptr->bytes,
bitmap_ptr->bytes + bitmap_ptr->size);
}
// TODO(b/274668697) Steal the bytes using ByteString::Take() instead of
// copying them. To do this, the `filter` argument will need to be
// non-const, which will affect the caller(s), and their caller(s), etc.
bitmap = ByteString(filter.unchanged_names.bits.bitmap);
}
return {filter.count,
BloomFilterParameters{std::move(bitmap), padding, hash_count}};
Expand Down
62 changes: 31 additions & 31 deletions Firestore/core/test/unit/remote/bloom_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

#include <fstream>
#include <iostream>
#include <vector>

#include "Firestore/core/src/util/hard_assert.h"
#include "Firestore/core/src/util/json_reader.h"
Expand All @@ -31,39 +30,40 @@ namespace firestore {
namespace remote {
namespace {

using nanopb::ByteString;
using nlohmann::json;
using util::JsonReader;
using util::Path;
using util::Status;
using util::StatusOr;

TEST(BloomFilterUnitTest, CanInstantiateEmptyBloomFilter) {
BloomFilter bloom_filter(std::vector<uint8_t>{}, 0, 0);
BloomFilter bloom_filter(ByteString{}, 0, 0);
EXPECT_EQ(bloom_filter.bit_count(), 0);
}

TEST(BloomFilterUnitTest, CanInstantiateNonEmptyBloomFilter) {
{
BloomFilter bloom_filter(std::vector<uint8_t>{1}, 0, 1);
BloomFilter bloom_filter(ByteString{1}, 0, 1);
EXPECT_EQ(bloom_filter.bit_count(), 8);
}
{
BloomFilter bloom_filter(std::vector<uint8_t>{1}, 7, 1);
BloomFilter bloom_filter(ByteString{1}, 7, 1);
EXPECT_EQ(bloom_filter.bit_count(), 1);
}
}

TEST(BloomFilterUnitTest, CreateShouldReturnBloomFilterOnValidInputs) {
StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::vector<uint8_t>{1}, 1, 1);
BloomFilter::Create(ByteString{1}, 1, 1);
ASSERT_TRUE(maybe_bloom_filter.ok());
BloomFilter bloom_filter = maybe_bloom_filter.ValueOrDie();
EXPECT_EQ(bloom_filter.bit_count(), 7);
}

TEST(BloomFilterUnitTest, CreateShouldBeAbleToCreatEmptyBloomFilter) {
StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::vector<uint8_t>{}, 0, 0);
BloomFilter::Create(ByteString{}, 0, 0);
ASSERT_TRUE(maybe_bloom_filter.ok());
BloomFilter bloom_filter = maybe_bloom_filter.ValueOrDie();
EXPECT_EQ(bloom_filter.bit_count(), 0);
Expand All @@ -72,14 +72,14 @@ TEST(BloomFilterUnitTest, CreateShouldBeAbleToCreatEmptyBloomFilter) {
TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnNegativePadding) {
{
StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::vector<uint8_t>{}, -1, 0);
BloomFilter::Create(ByteString{}, -1, 0);
ASSERT_FALSE(maybe_bloom_filter.ok());
EXPECT_EQ(maybe_bloom_filter.status().error_message(),
"Invalid padding: -1");
}
{
StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::vector<uint8_t>{1}, -1, 1);
BloomFilter::Create(ByteString{1}, -1, 1);
ASSERT_FALSE(maybe_bloom_filter.ok());
EXPECT_EQ(maybe_bloom_filter.status().error_message(),
"Invalid padding: -1");
Expand All @@ -89,14 +89,14 @@ TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnNegativePadding) {
TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnNegativeHashCount) {
{
StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::vector<uint8_t>{}, 0, -1);
BloomFilter::Create(ByteString{}, 0, -1);
ASSERT_FALSE(maybe_bloom_filter.ok());
EXPECT_EQ(maybe_bloom_filter.status().error_message(),
"Invalid hash count: -1");
}
{
StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::vector<uint8_t>{1}, 1, -1);
BloomFilter::Create(ByteString{1}, 1, -1);
ASSERT_FALSE(maybe_bloom_filter.ok());
EXPECT_EQ(maybe_bloom_filter.status().error_message(),
"Invalid hash count: -1");
Expand All @@ -105,51 +105,51 @@ TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnNegativeHashCount) {

TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnZeroHashCount) {
StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::vector<uint8_t>{1}, 1, 0);
BloomFilter::Create(ByteString{1}, 1, 0);
ASSERT_FALSE(maybe_bloom_filter.ok());
EXPECT_EQ(maybe_bloom_filter.status().error_message(),
"Invalid hash count: 0");
}

TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusIfPaddingIsTooLarge) {
StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::vector<uint8_t>{1}, 8, 1);
BloomFilter::Create(ByteString{1}, 8, 1);
ASSERT_FALSE(maybe_bloom_filter.ok());
EXPECT_EQ(maybe_bloom_filter.status().error_message(), "Invalid padding: 8");
}

TEST(BloomFilterTest, CheckBloomFiltersEqualityWithSameInput) {
BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
BloomFilter bloom_filter2(std::vector<uint8_t>{1}, 1, 1);
BloomFilter bloom_filter1(ByteString{1}, 1, 1);
BloomFilter bloom_filter2(ByteString{1}, 1, 1);
EXPECT_TRUE(bloom_filter1 == bloom_filter2);
EXPECT_FALSE(bloom_filter1 != bloom_filter2);
}

TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentBitmap) {
{
BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
BloomFilter bloom_filter2(std::vector<uint8_t>{2}, 1, 1);
BloomFilter bloom_filter1(ByteString{1}, 1, 1);
BloomFilter bloom_filter2(ByteString{2}, 1, 1);
EXPECT_FALSE(bloom_filter1 == bloom_filter2);
EXPECT_TRUE(bloom_filter1 != bloom_filter2);
}
{
BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
BloomFilter bloom_filter2(std::vector<uint8_t>{1, 1}, 1, 1);
BloomFilter bloom_filter1(ByteString{1}, 1, 1);
BloomFilter bloom_filter2(ByteString{1, 1}, 1, 1);
EXPECT_FALSE(bloom_filter1 == bloom_filter2);
EXPECT_TRUE(bloom_filter1 != bloom_filter2);
}
}

TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentPadding) {
BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
BloomFilter bloom_filter2(std::vector<uint8_t>{1}, 2, 1);
BloomFilter bloom_filter1(ByteString{1}, 1, 1);
BloomFilter bloom_filter2(ByteString{1}, 2, 1);
EXPECT_FALSE(bloom_filter1 == bloom_filter2);
EXPECT_TRUE(bloom_filter1 != bloom_filter2);
}

TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentHashCount) {
BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
BloomFilter bloom_filter2(std::vector<uint8_t>{1}, 1, 2);
BloomFilter bloom_filter1(ByteString{1}, 1, 1);
BloomFilter bloom_filter2(ByteString{1}, 1, 2);
EXPECT_FALSE(bloom_filter1 == bloom_filter2);
EXPECT_TRUE(bloom_filter1 != bloom_filter2);
}
Expand All @@ -159,17 +159,17 @@ TEST(BloomFilterTest,
// In BloomFilter bitmap, padding is guaranteed to be less than 8, and
// starts counting from the leftmost indexes of the last byte.
{
BloomFilter bloom_filter1(std::vector<uint8_t>{255, 127}, 1,
BloomFilter bloom_filter1(ByteString{255, 127}, 1,
1); // bitmap -> 11111111 01111111
BloomFilter bloom_filter2(std::vector<uint8_t>{255, 255}, 1,
BloomFilter bloom_filter2(ByteString{255, 255}, 1,
1); // bitmap -> 11111111 11111111
EXPECT_TRUE(bloom_filter1 == bloom_filter2);
EXPECT_FALSE(bloom_filter1 != bloom_filter2);
}
{
BloomFilter bloom_filter1(std::vector<uint8_t>{255, 207}, 4,
BloomFilter bloom_filter1(ByteString{255, 207}, 4,
1); // bitmap -> 11111111 11001111
BloomFilter bloom_filter2(std::vector<uint8_t>{255, 255}, 4,
BloomFilter bloom_filter2(ByteString{255, 255}, 4,
1); // bitmap -> 11111111 11111111
EXPECT_TRUE(bloom_filter1 == bloom_filter2);
EXPECT_FALSE(bloom_filter1 != bloom_filter2);
Expand All @@ -178,25 +178,25 @@ TEST(BloomFilterTest,

TEST(BloomFilterTest, MightContainCanProcessNonStandardCharacters) {
// A non-empty BloomFilter object with 1 insertion : "ÀÒ∑"
BloomFilter bloom_filter(std::vector<uint8_t>{237, 5}, 5, 8);
BloomFilter bloom_filter(ByteString{237, 5}, 5, 8);
EXPECT_TRUE(bloom_filter.MightContain("ÀÒ∑"));
EXPECT_FALSE(bloom_filter.MightContain("Ò∑À"));
}

TEST(BloomFilterUnitTest, MightContainOnEmptyBloomFilterShouldReturnFalse) {
BloomFilter bloom_filter(std::vector<uint8_t>{}, 0, 0);
BloomFilter bloom_filter(ByteString{}, 0, 0);
EXPECT_FALSE(bloom_filter.MightContain(""));
EXPECT_FALSE(bloom_filter.MightContain("a"));
}

TEST(BloomFilterUnitTest,
MightContainWithEmptyStringMightReturnFalsePositiveResult) {
{
BloomFilter bloom_filter(std::vector<uint8_t>{1}, 1, 1);
BloomFilter bloom_filter(ByteString{1}, 1, 1);
EXPECT_FALSE(bloom_filter.MightContain(""));
}
{
BloomFilter bloom_filter(std::vector<uint8_t>{255}, 0, 16);
BloomFilter bloom_filter(ByteString{255}, 0, 16);
EXPECT_TRUE(bloom_filter.MightContain(""));
}
}
Expand Down Expand Up @@ -241,7 +241,7 @@ class BloomFilterGoldenTest : public ::testing::Test {
int hash_count = reader.OptionalInt("hashCount", test_file, 0);
std::string decoded;
absl::Base64Unescape(bitmap, &decoded);
std::vector<uint8_t> decoded_map(decoded.begin(), decoded.end());
ByteString decoded_map(decoded);

StatusOr<BloomFilter> maybe_bloom_filter =
BloomFilter::Create(std::move(decoded_map), padding, hash_count);
Expand Down