fix: review comments

Author: vyavdoshenko

src/server/search/doc_index.cc

@@ -242,19 +242,6 @@ size_t ShardDocIndex::DocKeyIndex::Size() const {
   return ids_.size();
 }
 
-std::vector<std::string> ShardDocIndex::DocKeyIndex::GetAllKeys() const {
-  std::vector<std::string> keys;
-  keys.reserve(ids_.size());
-
-  for (const auto& [key, id] : ids_) {
-    if (!key.empty()) {
-      keys.push_back(key);
-    }
-  }
-
-  return keys;
-}
-
 uint8_t DocIndex::GetObjCode() const {
   return type == JSON ? OBJ_JSON : OBJ_HASH;
 }
@@ -620,10 +607,6 @@ DocIndexInfo ShardDocIndex::GetInfo() const {
   return {*base_, key_index_.Size()};
 }
 
-std::vector<std::string> ShardDocIndex::GetAllKeys() const {
-  return key_index_.GetAllKeys();
-}
-
 io::Result<StringVec, ErrorReply> ShardDocIndex::GetTagVals(string_view field) const {
   search::BaseIndex* base_index = indices_->GetIndex(field);
   if (base_index == nullptr) {
@@ -662,15 +645,16 @@ void ShardDocIndices::InitIndex(const OpArgs& op_args, std::string_view name,
       });
 }
 
-bool ShardDocIndices::DropIndex(string_view name) {
+unique_ptr<ShardDocIndex> ShardDocIndices::DropIndex(string_view name) {
   auto it = indices_.find(name);
   if (it == indices_.end())
-    return false;
+    return nullptr;
 
   DropIndexCache(*it->second);
+  auto index = std::move(it->second);
   indices_.erase(it);
 
-  return true;
+  return index;
 }
 
 void ShardDocIndices::DropAllIndices() {

src/server/search/doc_index.h

@@ -224,8 +224,10 @@ class ShardDocIndex {
     std::string_view Get(DocId id) const;
     size_t Size() const;
 
-    // Get all keys in the index
-    std::vector<std::string> GetAllKeys() const;
+    // Get const reference to the internal ids map
+    const absl::flat_hash_map<std::string, DocId>& GetDocKeysMap() const {
+      return ids_;
+    }
 
    private:
     absl::flat_hash_map<std::string, DocId> ids_;
@@ -265,9 +267,6 @@ class ShardDocIndex {
 
   DocIndexInfo GetInfo() const;
 
-  // Get all document keys in this index
-  std::vector<std::string> GetAllKeys() const;
-
   io::Result<StringVec, facade::ErrorReply> GetTagVals(std::string_view field) const;
 
   // Get synonym manager for this shard
@@ -283,6 +282,11 @@ class ShardDocIndex {
   void RebuildForGroup(const OpArgs& op_args, const std::string_view& group_id,
                        const std::vector<std::string_view>& terms);
 
+  // Public access to key index for direct operations (e.g., when dropping index with DD)
+  const DocKeyIndex& key_index() const {
+    return key_index_;
+  }
+
  private:
   // Clears internal data. Traverses all matching documents and assigns ids.
   void Rebuild(const OpArgs& op_args, PMR_NS::memory_resource* mr);
@@ -315,8 +319,8 @@ class ShardDocIndices {
   void InitIndex(const OpArgs& op_args, std::string_view name,
                  std::shared_ptr<const DocIndex> index);
 
-  // Drop index, return true if it existed and was dropped
-  bool DropIndex(std::string_view name);
+  // Drop index, return the dropped index if it existed or nullptr otherwise
+  std::unique_ptr<ShardDocIndex> DropIndex(std::string_view name);
 
   // Drop all indices
   void DropAllIndices();

src/server/search/search_family.cc

@@ -1120,7 +1120,8 @@ void SearchFamily::FtAlter(CmdArgList args, const CommandContext& cmd_cntx) {
   // Rebuild index
   // TODO: Introduce partial rebuild
   auto upd_cb = [idx_name, index_info](Transaction* tx, EngineShard* es) {
-    es->search_indices()->DropIndex(idx_name);
+    // Drop the old index (we don't need the documents)
+    (void)es->search_indices()->DropIndex(idx_name);
     es->search_indices()->InitIndex(tx->GetOpArgs(es), idx_name, index_info);
     return OpStatus::OK;
   };
@@ -1142,31 +1143,26 @@ void SearchFamily::FtDropIndex(CmdArgList args, const CommandContext& cmd_cntx)
     }
   }
 
-  // Collect document keys (if DD is set), drop index, and delete documents in single transaction
-  vector<vector<string>> shard_doc_keys(delete_docs ? shard_set->size() : 0);
   atomic_uint num_deleted{0};
 
   auto cb = [&](Transaction* t, EngineShard* es) {
-    // Step 1: If DD is set, collect all document keys before dropping the index
-    if (delete_docs) {
-      auto* index = es->search_indices()->GetIndex(idx_name);
-      if (index) {
-        shard_doc_keys[es->shard_id()] = index->GetAllKeys();
-      }
-    }
+    // Drop the index and get its pointer
+    auto index = es->search_indices()->DropIndex(idx_name);
+    if (!index)
+      return OpStatus::OK;
 
-    // Step 2: Drop the index
-    if (es->search_indices()->DropIndex(idx_name))
-      num_deleted.fetch_add(1);
+    num_deleted.fetch_add(1);
 
-    // Step 3: If DD is set, delete all documents that were in the index
+    // If DD is set, delete all documents that were in the index
     if (delete_docs) {
-      const auto& keys = shard_doc_keys[es->shard_id()];
-      if (!keys.empty()) {
+      // Get const reference to document keys map (index will be destroyed after this scope)
+      const auto& doc_keys = index->key_index().GetDocKeysMap();
+
+      if (!doc_keys.empty()) {
         auto op_args = t->GetOpArgs(es);
         auto& db_slice = op_args.GetDbSlice();
 
-        for (const auto& key : keys) {
+        for (const auto& [key, doc_id] : doc_keys) {
           auto it = db_slice.FindMutable(op_args.db_cntx, key).it;
           if (IsValid(it)) {
             db_slice.Del(op_args.db_cntx, it);