Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/server/search/doc_index.cc
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,19 @@ 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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keys shouldn't be empty inside ids_ (only inside keys_), but see previous comment on that we don't need this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

keys.push_back(key);
}
}

return keys;
}

uint8_t DocIndex::GetObjCode() const {
return type == JSON ? OBJ_JSON : OBJ_HASH;
}
Expand Down Expand Up @@ -607,6 +620,10 @@ 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) {
Expand Down
6 changes: 6 additions & 0 deletions src/server/search/doc_index.h
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,9 @@ 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;

private:
absl::flat_hash_map<std::string, DocId> ids_;
std::vector<std::string> keys_;
Expand Down Expand Up @@ -262,6 +265,9 @@ 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
Expand Down
47 changes: 44 additions & 3 deletions src/server/search/search_family.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1131,18 +1131,59 @@ void SearchFamily::FtAlter(CmdArgList args, const CommandContext& cmd_cntx) {

void SearchFamily::FtDropIndex(CmdArgList args, const CommandContext& cmd_cntx) {
string_view idx_name = ArgS(args, 0);
// TODO: Handle optional DD param

// Parse optional DD (Delete Documents) parameter
bool delete_docs = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool delete_docs = args.size() > 1 && absl::EqualsIgnoreCase(args[1], "DD")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

if (args.size() > 1) {
string_view option = ArgS(args, 1);
// Only check for DD option, ignore other arguments for compatibility
if (absl::EqualsIgnoreCase(option, "DD")) {
delete_docs = true;
}
}

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this inside the command, you can store the keys inside the transaction callback locally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

atomic_uint num_deleted{0};
cmd_cntx.tx->ScheduleSingleHop([&](Transaction* t, EngineShard* es) {

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();
}
}
Copy link
Contributor

@dranikpg dranikpg Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


// Step 2: Drop the index
if (es->search_indices()->DropIndex(idx_name))
num_deleted.fetch_add(1);

// Step 3: 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()) {
auto op_args = t->GetOpArgs(es);
auto& db_slice = op_args.GetDbSlice();

for (const auto& key : keys) {
auto it = db_slice.FindMutable(op_args.db_cntx, key).it;
if (IsValid(it)) {
db_slice.Del(op_args.db_cntx, it);
}
}
}
}

return OpStatus::OK;
});
};

cmd_cntx.tx->Execute(cb, true);

DCHECK(num_deleted == 0u || num_deleted == shard_set->size());
if (num_deleted == 0u)
return cmd_cntx.rb->SendError("-Unknown Index name");

return cmd_cntx.rb->SendOk();
}

Expand Down
76 changes: 76 additions & 0 deletions src/server/search/search_family_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3328,4 +3328,80 @@ TEST_F(SearchFamilyTest, InvalidConfigOptions) {
EXPECT_THAT(resp, IsArray());
}

TEST_F(SearchFamilyTest, DropIndexWithDD) {
// Create an index on HASH documents
Run({"FT.CREATE", "idx", "ON", "HASH", "PREFIX", "1", "doc:", "SCHEMA", "name", "TEXT"});

// Add some documents
Run({"HSET", "doc:1", "name", "Alice"});
Run({"HSET", "doc:2", "name", "Bob"});
Run({"HSET", "doc:3", "name", "Charlie"});

// Verify documents exist
auto resp = Run({"EXISTS", "doc:1", "doc:2", "doc:3"});
EXPECT_THAT(resp, IntArg(3));

// Verify index works
resp = Run({"FT.SEARCH", "idx", "*"});
EXPECT_THAT(resp, AreDocIds("doc:1", "doc:2", "doc:3"));

// Drop index WITHOUT DD - documents should remain
Run({"FT.DROPINDEX", "idx"});
resp = Run({"EXISTS", "doc:1", "doc:2", "doc:3"});
EXPECT_THAT(resp, IntArg(3));

// Create index again
Run({"FT.CREATE", "idx", "ON", "HASH", "PREFIX", "1", "doc:", "SCHEMA", "name", "TEXT"});

// Verify index works again
resp = Run({"FT.SEARCH", "idx", "*"});
EXPECT_THAT(resp, AreDocIds("doc:1", "doc:2", "doc:3"));

// Drop index WITH DD - documents should be deleted
Run({"FT.DROPINDEX", "idx", "DD"});
resp = Run({"EXISTS", "doc:1", "doc:2", "doc:3"});
EXPECT_THAT(resp, IntArg(0));
}

TEST_F(SearchFamilyTest, DropIndexWithDDJson) {
// Create an index on JSON documents
Run({"FT.CREATE", "jidx", "ON", "JSON", "PREFIX", "1", "jdoc:", "SCHEMA", "$.name", "AS", "name",
"TEXT"});

// Add some JSON documents
Run({"JSON.SET", "jdoc:1", "$", R"({"name": "Alice"})"});
Run({"JSON.SET", "jdoc:2", "$", R"({"name": "Bob"})"});
Run({"JSON.SET", "jdoc:3", "$", R"({"name": "Charlie"})"});

// Verify documents exist
auto resp = Run({"EXISTS", "jdoc:1", "jdoc:2", "jdoc:3"});
EXPECT_THAT(resp, IntArg(3));

// Verify index works
resp = Run({"FT.SEARCH", "jidx", "*"});
EXPECT_THAT(resp, AreDocIds("jdoc:1", "jdoc:2", "jdoc:3"));

// Drop index WITH DD - documents should be deleted
Run({"FT.DROPINDEX", "jidx", "DD"});
resp = Run({"EXISTS", "jdoc:1", "jdoc:2", "jdoc:3"});
EXPECT_THAT(resp, IntArg(0));
}

TEST_F(SearchFamilyTest, DropIndexWithInvalidOption) {
// Create an index
Run({"FT.CREATE", "idx", "ON", "HASH", "PREFIX", "1", "doc:", "SCHEMA", "name", "TEXT"});
Run({"HSET", "doc:1", "name", "test"});

// Drop with unrecognized option (should be ignored, index dropped but documents remain)
auto resp = Run({"FT.DROPINDEX", "idx", "INVALID"});
EXPECT_THAT(resp, "OK");

// Document should still exist
resp = Run({"EXISTS", "doc:1"});
EXPECT_THAT(resp, IntArg(1));

// Clean up
Run({"DEL", "doc:1"});
}

} // namespace dfly
Loading