From 1f8ade6bd67645577d68614a9e1e2204e197e671 Mon Sep 17 00:00:00 2001
From: Kai Liu <hfevers@gmail.com>
Date: Mon, 7 Oct 2013 18:33:49 -0700
Subject: [PATCH] Fix a bug in table builder

Summary:
In talbe.cc, when reading the metablock, it uses BytewiseComparator();
However in table_builder.cc, we use r->options.comparator. After tracing
the creation of r->options.comparator, I found this comparator is an
InternalKeyComparator, which wraps the user defined comparator(details
can be found in DBImpl::SanitizeOptions().

I encountered this problem when adding metadata about "bloom filter"
before. With different comparator, we may fail to do the binary sort.

Current code works well since there is only one entry in meta block.

Test Plan:
make all check

I've also tested this change in https://reviews.facebook.net/D8283 before.

Reviewers: dhruba, haobo

CC: leveldb

Differential Revision: https://reviews.facebook.net/D13335
---
 table/block_builder.cc | 20 +++++++++++++-------
 table/block_builder.h  | 14 +++++++++-----
 table/table_builder.cc |  6 +++++-
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/table/block_builder.cc b/table/block_builder.cc
index 96857c51a..017e9c359 100644
--- a/table/block_builder.cc
+++ b/table/block_builder.cc
@@ -36,15 +36,21 @@
 
 namespace rocksdb {
 
-BlockBuilder::BlockBuilder(const Options* options)
-    : options_(options),
+BlockBuilder::BlockBuilder(int block_restart_interval,
+                           const Comparator* comparator)
+    : block_restart_interval_(block_restart_interval),
+      comparator_(comparator),
       restarts_(),
       counter_(0),
       finished_(false) {
-  assert(options->block_restart_interval >= 1);
+  assert(block_restart_interval_ >= 1);
   restarts_.push_back(0);       // First restart point is at offset 0
 }
 
+BlockBuilder::BlockBuilder(const Options* options)
+    : BlockBuilder(options->block_restart_interval, options->comparator) {
+}
+
 void BlockBuilder::Reset() {
   buffer_.clear();
   restarts_.clear();
@@ -64,7 +70,7 @@ size_t BlockBuilder::EstimateSizeAfterKV(const Slice& key, const Slice& value)
   const {
   size_t estimate = CurrentSizeEstimate();
   estimate += key.size() + value.size();
-  if (counter_ >= options_->block_restart_interval) {
+  if (counter_ >= block_restart_interval_) {
     estimate += sizeof(uint32_t); // a new restart entry.
   }
 
@@ -88,11 +94,11 @@ Slice BlockBuilder::Finish() {
 void BlockBuilder::Add(const Slice& key, const Slice& value) {
   Slice last_key_piece(last_key_);
   assert(!finished_);
-  assert(counter_ <= options_->block_restart_interval);
+  assert(counter_ <= block_restart_interval_);
   assert(buffer_.empty() // No values yet?
-         || options_->comparator->Compare(key, last_key_piece) > 0);
+         || comparator_->Compare(key, last_key_piece) > 0);
   size_t shared = 0;
-  if (counter_ < options_->block_restart_interval) {
+  if (counter_ < block_restart_interval_) {
     // See how much sharing to do with previous string
     const size_t min_length = std::min(last_key_piece.size(), key.size());
     while ((shared < min_length) && (last_key_piece[shared] == key[shared])) {
diff --git a/table/block_builder.h b/table/block_builder.h
index 5e86a50ae..ec4b35b89 100644
--- a/table/block_builder.h
+++ b/table/block_builder.h
@@ -11,9 +11,11 @@
 namespace rocksdb {
 
 struct Options;
+class Comparator;
 
 class BlockBuilder {
  public:
+  BlockBuilder(int block_builder, const Comparator* comparator);
   explicit BlockBuilder(const Options* options);
 
   // Reset the contents as if the BlockBuilder was just constructed.
@@ -41,11 +43,13 @@ class BlockBuilder {
   }
 
  private:
-  const Options*        options_;
-  std::string           buffer_;      // Destination buffer
-  std::vector<uint32_t> restarts_;    // Restart points
-  int                   counter_;     // Number of entries emitted since restart
-  bool                  finished_;    // Has Finish() been called?
+  const int          block_restart_interval_;
+  const Comparator*  comparator_;
+
+  std::string           buffer_;    // Destination buffer
+  std::vector<uint32_t> restarts_;  // Restart points
+  int                   counter_;   // Number of entries emitted since restart
+  bool                  finished_;  // Has Finish() been called?
   std::string           last_key_;
 
   // No copying allowed
diff --git a/table/table_builder.cc b/table/table_builder.cc
index dd51332dd..d42ef0d2f 100644
--- a/table/table_builder.cc
+++ b/table/table_builder.cc
@@ -269,7 +269,11 @@ Status TableBuilder::Finish() {
 
   // Write metaindex block
   if (ok()) {
-    BlockBuilder meta_index_block(&r->options);
+    // We use `BytewiseComparator` as the comparator for meta block.
+    BlockBuilder meta_index_block(
+        r->options.block_restart_interval,
+        BytewiseComparator()
+    );
     if (r->filter_block != nullptr) {
       // Add mapping from "filter.Name" to location of filter data
       std::string key = "filter.";