From 25ba312289eb01cce923a67d93708eb66c45eb71 Mon Sep 17 00:00:00 2001 From: Jared Erwin Date: Thu, 26 Oct 2023 20:13:45 -0700 Subject: [PATCH 1/2] Change query generator to use INNER JOIN instead of EXISTS in more cases --- docs/rest/complexQueries.http | 161 ++++++++++++++++++ .../QueryGenerators/SqlQueryGenerator.cs | 17 +- 2 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 docs/rest/complexQueries.http diff --git a/docs/rest/complexQueries.http b/docs/rest/complexQueries.http new file mode 100644 index 0000000000..cae513a262 --- /dev/null +++ b/docs/rest/complexQueries.http @@ -0,0 +1,161 @@ +@hostname = localhost:44348 + +### Test rest client +https://{{hostname}}/metadata + +### Get the globalAdminServicePrincipal to verify scopes not enforced, and to be able to POST test data +# @name bearer +POST https://{{hostname}}/connect/token +content-type: application/x-www-form-urlencoded + +grant_type=client_credentials +&client_id=globalAdminServicePrincipal +&client_secret=globalAdminServicePrincipal +&scope=fhir-api + +### FROM ICM: https://portal.microsofticm.com/imp/v3/incidents/incident/352320409/summary +#GET https://{{hostname}}/Condition?patient=37e9d66c-9e3a-4344-b577-a32855916962&addressed_status=active&clinical-status=active&category=main&category=important&_count=100&extension-care-goals=6db9b2c6-45ce-4e5c-994e-02cc760dbdc5&recorder=f4845d77-d866-48c8-b548-2e867c9fb32f¬e-text=deat&_include=Condition:recorder&_include=Condition:extension-care-goals&_revinclude=Goal:addresses-conditions&onset-date=ge2022-11-01T00:00:00.000Z +GET https://{{hostname}}/Condition?patient=37e9d66c-9e3a-4344-b577-a32855916962&verification_status=active&clinical-status=active&category=main&category=important&_count=100&asserter=6db9b2c6-45ce-4e5c-994e-02cc760dbdc5&recorder=f4845d77-d866-48c8-b548-2e867c9fb32f¬e-text=deat&_include=Condition:recorder&_include=Condition:asserter&_revinclude=Goal:addresses-conditions&onset-date=ge2022-11-01T00:00:00.000Z +content-type: application/json +Authorization: Bearer {{bearer.response.body.access_token}} + +### +GET https://{{hostname}}/SearchParameter +content-type: application/json +Authorization: Bearer {{bearer.response.body.access_token}} + +### +DELETE https://{{hostname}}/SearchParameter/Resource-id-string +content-type: application/json +Authorization: Bearer {{bearer.response.body.access_token}} + +### POST a new search parameter +POST https://{{hostname}}/SearchParameter +content-type: application/json +Authorization: Bearer {{bearer.response.body.access_token}} + +{ + "resourceType" : "SearchParameter", + "id" : "Condition-recorder", + "extension" : [{ + "url" : "http://hl7.org/fhir/StructureDefinition/structuredefinition-standards-status", + "valueCode" : "trial-use" + }], + "url" : "http://hl7.org/fhir/SearchParameter/Condition-recorder", + "version" : "4.0.1", + "name" : "recorder", + "status" : "draft", + "experimental" : false, + "date" : "2019-11-01T09:29:23+11:00", + "publisher" : "Health Level Seven International (Patient Care)", + "contact" : [{ + "telecom" : [{ + "system" : "url", + "value" : "http://hl7.org/fhir" + }] + }, + { + "telecom" : [{ + "system" : "url", + "value" : "http://www.hl7.org/Special/committees/patientcare/index.cfm" + }] + }], + "description" : "Person who records this condition", + "code" : "recorder", + "base" : ["Condition"], + "type" : "reference", + "expression" : "Condition.recorder", + "xpath" : "f:Condition/f:recorder", + "xpathUsage" : "normal", + "target" : ["Practitioner", + "Patient", + "PractitionerRole", + "RelatedPerson"] + } + + +### POST a new search parameter +GET https://{{hostname}}/SearchParameter +content-type: application/json +Authorization: Bearer {{bearer.response.body.access_token}} + +### POST a new search parameter +POST https://{{hostname}}/SearchParameter +content-type: application/json +Authorization: Bearer {{bearer.response.body.access_token}} + + { + "resourceType" : "SearchParameter", + "id" : "Goal-addressess-condition", + "extension" : [{ + "url" : "http://hl7.org/fhir/StructureDefinition/structuredefinition-standards-status", + "valueCode" : "trial-use" + }], + "url" : "http://hl7.org/fhir/SearchParameter/Goal-addresses-conditions", + "version" : "4.0.1", + "name" : "addresses-conditions", + "status" : "draft", + "experimental" : false, + "date" : "2019-11-01T09:29:23+11:00", + "publisher" : "Health Level Seven International (Patient Care)", + "contact" : [{ + "telecom" : [{ + "system" : "url", + "value" : "http://hl7.org/fhir" + }] + }, + { + "telecom" : [{ + "system" : "url", + "value" : "http://www.hl7.org/Special/committees/patientcare/index.cfm" + }] + }], + "description" : "Health issues this plan addresses", + "code" : "addresses-conditions", + "base" : ["Goal"], + "type" : "reference", + "expression" : "Goal.addresses", + "xpath" : "f:Goal/f:addresses", + "xpathUsage" : "normal", + "target" : ["Condition"] + } + +### POST a new search parameter +PUT https://{{hostname}}/SearchParameter/2d3cf49d-1e8a-48ab-9129-3f3bc7630ee3 +content-type: application/json +Authorization: Bearer {{bearer.response.body.access_token}} + + { + "resourceType" : "SearchParameter", + "id" : "2d3cf49d-1e8a-48ab-9129-3f3bc7630ee3", + "extension" : [{ + "url" : "http://hl7.org/fhir/StructureDefinition/structuredefinition-standards-status", + "valueCode" : "trial-use" + }], + "url" : "http://hl7.org/fhir/SearchParameter/Condition-notes", + "version" : "4.0.1", + "name" : "note-text", + "status" : "draft", + "experimental" : false, + "date" : "2019-11-01T09:29:23+11:00", + "publisher" : "Health Level Seven International (FHIR Infrastructure)", + "contact" : [{ + "telecom" : [{ + "system" : "url", + "value" : "http://hl7.org/fhir" + }] + }, + { + "telecom" : [{ + "system" : "url", + "value" : "http://www.hl7.org/Special/committees/fiwg/index.cfm" + }] + }], + "description" : "The annotation - text content (as markdown)", + "code" : "note-text", + "base" : ["Condition"], + "type" : "string", + "expression" : "Condition.note.text", + "xpath" : "f:Condition/f:note/f:text", + "xpathUsage" : "normal" + } \ No newline at end of file diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index 415d85a8e2..c617fad63f 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -52,7 +52,7 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor 0 or if in sort mode or if ReferenceSearchParam, the intersection is already handled in a JOIN + // if chainLevel > 0 or if in sort mode or if we need to simplify the query, the intersection is already handled in a JOIN AppendIntersectionWithPredecessor(delimited, searchParamTableExpression); } @@ -1217,13 +1217,12 @@ private void AppendNewTableExpression(IndentedStringBuilder sb, SearchParamTable private bool CheckAppendWithJoin(string tableName) { - // if this is an inner join on the Reference Search Param table, and either: + // if either: // 1. the number of table expressions is greater than the limit indicating a complex query // 2. the previous query generator failed to generate a query // then we will use the EXISTS clause instead of the inner join - if (tableName == VLatest.ReferenceSearchParam.TableName && - (_rootExpression.SearchParamTableExpressions.Count > maxTableExpressionCountLimitForExists || - previousSqlQueryGeneratorFailure)) + if (_rootExpression.SearchParamTableExpressions.Count > maxTableExpressionCountLimitForExists || + previousSqlQueryGeneratorFailure) { return true; } From e9a8e82de67a23cdae7d11f236f33fadc29e6c59 Mon Sep 17 00:00:00 2001 From: Jared Erwin Date: Mon, 30 Oct 2023 09:26:44 -0700 Subject: [PATCH 2/2] Remove unnecessary method parameter, code review fixes --- docs/rest/complexQueries.http | 11 ++++++++--- .../QueryGenerators/SqlQueryGenerator.cs | 18 +++++++++--------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/docs/rest/complexQueries.http b/docs/rest/complexQueries.http index cae513a262..20e8d1077a 100644 --- a/docs/rest/complexQueries.http +++ b/docs/rest/complexQueries.http @@ -1,3 +1,8 @@ +# This file attempts to create a complex query seen by a customer where +# the SQL query plan generator failed to create a query plan +# It adds two new custom search parameters to the database +# which support the query on line 23, Also line 22 contains the original query from the customer + @hostname = localhost:44348 ### Test rest client @@ -15,7 +20,7 @@ grant_type=client_credentials ### FROM ICM: https://portal.microsofticm.com/imp/v3/incidents/incident/352320409/summary #GET https://{{hostname}}/Condition?patient=37e9d66c-9e3a-4344-b577-a32855916962&addressed_status=active&clinical-status=active&category=main&category=important&_count=100&extension-care-goals=6db9b2c6-45ce-4e5c-994e-02cc760dbdc5&recorder=f4845d77-d866-48c8-b548-2e867c9fb32f¬e-text=deat&_include=Condition:recorder&_include=Condition:extension-care-goals&_revinclude=Goal:addresses-conditions&onset-date=ge2022-11-01T00:00:00.000Z -GET https://{{hostname}}/Condition?patient=37e9d66c-9e3a-4344-b577-a32855916962&verification_status=active&clinical-status=active&category=main&category=important&_count=100&asserter=6db9b2c6-45ce-4e5c-994e-02cc760dbdc5&recorder=f4845d77-d866-48c8-b548-2e867c9fb32f¬e-text=deat&_include=Condition:recorder&_include=Condition:asserter&_revinclude=Goal:addresses-conditions&onset-date=ge2022-11-01T00:00:00.000Z +GET https://{{hostname}}/Condition?patient=37e9d66c-9e3a-4344-b577-a32855916962&verification-status=active&clinical-status=active&category=main&category=important&_count=100&asserter=6db9b2c6-45ce-4e5c-994e-02cc760dbdc5&recorder=f4845d77-d866-48c8-b548-2e867c9fb32f¬e-text=deat&_include=Condition:recorder&_include=Condition:asserter&_revinclude=Goal:addresses-conditions&onset-date=ge2022-11-01T00:00:00.000Z content-type: application/json Authorization: Bearer {{bearer.response.body.access_token}} @@ -74,7 +79,7 @@ Authorization: Bearer {{bearer.response.body.access_token}} } -### POST a new search parameter +### GET a new search parameter GET https://{{hostname}}/SearchParameter content-type: application/json Authorization: Bearer {{bearer.response.body.access_token}} @@ -120,7 +125,7 @@ Authorization: Bearer {{bearer.response.body.access_token}} "target" : ["Condition"] } -### POST a new search parameter +### PUT a new search parameter PUT https://{{hostname}}/SearchParameter/2d3cf49d-1e8a-48ab-9129-3f3bc7630ee3 content-type: application/json Authorization: Bearer {{bearer.response.body.access_token}} diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index c617fad63f..5982d11804 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -488,7 +488,7 @@ private void HandleTableKindNormal(SearchParamTableExpression searchParamTableEx } } - if (CheckAppendWithJoin(searchParamTableExpression.QueryGenerator.Table.TableName) + if (CheckAppendWithJoin() && searchParamTableExpression.ChainLevel == 0 && !IsInSortMode(context)) { AppendIntersectionWithPredecessorUsingInnerJoin(StringBuilder, searchParamTableExpression); @@ -498,7 +498,7 @@ private void HandleTableKindNormal(SearchParamTableExpression searchParamTableEx { AppendHistoryClause(delimited); - if (searchParamTableExpression.ChainLevel == 0 && !IsInSortMode(context) && !CheckAppendWithJoin(searchParamTableExpression.QueryGenerator.Table.TableName)) + if (searchParamTableExpression.ChainLevel == 0 && !IsInSortMode(context) && !CheckAppendWithJoin()) { // if chainLevel > 0 or if in sort mode or if we need to simplify the query, the intersection is already handled in a JOIN AppendIntersectionWithPredecessor(delimited, searchParamTableExpression); @@ -715,7 +715,7 @@ private void HandleTableKindChain( } // since we are in chain table expression, we know the Table is the ReferenceSearchParam table - else if (CheckAppendWithJoin(VLatest.ReferenceSearchParam.TableName)) + else if (CheckAppendWithJoin()) { AppendIntersectionWithPredecessorUsingInnerJoin(StringBuilder, searchParamTableExpression, chainedExpression.Reversed ? referenceTargetResourceTableAlias : referenceSourceTableAlias); } @@ -738,7 +738,7 @@ private void HandleTableKindChain( .Append(string.Join(", ", chainedExpression.TargetResourceTypes.Select(x => Parameters.AddParameter(VLatest.ReferenceSearchParam.ReferenceResourceTypeId, Model.GetResourceTypeId(x), true)))) .Append(")"); - if (searchParamTableExpression.ChainLevel == 1 && !CheckAppendWithJoin(VLatest.ReferenceSearchParam.TableName)) + if (searchParamTableExpression.ChainLevel == 1 && !CheckAppendWithJoin()) { // if > 1, the intersection is handled by the JOIN AppendIntersectionWithPredecessor(delimited, searchParamTableExpression, chainedExpression.Reversed ? referenceTargetResourceTableAlias : referenceSourceTableAlias); @@ -1053,7 +1053,7 @@ private void HandleTableKindSort(SearchParamTableExpression searchParamTableExpr .Append(sortContext.SortColumnName, null).AppendLine(" as SortValue") .Append("FROM ").AppendLine(searchParamTableExpression.QueryGenerator.Table); - if (CheckAppendWithJoin(searchParamTableExpression.QueryGenerator.Table.TableName)) + if (CheckAppendWithJoin()) { AppendIntersectionWithPredecessorUsingInnerJoin(StringBuilder, searchParamTableExpression); } @@ -1080,7 +1080,7 @@ private void HandleTableKindSort(SearchParamTableExpression searchParamTableExpr StringBuilder.Append(" OR ").Append(sortContext.SortColumnName, null).Append(" ").Append(sortOperand).Append(" ").Append(Parameters.AddParameter(sortContext.SortColumnName, sortContext.SortValue, includeInHash: false)).AppendLine(")"); } - if (!CheckAppendWithJoin(searchParamTableExpression.QueryGenerator.Table.TableName)) + if (!CheckAppendWithJoin()) { AppendIntersectionWithPredecessor(delimited, searchParamTableExpression); } @@ -1102,7 +1102,7 @@ private void HandleTableKindSortWithFilter(SearchParamTableExpression searchPara .Append(sortContext.SortColumnName, null).AppendLine(" as SortValue") .Append("FROM ").AppendLine(searchParamTableExpression.QueryGenerator.Table); - if (CheckAppendWithJoin(searchParamTableExpression.QueryGenerator.Table.TableName)) + if (CheckAppendWithJoin()) { AppendIntersectionWithPredecessorUsingInnerJoin(StringBuilder, searchParamTableExpression); } @@ -1129,7 +1129,7 @@ private void HandleTableKindSortWithFilter(SearchParamTableExpression searchPara StringBuilder.Append(" OR ").Append(sortContext.SortColumnName, null).Append(" ").Append(sortOperand).Append(" ").Append(Parameters.AddParameter(sortContext.SortColumnName, sortContext.SortValue, includeInHash: false)).AppendLine(")"); } - if (!CheckAppendWithJoin(searchParamTableExpression.QueryGenerator.Table.TableName)) + if (!CheckAppendWithJoin()) { AppendIntersectionWithPredecessor(delimited, searchParamTableExpression); } @@ -1215,7 +1215,7 @@ private void AppendNewTableExpression(IndentedStringBuilder sb, SearchParamTable sb.Append(")"); } - private bool CheckAppendWithJoin(string tableName) + private bool CheckAppendWithJoin() { // if either: // 1. the number of table expressions is greater than the limit indicating a complex query