-
Notifications
You must be signed in to change notification settings - Fork 356
Add Azure SQL Advise Command #333
Conversation
|
Please also update docs/azmcp-commands.md. |
wbreza
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 overall - just a few low priority comments. Main comment is around whether or not we need to re-expose types for the command results vs leveraging existing objects from the SDKs. Are we trimming down the amount of data or other reasons?
anuchandy
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.
LGTM, ty!
| { | ||
| DatabaseNotFoundException => "Database not found. Verify the database exists and you have access.", | ||
| _ => base.GetErrorMessage(ex) | ||
| }; internal record DbAdviseCommandResult(SqlAnalysisResult Analysis) : IDbAdviseCommandResult |
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.
nit; should this record definition be in a new line?
src/Services/Azure/Sql/SqlService.cs
Outdated
| var patterns = new[] | ||
| { | ||
| @"ON\s+\[?([^\]\s\[]+)\]?\.\[?([^\]\s\[]+)\]?", // ON [schema].[table] or ON schema.table | ||
| @"ON\s+\[?([^\]\s\[]+)\]?(?!\s*\.)", // ON [table] (without schema) |
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.
In Java, we try to compile the regex upfront so it can help with perf.
Reading the doc, .NET seems to have similar concept https://learn.microsoft.com/en-us/dotnet/standard/base-types/regular-expression-source-generators#compiled-regular-expressions, so we could avoid runtime regx compilation (and is AOT friendly).
// / ON [schema].[table] or ON schema.table
[GeneratedRegex(@"ON\s+\[?([^\]\s\[]+)\]?\.\[?([^\]\s\[]+)\]?", RegexOptions.IgnoreCase)]
private static partial Regex NameFromTableSchemaRegex();
// // ON [table] (without schema)
[GeneratedRegex(@"ON\s+\[?([^\]\s\[]+)\]?(?!\s*\.)", RegexOptions.IgnoreCase)]
private static partial Regex NameFromTableOnlyRegex();var match = NameFromTableSchemaRegex().Match(sql);
// rest is the same code.|
We are waiting for a db to be available that has auto tuning recommendations so we can test this. It takes 7 days for a db to show auto-tuning recommendations, so we'll verify it next week. |
…ex recommendations - Introduced new SQL command groups for managing Azure SQL resources. - Added commands for listing SQL servers and databases. - Implemented index recommendation functionality with appropriate options. - Created base command classes for SQL operations to streamline command creation. - Enhanced error handling for SQL operations with custom exceptions. - Updated documentation to reflect new command structure and usage. - Added unit tests for SQL index recommendation command to ensure functionality.
…g; deprecate old model
…thods and improve error handling
…g across SQL command files
…oup parameter and updating related methods
… handling in SqlService; add unit tests for SQL service functionality
…g SQL servers and databases; add SqlAuthorizationException for error handling
…Command and BaseServerCommand; update index recommendation logic and options handling
…ance index recommendation command with tenant support
…structure and additional metadata; update related commands and tests
- Removed the SqlIndexRecommendCommand and its associated tests. - Introduced DbAdviseCommand to provide SQL database advisor recommendations. - Updated CommandFactory to register new Db commands and removed obsolete index commands. - Created BaseDbCommand for shared functionality among database commands. - Implemented DbListCommand for listing SQL databases. - Added ServerListCommand for listing SQL servers. - Updated SqlService to support advisor type filtering in GetIndexRecommendationsAsync. - Refactored SQL option definitions to include advisor type options. - Created new tests for DbAdviseCommand to ensure correct functionality and error handling. - Removed outdated SqlIndexRecommendCommandTests.
…and tests with improved analysis result handling
…ndation handling; replace index analysis with general recommendations
…emove deprecated SqlIndexRecommendation model and update related tests
…AdviseCommand, DatabaseAdviseOptions, SqlService, ISqlService, and DbAdviseCommandTests
…null handling and code simplification
… simplifying method definitions
…descriptions and parameter naming
…ge assertions and validation checks
Co-authored-by: Xiang Yan <[email protected]>
…azmcp-commands.md
…null check in OnCallTools
…arser utility class with tests
jongio
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.
I'm still working on getting an env that has db recommendations so I can test this.
|
This is outdated. Will discuss options with SQL team. |
Adding first Azure SQL command that will be useful for developers wanting to improve their SQL implementation.
User Scenario: "help me optimize my azure sql db"
This tool will return optimization recommendations from sql advisor.