From 6a5e8486b6eea8dd192807d2e0eeb43fadb2f009 Mon Sep 17 00:00:00 2001
From: Andrey Zagrebin <azagrebin@gmail.com>
Date: Thu, 16 Aug 2018 10:44:09 -0700
Subject: [PATCH] Summary: This PR addresses issue #3865 and implements the
 following approach to fix it:  - adds
 `MergeContext::GetOperandsDirectionForward` and
 `MergeContext::GetOperandsDirectionBackward` to query merge operands in a
 specific order  - `MergeContext::GetOperands` becomes a shortcut for
 `MergeContext::GetOperandsDirectionForward`  - pass
 `MergeContext::GetOperandsDirectionBackward` to `MergeOperator::ShouldMerge`
 and document the order Pull Request resolved:
 https://github.com/facebook/rocksdb/pull/4266

Differential Revision: D9360750

Pulled By: sagar0

fbshipit-source-id: 20cb73ff017760b062ecdcf4382560767086e092
---
 HISTORY.md                       |  5 +++++
 db/memtable.cc                   |  2 +-
 db/merge_context.h               | 17 ++++++++++++++++-
 include/rocksdb/merge_operator.h |  5 +++++
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/HISTORY.md b/HISTORY.md
index 9aab2033e..e4658089d 100644
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -1,4 +1,9 @@
 # Rocksdb Change Log
+
+## 5.14.3 (8/21/2018)
+### Public API Change
+* The merge operands are passed to `MergeOperator::ShouldMerge` in the reversed order relative to how they were merged (passed to FullMerge or FullMergeV2) for performance reasons
+
 ## 5.14.2 (7/3/2018)
 ### Bug Fixes
 * Change default value of `bytes_max_delete_chunk` to 0 in NewSstFileManager() as it doesn't work well with checkpoints.
diff --git a/db/memtable.cc b/db/memtable.cc
index f2d2881d9..306109303 100644
--- a/db/memtable.cc
+++ b/db/memtable.cc
@@ -696,7 +696,7 @@ static bool SaveValue(void* arg, const char* entry) {
         *(s->merge_in_progress) = true;
         merge_context->PushOperand(
             v, s->inplace_update_support == false /* operand_pinned */);
-        if (merge_operator->ShouldMerge(merge_context->GetOperands())) {
+        if (merge_operator->ShouldMerge(merge_context->GetOperandsDirectionBackward())) {
           *(s->status) = MergeHelper::TimedFullMerge(
               merge_operator, s->key->user_key(), nullptr,
               merge_context->GetOperands(), s->value, s->logger, s->statistics,
diff --git a/db/merge_context.h b/db/merge_context.h
index 5e75e0997..c226f64e5 100644
--- a/db/merge_context.h
+++ b/db/merge_context.h
@@ -74,8 +74,13 @@ class MergeContext {
     return (*operand_list_)[index];
   }
 
-  // Return all the operands.
+  // Same as GetOperandsDirectionForward
   const std::vector<Slice>& GetOperands() {
+    return GetOperandsDirectionForward();
+  }
+
+  // Return all the operands in the order as they were merged (passed to FullMerge or FullMergeV2)
+  const std::vector<Slice>& GetOperandsDirectionForward() {
     if (!operand_list_) {
       return empty_operand_list;
     }
@@ -84,6 +89,16 @@ class MergeContext {
     return *operand_list_;
   }
 
+  // Return all the operands in the reversed order relative to how they were merged (passed to FullMerge or FullMergeV2)
+  const std::vector<Slice>& GetOperandsDirectionBackward() {
+    if (!operand_list_) {
+      return empty_operand_list;
+    }
+
+    SetDirectionBackward();
+    return *operand_list_;
+  }
+
  private:
   void Initialize() {
     if (!operand_list_) {
diff --git a/include/rocksdb/merge_operator.h b/include/rocksdb/merge_operator.h
index cd7563cff..8406d4a74 100644
--- a/include/rocksdb/merge_operator.h
+++ b/include/rocksdb/merge_operator.h
@@ -195,6 +195,11 @@ class MergeOperator {
   // during a point lookup, thereby helping in limiting the number of levels to
   // read from.
   // Doesn't help with iterators.
+  //
+  // Note: the merge operands are passed to this function in the reversed order
+  // relative to how they were merged (passed to FullMerge or FullMergeV2)
+  // for performance reasons, see also:
+  // https://github.com/facebook/rocksdb/issues/3865
   virtual bool ShouldMerge(const std::vector<Slice>& /*operands*/) const {
     return false;
   }