-
Notifications
You must be signed in to change notification settings - Fork 515
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
Iterative Include SQL Simplification #4699
base: main
Are you sure you want to change the base?
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlCommandSimplifier.cs
Fixed
Show fixed
Hide fixed
src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlCommandSimplifier.cs
Fixed
Show fixed
Hide fixed
src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlCommandSimplifier.cs
Fixed
Show fixed
Hide fixed
src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlCommandSimplifier.cs
Fixed
Show fixed
Hide fixed
src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlCommandSimplifier.cs
Fixed
Show fixed
Hide fixed
catch (Exception ex) | ||
{ | ||
logger.LogWarning(ex, "Exception combining iterative includes."); | ||
return; // Use the unmodified string | ||
} |
Check notice
Code scanning / CodeQL
Generic catch clause Note
if ((tempSortValue as DateTime?) != null) | ||
{ | ||
sortValue = (tempSortValue as DateTime?).Value.ToString("o"); | ||
} | ||
else | ||
{ | ||
sortValue = tempSortValue.ToString(); | ||
} |
Check notice
Code scanning / CodeQL
Missed ternary opportunity Note
@@ -15,6 +15,161 @@ namespace Microsoft.Health.Fhir.SqlServer.Features.Search | |||
{ | |||
internal static class SqlCommandSimplifier | |||
{ | |||
private static readonly Regex FindCteMatch = new Regex(",cte(\\d+) AS\\s*\\r\\n\\s*\\(\\s*\\r\\n\\s*SELECT DISTINCT refTarget.ResourceTypeId AS T1, refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch\\s*\\r\\n\\s*FROM dbo.ReferenceSearchParam refSource\\s*\\r\\n\\s*JOIN dbo.Resource refTarget ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId AND refSource.ReferenceResourceId = refTarget.ResourceId\\s*\\r\\n\\s*WHERE refSource.SearchParamId = (\\d*)\\s*\\r\\n\\s*AND refTarget.IsHistory = 0\\s*\\r\\n\\s*AND refTarget.IsDeleted = 0\\s*\\r\\n\\s*AND refSource.ResourceTypeId IN \\((\\d*)\\)\\s*\\r\\n\\s*AND EXISTS \\(SELECT \\* FROM cte(\\d+) WHERE refSource.ResourceTypeId = T1 AND refSource.ResourceSurrogateId = Sid1"); | |||
|
|||
private const string RemoveCteMatchBase = "(\\s*,cte<CteNumber> AS\\s*\\r\\n\\s*\\(\\s*\\r\\n\\s*SELECT DISTINCT refTarget.ResourceTypeId AS T1, refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch\\s*\\r\\n\\s*FROM dbo.ReferenceSearchParam refSource\\s*\\r\\n\\s*JOIN dbo.Resource refTarget ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId AND refSource.ReferenceResourceId = refTarget.ResourceId\\s*\\r\\n\\s*WHERE refSource.SearchParamId = <SearchParamId>\\s*\\r\\n\\s*AND refTarget.IsHistory = 0\\s*\\r\\n\\s*AND refTarget.IsDeleted = 0\\s*\\r\\n\\s*AND refSource.ResourceTypeId IN \\(<ResourceTypeId>\\)\\s*\\r\\n\\s*AND EXISTS \\(SELECT \\* FROM cte<SourceCte> WHERE refSource.ResourceTypeId = T1 AND refSource.ResourceSurrogateId = Sid1.*\\r\\n\\s*\\)\\s*\\r\\n\\s*,cte<CteNextNumber> AS\\s*\\r\\n\\s*\\(\\s*\\r\\n\\s*SELECT DISTINCT .*T1, Sid1, IsMatch, .* AS IsPartial\\s*\\r\\n\\s*FROM cte<CteNumber>\\s*\\r\\n\\s*\\))"; |
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.
Are we sure this is simpler than fixing the actual generator? Should this be a rewriter instead?
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 can take another look. Changing the generator looked more complicated.
For the query: Original:
After change:
|
Description
Simplifies the SQL query generated when lots of iterative include statements are added to a query.
Related issues
Addresses User Story 130995
Testing
Tested the results returned before and after including the simplification on a known data set.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)