BlockBasedTable::NewDataBlockIterator to always return BlockIter

Summary:
This is a pre-cleaning up before a major block based table iterator refactoring. BlockBasedTable::NewDataBlockIterator() will always return BlockIter. This simplifies the logic and code and enable further refactoring and optimization.
Closes https://github.com/facebook/rocksdb/pull/3398

Differential Revision: D6780165

Pulled By: siying

fbshipit-source-id: 273f7dc896724f682c0118fb69a359d9cc4418b4

table/block.cc

@@ -421,36 +421,28 @@ Block::Block(BlockContents&& contents, SequenceNumber _global_seqno,
   }
 }
 
-InternalIterator* Block::NewIterator(const Comparator* cmp, BlockIter* iter,
-                                     bool total_order_seek, Statistics* stats) {
+BlockIter* Block::NewIterator(const Comparator* cmp, BlockIter* iter,
+                              bool total_order_seek, Statistics* stats) {
+  BlockIter* ret_iter;
+  if (iter != nullptr) {
+    ret_iter = iter;
+  } else {
+    ret_iter = new BlockIter;
+  }
   if (size_ < 2*sizeof(uint32_t)) {
-    if (iter != nullptr) {
-      iter->SetStatus(Status::Corruption("bad block contents"));
-      return iter;
-    } else {
-      return NewErrorInternalIterator(Status::Corruption("bad block contents"));
-    }
+    ret_iter->SetStatus(Status::Corruption("bad block contents"));
+    return ret_iter;
   }
   const uint32_t num_restarts = NumRestarts();
   if (num_restarts == 0) {
-    if (iter != nullptr) {
-      iter->SetStatus(Status::OK());
-      return iter;
-    } else {
-      return NewEmptyInternalIterator();
-    }
+    ret_iter->SetStatus(Status::OK());
+    return ret_iter;
   } else {
     BlockPrefixIndex* prefix_index_ptr =
         total_order_seek ? nullptr : prefix_index_.get();
-
-    if (iter != nullptr) {
-      iter->Initialize(cmp, data_, restart_offset_, num_restarts,
-                       prefix_index_ptr, global_seqno_, read_amp_bitmap_.get());
-    } else {
-      iter = new BlockIter(cmp, data_, restart_offset_, num_restarts,
-                           prefix_index_ptr, global_seqno_,
-                           read_amp_bitmap_.get());
-    }
+    ret_iter->Initialize(cmp, data_, restart_offset_, num_restarts,
+                         prefix_index_ptr, global_seqno_,
+                         read_amp_bitmap_.get());
 
     if (read_amp_bitmap_) {
       if (read_amp_bitmap_->GetStatistics() != stats) {
@@ -460,7 +452,7 @@ InternalIterator* Block::NewIterator(const Comparator* cmp, BlockIter* iter,
     }
   }
 
-  return iter;
+  return ret_iter;
 }
 
 void Block::SetBlockPrefixIndex(BlockPrefixIndex* prefix_index) {

table/block.h

@@ -168,10 +168,10 @@ class Block {
   // If total_order_seek is true, hash_index_ and prefix_index_ are ignored.
   // This option only applies for index block. For data block, hash_index_
   // and prefix_index_ are null, so this option does not matter.
-  InternalIterator* NewIterator(const Comparator* comparator,
-                                BlockIter* iter = nullptr,
-                                bool total_order_seek = true,
-                                Statistics* stats = nullptr);
+  BlockIter* NewIterator(const Comparator* comparator,
+                         BlockIter* iter = nullptr,
+                         bool total_order_seek = true,
+                         Statistics* stats = nullptr);
   void SetBlockPrefixIndex(BlockPrefixIndex* prefix_index);
 
   // Report an approximation of how much memory has been used.
@@ -191,12 +191,15 @@ class Block {
   const SequenceNumber global_seqno_;
 
   // No copying allowed
-  Block(const Block&);
-  void operator=(const Block&);
+  Block(const Block&) = delete;
+  void operator=(const Block&) = delete;
 };
 
 class BlockIter : public InternalIterator {
  public:
+  // Object created using this constructor will behave like an iterator
+  // against an empty block. The state after the creation: Valid()=false
+  // and status() is OK.
   BlockIter()
       : comparator_(nullptr),
         data_(nullptr),

table/block_based_table_reader.cc

@@ -1442,7 +1442,7 @@ InternalIterator* BlockBasedTable::NewIndexIterator(
   return iter;
 }
 
-InternalIterator* BlockBasedTable::NewDataBlockIterator(
+BlockIter* BlockBasedTable::NewDataBlockIterator(
     Rep* rep, const ReadOptions& ro, const Slice& index_value,
     BlockIter* input_iter, bool is_index, GetContext* get_context) {
   BlockHandle handle;
@@ -1458,7 +1458,7 @@ InternalIterator* BlockBasedTable::NewDataBlockIterator(
 // into an iterator over the contents of the corresponding block.
 // If input_iter is null, new a iterator
 // If input_iter is not null, update this iter and return it
-InternalIterator* BlockBasedTable::NewDataBlockIterator(
+BlockIter* BlockBasedTable::NewDataBlockIterator(
     Rep* rep, const ReadOptions& ro, const BlockHandle& handle,
     BlockIter* input_iter, bool is_index, GetContext* get_context, Status s) {
   PERF_TIMER_GUARD(new_table_block_iter_nanos);
@@ -1476,16 +1476,18 @@ InternalIterator* BlockBasedTable::NewDataBlockIterator(
                                   get_context);
   }
 
+  BlockIter* iter;
+  if (input_iter != nullptr) {
+    iter = input_iter;
+  } else {
+    iter = new BlockIter;
+  }
   // Didn't get any data from block caches.
   if (s.ok() && block.value == nullptr) {
     if (no_io) {
       // Could not read from block_cache and can't do IO
-      if (input_iter != nullptr) {
-        input_iter->SetStatus(Status::Incomplete("no blocking io"));
-        return input_iter;
-      } else {
-        return NewErrorInternalIterator(Status::Incomplete("no blocking io"));
-      }
+      iter->SetStatus(Status::Incomplete("no blocking io"));
+      return iter;
     }
     std::unique_ptr<Block> block_value;
     s = ReadBlockFromFile(rep->file.get(), nullptr /* prefetch_buffer */,
@@ -1498,10 +1500,9 @@ InternalIterator* BlockBasedTable::NewDataBlockIterator(
     }
   }
 
-  InternalIterator* iter;
   if (s.ok()) {
     assert(block.value != nullptr);
-    iter = block.value->NewIterator(&rep->internal_comparator, input_iter, true,
+    iter = block.value->NewIterator(&rep->internal_comparator, iter, true,
                                     rep->ioptions.statistics);
     if (block.cache_handle != nullptr) {
       iter->RegisterCleanup(&ReleaseCachedEntry, block_cache,
@@ -1511,12 +1512,7 @@ InternalIterator* BlockBasedTable::NewDataBlockIterator(
     }
   } else {
     assert(block.value == nullptr);
-    if (input_iter != nullptr) {
-      input_iter->SetStatus(s);
-      iter = input_iter;
-    } else {
-      iter = NewErrorInternalIterator(s);
-    }
+    iter->SetStatus(s);
   }
   return iter;
 }

table/block_based_table_reader.h

@@ -215,11 +215,11 @@ class BlockBasedTable : public TableReader {
  private:
   friend class MockedBlockBasedTable;
   // input_iter: if it is not null, update this one and return it as Iterator
-  static InternalIterator* NewDataBlockIterator(
+  static BlockIter* NewDataBlockIterator(
       Rep* rep, const ReadOptions& ro, const Slice& index_value,
       BlockIter* input_iter = nullptr, bool is_index = false,
       GetContext* get_context = nullptr);
-  static InternalIterator* NewDataBlockIterator(
+  static BlockIter* NewDataBlockIterator(
       Rep* rep, const ReadOptions& ro, const BlockHandle& block_hanlde,
       BlockIter* input_iter = nullptr, bool is_index = false,
       GetContext* get_context = nullptr, Status s = Status());