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

Jump to bottom

Fixing include performance #3629

Merged
merged 9 commits into from
Dec 13, 2023
Merged

Fixing include performance #3629

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2cbf250
Fixing include performance
SergeyGaluzo Dec 12, 2023
f884f86
typo
SergeyGaluzo Dec 12, 2023
f0fbe06
corrected check for need of sort value
SergeyGaluzo Dec 12, 2023
4504837
AS
SergeyGaluzo Dec 13, 2023
1433d64
join shit
SergeyGaluzo Dec 13, 2023
c1e87b8
Addung test with combined text sort and include
SergeyGaluzo Dec 13, 2023
33957f6
Remove extra lines
SergeyGaluzo Dec 13, 2023
74138d3
Extra test
SergeyGaluzo Dec 13, 2023
a6aecca
simplified test
SergeyGaluzo Dec 13, 2023
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
6 changes: 4 additions & 2 deletions ...ver/Features/Search/Expressions/Visitors/QueryGenerators/SearchParameterQueryGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ public override SearchParameterQueryGeneratorContext VisitSortParameter(SortExpr
context.StringBuilder
.Append(searchParamIdColumn, context.TableAlias)
.Append(" = ")
.AppendLine(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName);
.Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName)
.Append(" ");

return context;
}
Expand All @@ -76,7 +77,8 @@ public override SearchParameterQueryGeneratorContext VisitMissingSearchParameter
context.StringBuilder
.Append(searchParamIdColumn, context.TableAlias)
.Append(" = ")
.AppendLine(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName);
.Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName)
.Append(" ");

return context;
}
Expand Down
86 changes: 63 additions & 23 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 @@ -107,7 +107,24 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions

// Union expressions must be executed first than all other expressions. The overral idea is that Union All expressions will
// filter the highest group of records, and the following expressions will be executed on top of this group of records.
StringBuilder.Append("WITH ");
// If include, split SQL into 2 parts: 1st filter and preserve data in filtered data table variable, and 2nd - use persisted data
StringBuilder.Append("DECLARE @FilteredData AS TABLE (T1 smallint, Sid1 bigint, IsMatch bit, IsPartial bit, Row int");
var isSortValueNeeded = IsSortValueNeeded(context);
if (isSortValueNeeded)
{
var sortContext = GetSortRelatedDetails(context);
var dbType = sortContext.SortColumnName.Metadata.SqlDbType;
var typeStr = dbType.ToString().ToLowerInvariant();
StringBuilder.Append($", SortValue {typeStr}");
if (dbType != System.Data.SqlDbType.DateTime2 && dbType != System.Data.SqlDbType.DateTime) // we support only date time and short string
{
StringBuilder.Append($"({sortContext.SortColumnName.Metadata.MaxLength})");
}
}

StringBuilder.AppendLine(")");
StringBuilder.AppendLine(";WITH");
var visitedInclude = false;
StringBuilder.AppendDelimited($"{Environment.NewLine},", expression.SearchParamTableExpressions.SortExpressionsByQueryLogic(), (sb, tableExpression) =>
{
if (tableExpression.SplitExpressions(out UnionExpression unionExpression, out SearchParamTableExpression allOtherRemainingExpressions))
Expand All @@ -122,25 +139,26 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions
}
else
{
// Look for include kind. Before going to include itself, add filtered data persistence.
if (!visitedInclude && tableExpression.Kind == SearchParamTableExpressionKind.Include)
{
sb.Remove(sb.Length - 1, 1); // remove last comma
AddHash(); // hash is required in upper SQL
sb.AppendLine($"INSERT INTO @FilteredData SELECT T1, Sid1, IsMatch, IsPartial, Row{(isSortValueNeeded ? ", SortValue " : " ")}FROM cte{_tableExpressionCounter}");
AddOptionClause();
sb.AppendLine($";WITH cte{_tableExpressionCounter} AS (SELECT * FROM @FilteredData)");
sb.Append(","); // add comma back
visitedInclude = true;
}

AppendNewTableExpression(sb, tableExpression, ++_tableExpressionCounter, context);
}
});

StringBuilder.AppendLine();
}

if (Parameters.HasParametersToHash) // hash cannot be last comment as it will not be stored in query store
{
// Add a hash of (most of the) parameter values as a comment.
// We do this to avoid re-using query plans unless two queries have
// the same parameter values. We currently exclude from the hash parameters
// that are related to TOP clauses or continuation tokens.
// We can exclude more in the future.

StringBuilder.Append("/* HASH ");
Parameters.AppendHash(StringBuilder);
StringBuilder.AppendLine(" */");
}
AddHash();

string resourceTableAlias = "r";
bool selectingFromResourceTable;
Expand Down Expand Up @@ -222,7 +240,7 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions

