Remove the primary_key parameter of SecondaryIndex::GetSecondary{KeyP…

…refix,Value} (#13207) Summary: The patch tweaks the new `SecondaryIndex` interface a bit by removing the `primary_key` parameter of `GetSecondaryKeyPrefix` and `GetSecondaryValue`. This parameter is currently unused by existing implementations and it actually does not make sense to have the secondary index prefix depend on the primary key since it would lead to potential chicken-and-egg problems at query time. Pull Request resolved: #13207 Reviewed By: jaykorean Differential Revision: D67184936 fbshipit-source-id: 5707a35225a0160132e5e87e9fe6c36bee5eada1
facebook · Dec 30, 2024 · 3570e4f · 3570e4f
1 parent 02b4197
commit 3570e4f
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 16 deletions.
diff --git a/include/rocksdb/utilities/secondary_index.h b/include/rocksdb/utilities/secondary_index.h
@@ -82,7 +82,7 @@ class SecondaryIndex {
   // index id or length indicator). Returning a non-OK status rolls back all
   // operations in the transaction related to this primary key-value.
   virtual Status GetSecondaryKeyPrefix(
-      const Slice& primary_key, const Slice& primary_column_value,
+      const Slice& primary_column_value,
       std::variant<Slice, std::string>* secondary_key_prefix) const = 0;
 
   // Get the optional secondary value for a given primary key-value. This method
@@ -93,8 +93,7 @@ class SecondaryIndex {
   // Returning a non-OK status rolls back all operations in the transaction
   // related to this primary key-value.
   virtual Status GetSecondaryValue(
-      const Slice& primary_key, const Slice& primary_column_value,
-      const Slice& previous_column_value,
+      const Slice& primary_column_value, const Slice& previous_column_value,
       std::optional<std::variant<Slice, std::string>>* secondary_value)
       const = 0;
 };

diff --git a/utilities/secondary_index/faiss_ivf_index.cc b/utilities/secondary_index/faiss_ivf_index.cc
@@ -165,7 +165,7 @@ Status FaissIVFIndex::UpdatePrimaryColumnValue(
 }
 
 Status FaissIVFIndex::GetSecondaryKeyPrefix(
-    const Slice& /* primary_key */, const Slice& primary_column_value,
+    const Slice& primary_column_value,
     std::variant<Slice, std::string>* secondary_key_prefix) const {
   assert(secondary_key_prefix);
 
@@ -180,8 +180,7 @@ Status FaissIVFIndex::GetSecondaryKeyPrefix(
 }
 
 Status FaissIVFIndex::GetSecondaryValue(
-    const Slice& /* primary_key */, const Slice& primary_column_value,
-    const Slice& original_column_value,
+    const Slice& primary_column_value, const Slice& original_column_value,
     std::optional<std::variant<Slice, std::string>>* secondary_value) const {
   assert(secondary_value);
 

diff --git a/utilities/secondary_index/faiss_ivf_index.h b/utilities/secondary_index/faiss_ivf_index.h
@@ -35,11 +35,10 @@ class FaissIVFIndex : public SecondaryIndex {
       const override;
 
   Status GetSecondaryKeyPrefix(
-      const Slice& primary_key, const Slice& primary_column_value,
+      const Slice& primary_column_value,
       std::variant<Slice, std::string>* secondary_key_prefix) const override;
 
-  Status GetSecondaryValue(const Slice& primary_key,
-                           const Slice& primary_column_value,
+  Status GetSecondaryValue(const Slice& primary_column_value,
                            const Slice& original_column_value,
                            std::optional<std::variant<Slice, std::string>>*
                                secondary_value) const override;

diff --git a/utilities/secondary_index/secondary_index_mixin.h b/utilities/secondary_index/secondary_index_mixin.h
@@ -235,7 +235,7 @@ class SecondaryIndexMixin : public Txn {
     std::variant<Slice, std::string> secondary_key_prefix;
 
     const Status s = secondary_index->GetSecondaryKeyPrefix(
-        primary_key, existing_primary_column_value, &secondary_key_prefix);
+        existing_primary_column_value, &secondary_key_prefix);
     if (!s.ok()) {
       return s;
     }
@@ -268,7 +268,7 @@ class SecondaryIndexMixin : public Txn {
 
     {
       const Status s = secondary_index->GetSecondaryKeyPrefix(
-          primary_key, primary_column_value, &secondary_key_prefix);
+          primary_column_value, &secondary_key_prefix);
       if (!s.ok()) {
         return s;
       }
@@ -278,8 +278,7 @@ class SecondaryIndexMixin : public Txn {
 
     {
       const Status s = secondary_index->GetSecondaryValue(
-          primary_key, primary_column_value, previous_column_value,
-          &secondary_value);
+          primary_column_value, previous_column_value, &secondary_value);
       if (!s.ok()) {
         return s;
       }

diff --git a/utilities/transactions/transaction_test.cc b/utilities/transactions/transaction_test.cc
@@ -8054,7 +8054,7 @@ TEST_P(TransactionTest, SecondaryIndex) {
     }
 
     Status GetSecondaryKeyPrefix(
-        const Slice& /* primary_key */, const Slice& primary_column_value,
+        const Slice& primary_column_value,
         std::variant<Slice, std::string>* secondary_key_prefix) const override {
       assert(secondary_key_prefix);
 
@@ -8068,8 +8068,7 @@ TEST_P(TransactionTest, SecondaryIndex) {
       return Status::OK();
     }
 
-    Status GetSecondaryValue(const Slice& /* primary_key */,
-                             const Slice& primary_column_value,
+    Status GetSecondaryValue(const Slice& primary_column_value,
                              const Slice& previous_column_value,
                              std::optional<std::variant<Slice, std::string>>*
                                  secondary_value) const override {