From 65abdf3430ba16b25c578d7ff468c6249e35f7c6 Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Fri, 27 Dec 2024 17:11:19 -0800 Subject: [PATCH] Remove the primary_key parameter of SecondaryIndex::GetSecondary{KeyPrefix,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. Differential Revision: D67184936 --- include/rocksdb/utilities/secondary_index.h | 5 ++--- utilities/secondary_index/faiss_ivf_index.cc | 5 ++--- utilities/secondary_index/faiss_ivf_index.h | 5 ++--- utilities/secondary_index/secondary_index_mixin.h | 7 +++---- utilities/transactions/transaction_test.cc | 5 ++--- 5 files changed, 11 insertions(+), 16 deletions(-) diff --git a/include/rocksdb/utilities/secondary_index.h b/include/rocksdb/utilities/secondary_index.h index 4b66da6226ff..c6734cfc8722 100644 --- 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* 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>* secondary_value) const = 0; }; diff --git a/utilities/secondary_index/faiss_ivf_index.cc b/utilities/secondary_index/faiss_ivf_index.cc index c419b98a2d1c..0ad11411951d 100644 --- 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* 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>* secondary_value) const { assert(secondary_value); diff --git a/utilities/secondary_index/faiss_ivf_index.h b/utilities/secondary_index/faiss_ivf_index.h index 956dba7762ed..78463c22cd4c 100644 --- 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* 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>* secondary_value) const override; diff --git a/utilities/secondary_index/secondary_index_mixin.h b/utilities/secondary_index/secondary_index_mixin.h index fa1891c2a588..6f5f85384a3d 100644 --- 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 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 index 1f3a64248e08..fd1715ea0f67 100644 --- 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* 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>* secondary_value) const override {