From 1e403a0c6c1aee3529f20eaa50e04467d76d05ff Mon Sep 17 00:00:00 2001
From: Peter Dillinger <peterd@fb.com>
Date: Wed, 16 Feb 2022 22:42:28 -0800
Subject: [PATCH] Fix assertion failure in FastLocalBloomBitsBuilder (#9585)

Summary:
As in

```
db_stress: table/block_based/filter_policy.cc:316: rocksdb::{anonymous}::FastLocalBloomBitsBuilder::FastLocalBloomBitsBuilder(int, std::atomic<long int>*, std::shared_ptr<rocksdb::CacheReservationManager>, bool): Assertion `millibits_per_key >= 1000' failed.
```

This assertion failure was actually happening with our RibbonFilterPolicy
which falls back to Bloom for some cases, often for flush, but was
missing new special logic to skip generating filter for 0 bits per key
case. Fixed by adding the logic in other builtin FilterPolicy
implementations.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/9585

Test Plan:
Updated db_bloom_filter_test to do more integration testing
of the RibbonFilterPolicy ("auto Ribbon") class, incl regression test
this with SkipFilterOnEssentiallyZeroBpk

Reviewed By: ajkr

Differential Revision: D34295101

Pulled By: pdillinger

fbshipit-source-id: 3488eb207fc1d67bbbd1301313714aa1b6406e6e
---
 db/db_bloom_filter_test.cc         | 19 +++++++++++++++----
 table/block_based/filter_policy.cc |  4 ++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc
index df51163e5..d398d9269 100644
--- a/db/db_bloom_filter_test.cc
+++ b/db/db_bloom_filter_test.cc
@@ -22,6 +22,7 @@
 #include "rocksdb/table.h"
 #include "table/block_based/block_based_table_reader.h"
 #include "table/block_based/filter_policy_internal.h"
+#include "table/format.h"
 #include "test_util/testutil.h"
 #include "util/string_util.h"
 
@@ -39,6 +40,7 @@ const std::string kFastLocalBloom = test::FastLocalBloomFilterPolicy::kName();
 const std::string kStandard128Ribbon =
     test::Standard128RibbonFilterPolicy::kName();
 const std::string kAutoBloom = BloomFilterPolicy::kName();
+const std::string kAutoRibbon = RibbonFilterPolicy::kName();
 }  // namespace
 
 // DB tests related to bloom filter.
@@ -559,7 +561,13 @@ TEST_P(DBBloomFilterTestWithParam, BloomFilter) {
     uint64_t filter_size = ParseUint64(props["filter_size"]);
     EXPECT_LE(filter_size,
               (partition_filters_ ? 12 : 11) * nkeys / /*bits / byte*/ 8);
-    EXPECT_GE(filter_size, 10 * nkeys / /*bits / byte*/ 8);
+    if (bfp_impl_ == kAutoRibbon) {
+      // Sometimes using Ribbon filter which is more space-efficient
+      EXPECT_GE(filter_size, 7 * nkeys / /*bits / byte*/ 8);
+    } else {
+      // Always Bloom
+      EXPECT_GE(filter_size, 10 * nkeys / /*bits / byte*/ 8);
+    }
 
     uint64_t num_filter_entries = ParseUint64(props["num_filter_entries"]);
     EXPECT_EQ(num_filter_entries, nkeys);
@@ -741,21 +749,24 @@ INSTANTIATE_TEST_CASE_P(
     ::testing::Values(
         std::make_tuple(kDeprecatedBlock, false, test::kDefaultFormatVersion),
         std::make_tuple(kAutoBloom, true, test::kDefaultFormatVersion),
-        std::make_tuple(kAutoBloom, false, test::kDefaultFormatVersion)));
+        std::make_tuple(kAutoBloom, false, test::kDefaultFormatVersion),
+        std::make_tuple(kAutoRibbon, false, test::kDefaultFormatVersion)));
 
 INSTANTIATE_TEST_CASE_P(
     FormatDef, DBBloomFilterTestWithParam,
     ::testing::Values(
         std::make_tuple(kDeprecatedBlock, false, test::kDefaultFormatVersion),
         std::make_tuple(kAutoBloom, true, test::kDefaultFormatVersion),
-        std::make_tuple(kAutoBloom, false, test::kDefaultFormatVersion)));
+        std::make_tuple(kAutoBloom, false, test::kDefaultFormatVersion),
+        std::make_tuple(kAutoRibbon, false, test::kDefaultFormatVersion)));
 
 INSTANTIATE_TEST_CASE_P(
     FormatLatest, DBBloomFilterTestWithParam,
     ::testing::Values(
         std::make_tuple(kDeprecatedBlock, false, kLatestFormatVersion),
         std::make_tuple(kAutoBloom, true, kLatestFormatVersion),
-        std::make_tuple(kAutoBloom, false, kLatestFormatVersion)));
+        std::make_tuple(kAutoBloom, false, kLatestFormatVersion),
+        std::make_tuple(kAutoRibbon, false, kLatestFormatVersion)));
 #endif  // !defined(ROCKSDB_VALGRIND_RUN) || defined(ROCKSDB_FULL_VALGRIND_RUN)
 
 TEST_F(DBBloomFilterTest, BloomFilterRate) {
diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc
index 7e2cbc6d7..968231fb4 100644
--- a/table/block_based/filter_policy.cc
+++ b/table/block_based/filter_policy.cc
@@ -1781,6 +1781,10 @@ RibbonFilterPolicy::RibbonFilterPolicy(double bloom_equivalent_bits_per_key,
 
 FilterBitsBuilder* RibbonFilterPolicy::GetBuilderWithContext(
     const FilterBuildingContext& context) const {
+  if (GetMillibitsPerKey() == 0) {
+    // "No filter" special case
+    return nullptr;
+  }
   // Treat unknown same as bottommost
   int levelish = INT_MAX;