if (expression.SearchParamTableExpressions.Count > 0)
{
StringBuilder.Append(" JOIN ").Append(TableExpressionName(_tableExpressionCounter));
StringBuilder.Append(_joinShift).Append("JOIN ").Append(TableExpressionName(_tableExpressionCounter));
StringBuilder.Append(" ON ")
.Append(VLatest.Resource.ResourceTypeId, resourceTableAlias).Append(" = ").Append(TableExpressionName(_tableExpressionCounter)).Append(".T1 AND ")
.Append(VLatest.Resource.ResourceSurrogateId, resourceTableAlias).Append(" = ").Append(TableExpressionName(_tableExpressionCounter)).AppendLine(".Sid1");
Expand Down Expand Up @@ -283,13 +301,7 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions
.Append(VLatest.Resource.ResourceSurrogateId, resourceTableAlias).AppendLine(" ASC ");
}

// if we have a complex query more than one SearchParemter, one of the parameters is "identifier", and we have an include
// then we will tell SQL to ignore the parameter values and base the query plan one the
// statistics only. We have seen SQL make poor choices in this instance, so we are making a special case here
if (AddOptimizeForUnknownClause())
{
StringBuilder.AppendLine(" OPTION (OPTIMIZE FOR UNKNOWN)"); // TODO: Remove when TokemSearchParamHighCard is used
}
AddOptionClause();
}
}
else
Expand All @@ -301,6 +313,34 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions
return null;
}

// TODO: Remove when code starts using TokenSearchParamHighCard table
private void AddOptionClause()
{
// if we have a complex query more than one SearchParemter, one of the parameters is "identifier", and we have an include
// then we will tell SQL to ignore the parameter values and base the query plan one the
// statistics only. We have seen SQL make poor choices in this instance, so we are making a special case here
if (AddOptimizeForUnknownClause())
{
StringBuilder.AppendLine("OPTION (OPTIMIZE FOR UNKNOWN)");
}
}

private void AddHash()
{
if (Parameters.HasParametersToHash) // hash cannot be last comment as it will not be stored in query store
{
// Add a hash of (most of the) parameter values as a comment.
// We do this to avoid re-using query plans unless two queries have
// the same parameter values. We currently exclude from the hash parameters
// that are related to TOP clauses or continuation tokens.
// We can exclude more in the future.

StringBuilder.Append("/* HASH ");
Parameters.AppendHash(StringBuilder);
StringBuilder.AppendLine(" */");
}
}

private static string TableExpressionName(int id) => "cte" + id;

