From 5a29838cc8b7645f83597f3b5b638d6458c14c5f Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Fri, 27 Dec 2024 14:17:17 -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 4b66da6226f..c6734cfc872 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 c419b98a2d1..0ad11411951 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 956dba7762e..78463c22cd4 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 fa1891c2a58..6f5f85384a3 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 1f3a64248e0..fd1715ea0f6 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 {