feat(search): Add DD option support for FT.DROPINDEX command #5885
Conversation
src/server/search/search_family.cc
Outdated
| // 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); |
There was a problem hiding this comment.
You don't need this inside the command, you can store the keys inside the transaction callback locally
src/server/search/search_family.cc
Outdated
| if (delete_docs) { | ||
| auto* index = es->search_indices()->GetIndex(idx_name); | ||
| if (index) { | ||
| shard_doc_keys[es->shard_id()] = index->GetAllKeys(); | ||
| } | ||
| } |
There was a problem hiding this comment.
You don't need to copy all the keys, you're gonna delete the index anyway. You can just return the DocKeyIndex by value from DropIndex() and empty it either with a helper function or by moving the ids_ map out of it directly
src/server/search/doc_index.cc
Outdated
| keys.reserve(ids_.size()); | ||
|
|
||
| for (const auto& [key, id] : ids_) { | ||
| if (!key.empty()) { |
There was a problem hiding this comment.
keys shouldn't be empty inside ids_ (only inside keys_), but see previous comment on that we don't need this function
|
LGTM |
src/server/search/search_family.cc
Outdated
| // TODO: Handle optional DD param | ||
|
|
||
| // Parse optional DD (Delete Documents) parameter | ||
| bool delete_docs = false; |
There was a problem hiding this comment.
bool delete_docs = args.size() > 1 && absl::EqualsIgnoreCase(args[1], "DD")
src/server/search/search_family.cc
Outdated
| // 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()) { |
There was a problem hiding this comment.
no need for the if, you can just iterate over it right away
Implements the
DD(Delete Documents) option forFT.DROPINDEXcommand.Fixes: #5513