Improve documentation for MergingIterator (#11161)

Summary:
Add some comments to try to explain how/why MergingIterator works. Made some small refactoring, mostly in MergingIterator::SkipNextDeleted() and MergingIterator::SeekImpl().

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

Test Plan:
crash test with small key range:
```
python3 tools/db_crashtest.py blackbox --simple --max_key=100 --interval=6000 --write_buffer_size=262144 --target_file_size_base=256 --max_bytes_for_level_base=262144 --block_size=128 --value_size_mult=33 --subcompactions=10 --use_multiget=1 --delpercent=3 --delrangepercent=2 --verify_iterator_with_expected_state_one_in=2 --num_iterations=10
```

Reviewed By: ajkr

Differential Revision: D42860994

Pulled By: cbi42

fbshipit-source-id: 3f0c1c9c6481a7f468bf79d823998907a8116e9e

db/range_del_aggregator.cc

@@ -30,6 +30,8 @@ TruncatedRangeDelIterator::TruncatedRangeDelIterator(
       icmp_(icmp),
       smallest_ikey_(smallest),
       largest_ikey_(largest) {
+  // Set up bounds such that range tombstones from this iterator are
+  // truncated to range [smallest_, largest_).
   if (smallest != nullptr) {
     pinned_bounds_.emplace_back();
     auto& parsed_smallest = pinned_bounds_.back();
@@ -64,6 +66,8 @@ TruncatedRangeDelIterator::TruncatedRangeDelIterator(
       //
       // Therefore, we will never truncate a range tombstone at largest, so we
       // can leave it unchanged.
+      // TODO: maybe use kMaxValid here to ensure range tombstone having
+      //  distinct key from point keys.
     } else {
       // The same user key may straddle two sstable boundaries. To ensure that
       // the truncated end key can cover the largest key in this sstable, reduce
@@ -102,6 +106,24 @@ void TruncatedRangeDelIterator::Seek(const Slice& target) {
   iter_->Seek(target);
 }
 
+void TruncatedRangeDelIterator::SeekInternalKey(const Slice& target) {
+  if (largest_ && icmp_->Compare(*largest_, target) <= 0) {
+    iter_->Invalidate();
+    return;
+  }
+  if (smallest_ && icmp_->Compare(target, *smallest_) < 0) {
+    // Since target < smallest, target < largest_.
+    // This seek must land on a range tombstone where end_key() > target,
+    // so there is no need to check again.
+    iter_->Seek(smallest_->user_key);
+  } else {
+    iter_->Seek(ExtractUserKey(target));
+    while (Valid() && icmp_->Compare(end_key(), target) <= 0) {
+      Next();
+    }
+  }
+}
+
 // NOTE: target is a user key, with timestamp if enabled.
 void TruncatedRangeDelIterator::SeekForPrev(const Slice& target) {
   if (smallest_ != nullptr &&

db/range_del_aggregator.h

@@ -49,6 +49,9 @@ class TruncatedRangeDelIterator {
   // REQUIRES: target is a user key.
   void Seek(const Slice& target);
 
+  // Seeks to the first range tombstone with end_key() > target.
+  void SeekInternalKey(const Slice& target);
+
   // Seeks to the tombstone with the highest visible sequence number that covers
   // target (a user key). If no such tombstone exists, the position will be at
   // the latest tombstone that starts before target.

table/merging_iterator.h

@@ -24,7 +24,7 @@ template <class TValue>
 class InternalIteratorBase;
 using InternalIterator = InternalIteratorBase<Slice>;
 
-// Return an iterator that provided the union of the data in
+// Return an iterator that provides the union of the data in
 // children[0,n-1].  Takes ownership of the child iterators and
 // will delete them when the result iterator is deleted.
 //
@@ -36,11 +36,15 @@ extern InternalIterator* NewMergingIterator(
     const InternalKeyComparator* comparator, InternalIterator** children, int n,
     Arena* arena = nullptr, bool prefix_seek_mode = false);
 
+// The iterator returned by NewMergingIterator() and
+// MergeIteratorBuilder::Finish(). MergingIterator handles the merging of data
+// from different point and/or range tombstone iterators.
 class MergingIterator;
 
-// A builder class to build a merging iterator by adding iterators one by one.
-// User should call only one of AddIterator() or AddPointAndTombstoneIterator()
-// exclusively for the same builder.
+// A builder class to for an iterator that provides the union of data
+// of input iterators. Two APIs are provided to add input iterators. User should
+// only call one of them exclusively depending on if range tombstone should be
+// processed.
 class MergeIteratorBuilder {
  public:
   // comparator: the comparator used in merging comparator
@@ -50,7 +54,7 @@ class MergeIteratorBuilder {
                                 const Slice* iterate_upper_bound = nullptr);
   ~MergeIteratorBuilder();
 
-  // Add iter to the merging iterator.
+  // Add point key iterator `iter` to the merging iterator.
   void AddIterator(InternalIterator* iter);
 
   // Add a point key iterator and a range tombstone iterator.