-
Notifications
You must be signed in to change notification settings - Fork 28
DA-1203 Modified Add Slow Running Queries tools #74
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
DA-1203 Modified Add Slow Running Queries tools #74
Conversation
Summary of ChangesHello @AayushTyagi1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive set of new query analysis tools designed to help identify and diagnose performance issues within the system. By leveraging the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds multiple query analysis tools to diagnose slow-running and inefficient queries by leveraging the system:completed_requests catalog. The change also removes a comment that was preventing the exposure of run_cluster_query to the MCP server.
Key changes:
- Exposed
run_cluster_queryfunction by removing restriction comment - Added seven new query analysis functions for performance diagnostics
- Updated exports in
__init__.pyto include all new query analysis tools
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tools/query.py | Removed comment blocking run_cluster_query exposure; added seven query analysis functions for identifying slow, frequent, large-response, primary-index-using, non-covering-index, and non-selective queries |
| src/tools/init.py | Added imports and exports for all seven new query analysis functions to make them available as tools |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a suite of new tools for analyzing query performance by querying the system:completed_requests catalog. The changes are well-structured, adding new functions in src/tools/query.py and exposing them through src/tools/__init__.py.
My review focuses on several key areas:
- Security: I've identified multiple instances of potential SQL injection vulnerabilities due to the use of f-strings for constructing queries with user-provided parameters. I've provided suggestions to use parameterized queries, which is a critical security best practice.
- Correctness & Consistency: Some of the new query functions have inconsistent filtering logic compared to others. I've suggested changes to align them, ensuring they all exclude system-level queries for more focused and relevant results.
- Maintainability: I've also pointed out a minor style issue in
__init__.pyregarding the sorting of tool lists, which will improve code readability and long-term maintenance.
Overall, these are valuable additions for monitoring database performance. Addressing the feedback will make the new tools more secure, robust, and maintainable.
| WHERE phaseCounts.`primaryScan` IS NOT MISSING | ||
| AND UPPER(statement) NOT LIKE '% SYSTEM:%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other query-analyzing tools in this file, consider filtering out INFER and CREATE INDEX statements. This will help focus the results on application-level queries.
| WHERE phaseCounts.`primaryScan` IS NOT MISSING | |
| AND UPPER(statement) NOT LIKE '% SYSTEM:%' | |
| WHERE phaseCounts.`primaryScan` IS NOT MISSING | |
| AND UPPER(statement) NOT LIKE 'INFER %' | |
| AND UPPER(statement) NOT LIKE 'CREATE INDEX%' | |
| AND UPPER(statement) NOT LIKE '% SYSTEM:%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are adding INFER and SYSTEM, might as well add explain and advise as well- for consistency
| WHERE phaseCounts.`indexScan` IS NOT MISSING | ||
| AND phaseCounts.`fetch` IS NOT MISSING | ||
| AND UPPER(statement) NOT LIKE '% SYSTEM:%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This query is missing filters for INFER and CREATE INDEX statements, which are present in other similar functions. Adding them would provide more consistent and focused results on application queries.
| WHERE phaseCounts.`indexScan` IS NOT MISSING | |
| AND phaseCounts.`fetch` IS NOT MISSING | |
| AND UPPER(statement) NOT LIKE '% SYSTEM:%' | |
| WHERE phaseCounts.`indexScan` IS NOT MISSING | |
| AND phaseCounts.`fetch` IS NOT MISSING | |
| AND UPPER(statement) NOT LIKE 'INFER %' | |
| AND UPPER(statement) NOT LIKE 'CREATE INDEX%' | |
| AND UPPER(statement) NOT LIKE '% SYSTEM:%' |
| SELECT statement, | ||
| AVG(phaseCounts.`indexScan` - resultCount) AS diff | ||
| FROM system:completed_requests | ||
| WHERE phaseCounts.`indexScan` > resultCount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This query is missing filters to exclude system-level statements (like INFER, CREATE INDEX, and queries on SYSTEM: keyspaces). Including these filters, as done in other functions in this file, will help to focus on application performance issues.
| WHERE phaseCounts.`indexScan` > resultCount | |
| WHERE phaseCounts.`indexScan` > resultCount | |
| AND UPPER(statement) NOT LIKE 'INFER %' | |
| AND UPPER(statement) NOT LIKE 'CREATE INDEX%' | |
| AND UPPER(statement) NOT LIKE '% SYSTEM:%' |
| SELECT statement, | ||
| COUNT(1) AS queries | ||
| FROM system:completed_requests | ||
| WHERE UPPER(statement) NOT LIKE 'INFER %' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For every query that we run via UI, we also run explain and advise on it.
So you might also need to filter out explain and advise for top most frequent queries
kaminijagtiani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For every query that we run via UI, we also run explain and advise on it.
So you might also need to filter out explain and advise for top most frequent queries.
And if for consistency you are filtering out infer, create index for all queries- filter out explain and advise as well
|
Resolve Comments. Ready to review @nithishr |
nithishr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some minor nits & we can merge it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Screen.Recording.2025-11-11.at.2.02.48.PM.mov