Skip to content
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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 19, 2024
Merged

Iterative Include SQL Simplification #4699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b5d6ebf
Add simplificiation for SQL iterative includes
Oct 22, 2024
92a94d8
Finish adding include iterate simplification.
Oct 24, 2024
9cf2e2f
Add unit tests
Oct 29, 2024
225777b
Change simplification method
Nov 14, 2024
9c68393
Remove comments
Nov 14, 2024
2ec303a
Remove unused method
Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 28 additions & 58 deletions ....Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor<SearchOptions, ob
// Include:iterate may be applied on results from multiple ctes
private List<string> _includeFromCteIds;

private int _curFromCteIndex = -1;
private int _tableExpressionCounter = -1;
private SqlRootExpression _rootExpression;
private readonly SchemaInformation _schemaInfo;
Expand Down Expand Up @@ -848,7 +847,9 @@ private void HandleTableKindInclude(
.Append(")");

// Get FROM ctes
string fromCte = _cteMainSelect;
List<string> fromCte = new List<string>();
fromCte.Add(_cteMainSelect);

if (includeExpression.Iterate)
{
// Include Iterate
Expand All @@ -858,7 +859,7 @@ private void HandleTableKindInclude(
// On that case, the fromCte is _cteMainSelect
if (TryGetIncludeCtes(includeExpression.SourceResourceType, out _includeFromCteIds))
{
fromCte = _includeFromCteIds[++_curFromCteIndex];
fromCte = _includeFromCteIds;
}
}

Expand All @@ -869,7 +870,7 @@ private void HandleTableKindInclude(
{
if (TryGetIncludeCtes(includeExpression.TargetResourceType, out _includeFromCteIds))
{
fromCte = _includeFromCteIds[++_curFromCteIndex];
fromCte = _includeFromCteIds;
}
}
else if (includeExpression.ReferenceSearchParameter?.TargetResourceTypes != null)
Expand All @@ -884,7 +885,7 @@ private void HandleTableKindInclude(
}

_includeFromCteIds = _includeFromCteIds.Distinct().ToList();
fromCte = _includeFromCteIds.Count > 0 ? _includeFromCteIds[++_curFromCteIndex] : fromCte;
fromCte = _includeFromCteIds.Count > 0 ? _includeFromCteIds : fromCte;
}
}
}
Expand All @@ -895,19 +896,30 @@ private void HandleTableKindInclude(
.Append(" = ").Append(Parameters.AddParameter(VLatest.ReferenceSearchParam.ResourceTypeId, Model.GetResourceTypeId(includeExpression.SourceResourceType), true));
}

delimited.BeginDelimitedElement().Append("EXISTS (SELECT * FROM ").Append(fromCte)
.Append(" WHERE ").Append(VLatest.Resource.ResourceTypeId, table).Append(" = T1 AND ")
.Append(VLatest.Resource.ResourceSurrogateId, table).Append(" = Sid1");

if (!includeExpression.Iterate)
var scope = delimited.BeginDelimitedElement();
scope.Append("EXISTS (");
for (var index = 0; index < fromCte.Count; index++)
{
// Limit the join to the main select CTE.
// The main select will have max+1 items in the result set to account for paging, so we only want to join using the max amount.
var cte = fromCte[index];
scope.Append("SELECT * FROM ").Append(cte)
.Append(" WHERE ").Append(VLatest.Resource.ResourceTypeId, table).Append(" = T1 AND ")
.Append(VLatest.Resource.ResourceSurrogateId, table).Append(" = Sid1");

if (!includeExpression.Iterate)
{
// Limit the join to the main select CTE.
// The main select will have max+1 items in the result set to account for paging, so we only want to join using the max amount.

StringBuilder.Append(" AND Row < ").Append(Parameters.AddParameter(context.MaxItemCount + 1, true));
scope.Append(" AND Row < ").Append(Parameters.AddParameter(context.MaxItemCount + 1, true));
}

if (index < fromCte.Count - 1)
{
scope.AppendLine(" UNION ALL ");
}
}

StringBuilder.Append(")");
scope.Append(")");
}

if (includeExpression.Reversed)
Expand Down Expand Up @@ -941,30 +953,9 @@ private void HandleTableKindInclude(
}
}

