Skip to content

feat: Add drop-search-index tool #243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 14, 2025

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