Revert "Fix compile"

This reverts commit 51440f83ec.

Revert "Re-generate WriteEntry on WBWIIterator::Entry()"

This reverts commit 4949ef08db.

HISTORY.md

@@ -11,7 +11,6 @@
 * DB::CompactRange()'s parameter reduce_level is changed to change_level, to allow users to move levels to lower levels if allowed. It can be used to migrate a DB from options.level_compaction_dynamic_level_bytes=false to options.level_compaction_dynamic_level_bytes.true.
 * Change default value for options.compaction_filter_factory and options.compaction_filter_factory_v2 to nullptr instead of DefaultCompactionFilterFactory and DefaultCompactionFilterFactoryV2.
 * If CancelAllBackgroundWork is called without doing a flush after doing loads with WAL disabled, the changes which haven't been flushed before the call to CancelAllBackgroundWork will be lost.
-* WBWIIterator::Entry() now returns WriteEntry instead of `const WriteEntry&`
 
 ## 3.11.0 (5/19/2015)
 ### New Features

include/rocksdb/utilities/write_batch_with_index.h

@@ -55,9 +55,7 @@ class WBWIIterator {
 
   virtual void Prev() = 0;
 
-  // the return WriteEntry is only valid until the next mutation of
-  // WriteBatchWithIndex
-  virtual WriteEntry Entry() const = 0;
+  virtual const WriteEntry& Entry() const = 0;
 
   virtual Status status() const = 0;
 };

utilities/write_batch_with_index/write_batch_with_index.cc