// Handle Multiple Results sets to include from
if (count > 1 && _curFromCteIndex >= 0 && _curFromCteIndex < count - 1)
if (includeExpression.WildCard)
{
StringBuilder.AppendLine("),");

// If it's not the last result set, append a new IncludeLimit cte, since IncludeLimitCte was not created for the current cte
if (_curFromCteIndex < count - 1)
{
var cteToLimit = TableExpressionName(_tableExpressionCounter);
WriteIncludeLimitCte(cteToLimit, context);
}

// Generate CTE to include from the additional result sets
StringBuilder.Append(TableExpressionName(++_tableExpressionCounter)).AppendLine(" AS").AppendLine("(");
searchParamTableExpression.AcceptVisitor(this, context);
}
else
{
_curFromCteIndex = -1;

if (includeExpression.WildCard)
{
includeExpression.ReferencedTypes?.ToList().ForEach(t => AddIncludeLimitCte(t, curLimitCte));
}
includeExpression.ReferencedTypes?.ToList().ForEach(t => AddIncludeLimitCte(t, curLimitCte));
}
}

Expand Down Expand Up @@ -1141,27 +1132,6 @@ private void HandleTableKindSortWithFilter(SearchParamTableExpression searchPara
_sortVisited = true;
}

private void WriteIncludeLimitCte(string cteToLimit, SearchOptions context)
{
StringBuilder.Append(TableExpressionName(++_tableExpressionCounter)).AppendLine(" AS").AppendLine("(");

// the related cte is a reverse include, limit the number of returned items and count to
// see if we are over the threshold (to produce a warning to the client)
StringBuilder.Append("SELECT DISTINCT ");
StringBuilder.Append("TOP (").Append(Parameters.AddParameter(context.IncludeCount, true)).Append(") ");

StringBuilder.Append("T1, Sid1, IsMatch, ");
StringBuilder.Append("CASE WHEN count_big(*) over() > ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the missing count for includes get resolved?

.Append(Parameters.AddParameter(context.IncludeCount, true))
.AppendLine(" THEN 1 ELSE 0 END AS IsPartial ");

StringBuilder.Append("FROM ").AppendLine(cteToLimit);
StringBuilder.AppendLine("),");

// the 'original' include cte is not in the union, but this new layer is instead
_includeCteIds.Add(TableExpressionName(_tableExpressionCounter));
}

private SearchParameterQueryGeneratorContext GetContext(string tableAlias = null)
{
return new SearchParameterQueryGeneratorContext(StringBuilder, Parameters, Model, _schemaInfo, tableAlias);
Expand Down
2 changes: 2 additions & 0 deletions src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ private async Task<SearchResult> SearchImpl(SqlSearchOptions sqlSearchOptions, C
await CreateStats(expression, cancellationToken);

SearchResult searchResult = null;

await _sqlRetryService.ExecuteSql(
async (connection, cancellationToken, sqlException) =>
{
Expand Down Expand Up @@ -571,6 +572,7 @@ await _sqlRetryService.ExecuteSql(
_logger,
cancellationToken,
true); // this enables reads from replicas

return searchResult;
}

Expand Down
6 changes: 4 additions & 2 deletions test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/IncludeSearchTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,7 @@ await SearchAndValidateBundleAsync(
public async Task GivenAnIncludeIterateSearchExpressionWithMultitypeArrayReference_WhenSearched_TheIterativeResultsShouldBeAddedToTheBundle()
{
// Non-recursive iteration - Reference array of multiple target types: CareTeam:participant of type Patient, Practitioner, Organization, etc.
string query = $"_include=CareTeam:participant:Patient&_include:iterate=Patient:general-practitioner&_tag={Fixture.Tag}";
string query = $"_include=CareTeam:participant&_include:iterate=Patient:general-practitioner&_tag={Fixture.Tag}";

await SearchAndValidateBundleAsync(
ResourceType.CareTeam,
Expand All @@ -682,7 +682,9 @@ await SearchAndValidateBundleAsync(
Fixture.TrumanPatient,
Fixture.AndersonPractitioner,
Fixture.SanchezPractitioner,
Fixture.TaylorPractitioner);
Fixture.TaylorPractitioner,
Fixture.Organization,
Fixture.Practitioner);
}

[Fact]
Expand Down