private bool IsInSortMode(SearchOptions context) => context.Sort != null && context.Sort.Count > 0 && _sortVisited;
Expand Down Expand Up @@ -1016,7 +1056,7 @@ private void HandleTableKindSort(SearchParamTableExpression searchParamTableExpr
StringBuilder.Append("SELECT ")
.Append(VLatest.Resource.ResourceTypeId, null).Append(" AS T1, ")
.Append(VLatest.Resource.ResourceSurrogateId, null).Append(" AS Sid1, ")
.Append(sortContext.SortColumnName, null).AppendLine(" as SortValue")
.Append(sortContext.SortColumnName, null).AppendLine(" AS SortValue")
.Append("FROM ").AppendLine(searchParamTableExpression.QueryGenerator.Table);

if (CheckAppendWithJoin())
Expand Down Expand Up @@ -1065,7 +1105,7 @@ private void HandleTableKindSortWithFilter(SearchParamTableExpression searchPara
StringBuilder.Append("SELECT ")
.Append(VLatest.Resource.ResourceTypeId, null).Append(" AS T1, ")
.Append(VLatest.Resource.ResourceSurrogateId, null).Append(" AS Sid1, ")
.Append(sortContext.SortColumnName, null).AppendLine(" as SortValue")
.Append(sortContext.SortColumnName, null).AppendLine(" AS SortValue")
.Append("FROM ").AppendLine(searchParamTableExpression.QueryGenerator.Table);

if (CheckAppendWithJoin())
Expand Down
27 changes: 13 additions & 14 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 @@ -837,8 +837,7 @@ private void LogSqlCommand(SqlCommand sqlCommand)
.Append(p.Value is string ? (p.Size <= 0 ? "(max)" : $"({p.Size})") : p.Value is decimal ? $"({p.Precision},{p.Scale})" : null)
.Append(" = ")
.Append(p.SqlDbType == SqlDbType.NChar || p.SqlDbType == SqlDbType.NText || p.SqlDbType == SqlDbType.NVarChar ? "N" : null)
.Append(p.Value is string || p.Value is DateTime ? $"'{p.Value:O}'" : (p.Value == null ? "NULL" : p.Value.ToString()))
.AppendLine(";");
.AppendLine(p.Value is string || p.Value is DateTime ? $"'{p.Value:O}'" : (p.Value == null ? "NULL" : p.Value.ToString()));
}

sb.AppendLine();
Expand Down Expand Up @@ -950,17 +949,17 @@ private async Task<SearchResult> SearchForReindexSurrogateIdsBySearchParamHashAs
sqlCommand.Parameters.AddWithValue("@p2", tmpStartResourceSurrogateId);
sqlCommand.Parameters.AddWithValue("@p3", rowCount);
sqlCommand.CommandText = @"
SELECT ISNULL(MIN(ResourceSurrogateId), 0), ISNULL(MAX(ResourceSurrogateId), 0), COUNT(*)
FROM (SELECT TOP (@p3) ResourceSurrogateId
FROM dbo.Resource
WHERE ResourceTypeId = @p1
AND IsHistory = 0
AND IsDeleted = 0
AND ResourceSurrogateId > @p2
AND (SearchParamHash != @p0 OR SearchParamHash IS NULL)
ORDER BY
ResourceSurrogateId
) A";
SELECT isnull(min(ResourceSurrogateId), 0), isnull(max(ResourceSurrogateId), 0), count(*)
FROM (SELECT TOP (@p3) ResourceSurrogateId
FROM dbo.Resource
WHERE ResourceTypeId = @p1
AND IsHistory = 0
AND IsDeleted = 0
AND ResourceSurrogateId > @p2
AND (SearchParamHash != @p0 OR SearchParamHash IS NULL)
ORDER BY
ResourceSurrogateId
) A";
LogSqlCommand(sqlCommand);

IReadOnlyList<(long StartResourceSurrogateId, long EndResourceSurrogateId, int Count)> results = await sqlCommand.ExecuteReaderAsync(_sqlRetryService, ReaderGetSurrogateIdsAndCountForResourceType, _logger, cancellationToken);
Expand Down Expand Up @@ -1022,7 +1021,7 @@ private async Task<SearchResult> SearchForReindexSurrogateIdsWithoutSearchParamH
using var sqlCommand = new SqlCommand();
sqlCommand.CommandTimeout = Math.Max((int)_sqlServerDataStoreConfiguration.CommandTimeout.TotalSeconds, 180);
sqlCommand.Parameters.AddWithValue("@p0", resourceTypeId);
sqlCommand.CommandText = "SELECT ISNULL(MIN(ResourceSurrogateId), 0), ISNULL(MAX(ResourceSurrogateId), 0), COUNT(ResourceSurrogateId) FROM dbo.Resource WHERE ResourceTypeId = @p0 AND IsHistory = 0 AND IsDeleted = 0";
sqlCommand.CommandText = "SELECT isnull(min(ResourceSurrogateId), 0), isnull(max(ResourceSurrogateId), 0), count(*) FROM dbo.Resource WHERE ResourceTypeId = @p0 AND IsHistory = 0 AND IsDeleted = 0";
LogSqlCommand(sqlCommand);

IReadOnlyList<(long StartResourceSurrogateId, long EndResourceSurrogateId, int Count)> results = await sqlCommand.ExecuteReaderAsync(_sqlRetryService, ReaderGetSurrogateIdsAndCountForResourceType, _logger, cancellationToken);
Expand Down
17 changes: 17 additions & 0 deletions test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/SortTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,23 @@ public async Task GivenPatients_WhenSearchedWithFamilySortParams_ThenPatientsAre
SortTestsAssert.AssertPatientFamilyNamesAreEqualInRange(0, patients.OrderBy(x => x.Name.Min(n => n.Family)).ToList(), returnedResults);
}

[Fact]
[Trait(Traits.Priority, Priority.One)]
public async Task GivenPatients_WhenSearchedWithTextSortAndInclude_ThenPatientsAreReturned()
{
var tag = Guid.NewGuid().ToString();
var patients = await CreatePatients(tag);
Assert.True(patients.Count() > 3);

// let's make sure that nothing is broken without sort first. This also tests pagination.
var results = await GetResultsFromAllPagesAsync($"Patient?_count=3&_include=Observation:performer");
SergeyGaluzo marked this conversation as resolved.
Show resolved Hide resolved
Assert.True(results.Count() >= patients.Count());

// main check
results = await GetResultsFromAllPagesAsync($"Patient?_tag={tag}&_sort=family&_include=Observation:performer");
SortTestsAssert.AssertNumberOfResources(patients, results);
}

[Fact]
[Trait(Traits.Priority, Priority.One)]
public async Task GivenPatients_WhenSearchedWithFamilySortParamsWithHyphen_ThenPatientsAreReturnedInTheDescendingOrder()
Expand Down