Skip to content

Conversation

mihirmpatil
Copy link
Collaborator

@mihirmpatil mihirmpatil commented May 13, 2025

Add drop-search-index tool

Tested manually with VS code + Copilot and Claude 3.5
Unit/Integration tests will be added in a separate PR

@mihirmpatil mihirmpatil requested a review from a team as a code owner May 13, 2025 20:33

protected async execute({ database, collection, indexName }: ToolArgs<typeof this.argsShape>): Promise<CallToolResult> {
const provider = await this.ensureConnected();
const result = await provider.dropSearchIndex(database, collection, indexName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should the tool be a bit smarter and check whether the index exists first? If it doesn't, maybe we notify the LLM? I guess the more context LLM has, the better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The provider already checks and returns an error if the index doesn't exist

Copy link
Collaborator

@manoj-pillay-10gen manoj-pillay-10gen left a comment

Choose a reason for hiding this comment

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

Please also look at Github actions checkstyle suggestions

"Deletes a text or vector search index from the database.";
protected argsShape = {
...DbOperationArgs,
indexName: z.string().describe("The name of the search or vector index to delete"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need to repeat this often, so let's register a

export const SearchIndexOperationArgs = {
    database: z.string().describe("Database name"),
    collection: z.string().describe("Collection name"),
    searchIndexName: z.string().describe("Search or VectorSearch Index Name"); 
};

in mongodbTool.ts and import it here and use it on L16 and anywhere else we need.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, see 17f9022

return {
content: [
{
text: `"Successfully dropped index "${indexName}" from database "${database}"`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: looks like we don't need double quotes

`"Successfully - before
`Successfully - after

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, see 17f9022

Copy link
Collaborator

@edgarw19 edgarw19 left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Collaborator

@vm-mishchenko vm-mishchenko left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@manoj-pillay-10gen manoj-pillay-10gen left a comment

Choose a reason for hiding this comment

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

LGTM

@mihirmpatil mihirmpatil changed the title Add drop-search-index tool feat: Add drop-search-index tool May 14, 2025
@mihirmpatil mihirmpatil merged commit fa62241 into search-skunkworks-2025 May 14, 2025
7 checks passed
@mihirmpatil mihirmpatil deleted the mihirmpatil/del-search-index branch May 14, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants