From 7604e2f70cbab664d4796455214ed702aecffc77 Mon Sep 17 00:00:00 2001
From: Kai Liu <hfevers@gmail.com>
Date: Sat, 16 Nov 2013 23:21:15 -0800
Subject: [PATCH] Make the options in table_builder/block_builder less
 misleading

Summary:
By original design, the regular `block options` and index `block options` in table_builder is mutable. We can use ChangeOptions to change the options directly.

However, with my last change, `BlockBuilder` no longer hold the reference to the index_block_options -- as a result, any changes made after the creation of index block builder will be of no effect.

But still the code is very error-prone and developers can easily fall into the trap without aware of it. To avoid this problem from happening in the future, I deleted the `ChangeOptions` and the `index_block_options`, as well as many other changes to make it less misleading.

Test Plan:
make
make check
make release

Reviewers: dhruba, haobo

Reviewed By: haobo

CC: leveldb

Differential Revision: https://reviews.facebook.net/D13707
---
 table/block_based_table_builder.cc | 8 ++++----
 table/block_builder.cc             | 4 ++--
 table/block_builder.h              | 4 ++--
 table/block_test.cc                | 2 +-
 table/table_test.cc                | 4 ++--
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/table/block_based_table_builder.cc b/table/block_based_table_builder.cc
index 55a2e3d2c..237d88935 100644
--- a/table/block_based_table_builder.cc
+++ b/table/block_based_table_builder.cc
@@ -77,7 +77,6 @@ void LogStatsCollectionError(
 
 struct BlockBasedTableBuilder::Rep {
   Options options;
-  Options index_block_options;
   WritableFile* file;
   uint64_t offset = 0;
   Status status;
@@ -105,10 +104,11 @@ struct BlockBasedTableBuilder::Rep {
 
   Rep(const Options& opt, WritableFile* f, CompressionType compression_type)
       : options(opt),
-        index_block_options(opt),
         file(f),
-        data_block(&options),
-        index_block(1, index_block_options.comparator),
+        data_block(options),
+        // To avoid linear scan, we make the block_restart_interval to be `1`
+        // in index block builder
+        index_block(1 /* block_restart_interval */, options.comparator),
         compression_type(compression_type),
         filter_block(opt.filter_policy == nullptr ? nullptr
                      : new FilterBlockBuilder(opt)) {
diff --git a/table/block_builder.cc b/table/block_builder.cc
index 0382dfc24..917601865 100644
--- a/table/block_builder.cc
+++ b/table/block_builder.cc
@@ -51,8 +51,8 @@ BlockBuilder::BlockBuilder(int block_restart_interval,
   restarts_.push_back(0);       // First restart point is at offset 0
 }
 
-BlockBuilder::BlockBuilder(const Options* options)
-    : BlockBuilder(options->block_restart_interval, options->comparator) {
+BlockBuilder::BlockBuilder(const Options& options)
+    : BlockBuilder(options.block_restart_interval, options.comparator) {
 }
 
 void BlockBuilder::Reset() {
diff --git a/table/block_builder.h b/table/block_builder.h
index c5ea6690d..31faf19b8 100644
--- a/table/block_builder.h
+++ b/table/block_builder.h
@@ -20,8 +20,8 @@ class Comparator;
 
 class BlockBuilder {
  public:
-  BlockBuilder(int block_restart_interval, const Comparator* comparator);
-  explicit BlockBuilder(const Options* options);
+  BlockBuilder(int block_builder, const Comparator* comparator);
+  explicit BlockBuilder(const Options& options);
 
   // Reset the contents as if the BlockBuilder was just constructed.
   void Reset();
diff --git a/table/block_test.cc b/table/block_test.cc
index f1b16faff..7f33e3a90 100644
--- a/table/block_test.cc
+++ b/table/block_test.cc
@@ -34,7 +34,7 @@ TEST(BlockTest, SimpleTest) {
   Options options = Options();
   std::vector<std::string> keys;
   std::vector<std::string> values;
-  BlockBuilder builder(&options);
+  BlockBuilder builder(options);
   int num_records = 100000;
   char buf[10];
   char* p = &buf[0];
diff --git a/table/table_test.cc b/table/table_test.cc
index 9fa687b12..064948692 100644
--- a/table/table_test.cc
+++ b/table/table_test.cc
@@ -217,7 +217,7 @@ class BlockConstructor: public Constructor {
   virtual Status FinishImpl(const Options& options, const KVMap& data) {
     delete block_;
     block_ = nullptr;
-    BlockBuilder builder(&options);
+    BlockBuilder builder(options);
 
     for (KVMap::const_iterator it = data.begin();
          it != data.end();
@@ -832,7 +832,7 @@ TEST(TableTest, BasicTableStats) {
   ASSERT_EQ("", stats.filter_policy_name);  // no filter policy is used
 
   // Verify data size.
-  BlockBuilder block_builder(&options);
+  BlockBuilder block_builder(options);
   for (const auto& item : kvmap) {
     block_builder.Add(item.first, item.second);
   }