@@ -89,8 +89,7 @@ class BaseDeltaIterator : public Iterator {
         AdvanceBase();
       }
       if (DeltaValid() && BaseValid()) {
-        if (comparator_->Compare(delta_iterator_->Entry().key,
-                                 base_iterator_->key()) == 0) {
+        if (Compare() == 0) {
           equal_keys_ = true;
         }
       }
@@ -124,8 +123,7 @@ class BaseDeltaIterator : public Iterator {
         AdvanceBase();
       }
       if (DeltaValid() && BaseValid()) {
-        if (comparator_->Compare(delta_iterator_->Entry().key,
-                                 base_iterator_->key()) == 0) {
+        if (Compare() == 0) {
           equal_keys_ = true;
         }
       }
@@ -155,6 +153,23 @@ class BaseDeltaIterator : public Iterator {
   }
 
  private:
+  // -1 -- delta less advanced than base
+  // 0 -- delta == base
+  // 1 -- delta more advanced than base
+  int Compare() const {
+    assert(delta_iterator_->Valid() && base_iterator_->Valid());
+    int cmp = comparator_->Compare(delta_iterator_->Entry().key,
+                                   base_iterator_->key());
+    if (forward_) {
+      return cmp;
+    } else {
+      return -cmp;
+    }
+  }
+  bool IsDeltaDelete() {
+    assert(DeltaValid());
+    return delta_iterator_->Entry().type == kDeleteRecord;
+  }
   void AssertInvariants() {
 #ifndef NDEBUG
     if (!Valid()) {
@@ -224,10 +239,6 @@ class BaseDeltaIterator : public Iterator {
   bool DeltaValid() const { return delta_iterator_->Valid(); }
   void UpdateCurrent() {
     while (true) {
-      WriteEntry delta_entry;
-      if (DeltaValid()) {
-        delta_entry = delta_iterator_->Entry();
-      }
       equal_keys_ = false;
       if (!BaseValid()) {
         // Base has finished.
@@ -235,7 +246,7 @@ class BaseDeltaIterator : public Iterator {
           // Finished
           return;
         }
-        if (delta_entry.type == kDeleteRecord) {
+        if (IsDeltaDelete()) {
           AdvanceDelta();
         } else {
           current_at_base_ = false;
@@ -246,14 +257,12 @@ class BaseDeltaIterator : public Iterator {
         current_at_base_ = true;
         return;
       } else {
-        int compare =
-            (forward_ ? 1 : -1) *
-            comparator_->Compare(delta_entry.key, base_iterator_->key());
+        int compare = Compare();
         if (compare <= 0) {  // delta bigger or equal
           if (compare == 0) {
             equal_keys_ = true;
           }
-          if (delta_entry.type == kDeleteRecord) {
+          if (!IsDeltaDelete()) {
             current_at_base_ = false;
             return;
           }
@@ -291,26 +300,23 @@ class WBWIIteratorImpl : public WBWIIterator {
                    const ReadableWriteBatch* write_batch)
       : column_family_id_(column_family_id),
         skip_list_iter_(skip_list),
-        write_batch_(write_batch) {}
+        write_batch_(write_batch),
+        valid_(false) {}
 
   virtual ~WBWIIteratorImpl() {}
 
-  virtual bool Valid() const override {
-    if (!skip_list_iter_.Valid()) {
-      return false;
-    }
-    const WriteBatchIndexEntry* iter_entry = skip_list_iter_.key();
-    return (iter_entry != nullptr &&
-            iter_entry->column_family == column_family_id_);
-  }
+  virtual bool Valid() const override { return valid_; }
 
   virtual void SeekToFirst() override {
+    valid_ = true;
     WriteBatchIndexEntry search_entry(WriteBatchIndexEntry::kFlagMin,
                                       column_family_id_);
     skip_list_iter_.Seek(&search_entry);
+    ReadEntry();
   }
 
   virtual void SeekToLast() override {
+    valid_ = true;
     WriteBatchIndexEntry search_entry(WriteBatchIndexEntry::kFlagMin,
                                       column_family_id_ + 1);
     skip_list_iter_.Seek(&search_entry);
@@ -319,38 +325,30 @@ class WBWIIteratorImpl : public WBWIIterator {
     } else {
       skip_list_iter_.Prev();
     }
+    ReadEntry();
   }
 
   virtual void Seek(const Slice& key) override {
+    valid_ = true;
     WriteBatchIndexEntry search_entry(&key, column_family_id_);
     skip_list_iter_.Seek(&search_entry);
+    ReadEntry();
   }
 
-  virtual void Next() override { skip_list_iter_.Next(); }
-
-  virtual void Prev() override { skip_list_iter_.Prev(); }
-
-  virtual WriteEntry Entry() const override {
-    WriteEntry ret;
-    Slice blob;
-    const WriteBatchIndexEntry* iter_entry = skip_list_iter_.key();
-    // this is guaranteed with Valid()
-    assert(iter_entry != nullptr &&
-           iter_entry->column_family == column_family_id_);
-    auto s = write_batch_->GetEntryFromDataOffset(iter_entry->offset, &ret.type,
-                                                  &ret.key, &ret.value, &blob);
-    assert(s.ok());
-    assert(ret.type == kPutRecord || ret.type == kDeleteRecord ||
-           ret.type == kMergeRecord);
-    return ret;
+  virtual void Next() override {
+    skip_list_iter_.Next();
+    ReadEntry();
   }
 
-  virtual Status status() const override {
-    // this is in-memory data structure, so the only way status can be non-ok is
-    // through memory corruption
-    return Status::OK();
+  virtual void Prev() override {
+    skip_list_iter_.Prev();
+    ReadEntry();
   }
 
+  virtual const WriteEntry& Entry() const override { return current_; }
+
+  virtual Status status() const override { return status_; }
+
   const WriteBatchIndexEntry* GetRawEntry() const {
     return skip_list_iter_.key();
   }
@@ -359,6 +357,33 @@ class WBWIIteratorImpl : public WBWIIterator {
   uint32_t column_family_id_;
   WriteBatchEntrySkipList::Iterator skip_list_iter_;
   const ReadableWriteBatch* write_batch_;
+  Status status_;
+  bool valid_;
+  WriteEntry current_;
+
+  void ReadEntry() {
+    if (!status_.ok() || !skip_list_iter_.Valid()) {
+      valid_ = false;
+      return;
+    }
+    const WriteBatchIndexEntry* iter_entry = skip_list_iter_.key();
+    if (iter_entry == nullptr ||
+        iter_entry->column_family != column_family_id_) {
+      valid_ = false;
+      return;
+    }
+    Slice blob;
+    status_ = write_batch_->GetEntryFromDataOffset(
+        iter_entry->offset, &current_.type, &current_.key, &current_.value,
+        &blob);
+    if (!status_.ok()) {
+      valid_ = false;
+    } else if (current_.type != kPutRecord && current_.type != kDeleteRecord &&
+               current_.type != kMergeRecord) {
+      valid_ = false;
+      status_ = Status::Corruption("write batch index is corrupted");
+    }
+  }
 };
 
 struct WriteBatchWithIndex::Rep {

utilities/write_batch_with_index/write_batch_with_index_test.cc

@@ -103,7 +103,7 @@ void TestValueAsSecondaryIndexHelper(std::vector<Entry> entries,
       std::unique_ptr<WBWIIterator> iter(batch->NewIterator(&data));
       iter->Seek(e.key);
       ASSERT_OK(iter->status());
-      auto write_entry = iter->Entry();
+      auto& write_entry = iter->Entry();
       ASSERT_EQ(e.key, write_entry.key.ToString());
       ASSERT_EQ(e.value, write_entry.value.ToString());
       batch->Delete(&data, e.key);
@@ -124,7 +124,7 @@ void TestValueAsSecondaryIndexHelper(std::vector<Entry> entries,
         for (auto v : pair.second) {
           ASSERT_OK(iter->status());
           ASSERT_TRUE(iter->Valid());
-          auto write_entry = iter->Entry();
+          auto& write_entry = iter->Entry();
           ASSERT_EQ(pair.first, write_entry.key.ToString());
           ASSERT_EQ(v->type, write_entry.type);
           if (write_entry.type != kDeleteRecord) {
@@ -140,7 +140,7 @@ void TestValueAsSecondaryIndexHelper(std::vector<Entry> entries,
       for (auto v = pair->second.rbegin(); v != pair->second.rend(); v++) {
         ASSERT_OK(iter->status());
         ASSERT_TRUE(iter->Valid());
-        auto write_entry = iter->Entry();
+        auto& write_entry = iter->Entry();
         ASSERT_EQ(pair->first, write_entry.key.ToString());
         ASSERT_EQ((*v)->type, write_entry.type);
         if (write_entry.type != kDeleteRecord) {
@@ -165,7 +165,7 @@ void TestValueAsSecondaryIndexHelper(std::vector<Entry> entries,
         for (auto v : pair.second) {
           ASSERT_OK(iter->status());
           ASSERT_TRUE(iter->Valid());
-          auto write_entry = iter->Entry();
+          auto& write_entry = iter->Entry();
           ASSERT_EQ(pair.first, write_entry.key.ToString());
           if (v->type != kDeleteRecord) {
             ASSERT_EQ(v->key, write_entry.value.ToString());
@@ -182,7 +182,7 @@ void TestValueAsSecondaryIndexHelper(std::vector<Entry> entries,
       for (auto v = pair->second.rbegin(); v != pair->second.rend(); v++) {
         ASSERT_OK(iter->status());
         ASSERT_TRUE(iter->Valid());
-        auto write_entry = iter->Entry();
+        auto& write_entry = iter->Entry();
         ASSERT_EQ(pair->first, write_entry.key.ToString());
         if ((*v)->type != kDeleteRecord) {
           ASSERT_EQ((*v)->key, write_entry.value.ToString());
@@ -204,7 +204,7 @@ void TestValueAsSecondaryIndexHelper(std::vector<Entry> entries,
       ASSERT_OK(iter->status());
       for (auto v : pair->second) {
         ASSERT_TRUE(iter->Valid());
-        auto write_entry = iter->Entry();
+        auto& write_entry = iter->Entry();
         ASSERT_EQ(pair->first, write_entry.key.ToString());
         ASSERT_EQ(v->type, write_entry.type);
         if (write_entry.type != kDeleteRecord) {
@@ -226,7 +226,7 @@ void TestValueAsSecondaryIndexHelper(std::vector<Entry> entries,
       ASSERT_OK(iter->status());
       for (auto v : pair->second) {
         ASSERT_TRUE(iter->Valid());
-        auto write_entry = iter->Entry();
+        auto& write_entry = iter->Entry();
         ASSERT_EQ(pair->first, write_entry.key.ToString());
         ASSERT_EQ(v->value, write_entry.key.ToString());
         if (v->type != kDeleteRecord) {
@@ -1268,6 +1268,9 @@ TEST_F(WriteBatchWithIndexTest, MutateWhileIteratingBaseCorrectnessTest) {
   AssertIterKey("mm", iter.get());
   AssertIterValue("kk", iter.get());
   batch.Delete("mm");
+  // still mm even though it's deleted
+  AssertIterKey("mm", iter.get());
+  AssertIterValue("kk", iter.get());
   iter->Next();
   AssertIterKey("n", iter.get());
   iter->Prev();