Skip to content

Commit

Permalink
Removing search param and resource type ids from sql parameters (#3656)
Browse files Browse the repository at this point in the history 
* Removing search param and resource type ids from sql parameters

* Comment

* OPTION (RECOMPILE)

* Add hashed parameter list

* comments
  • Loading branch information
@SergeyGaluzo
SergeyGaluzo authored Jan 6, 2024
1 parent 1b548e6 commit b07b02b
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 30 deletions.
26 changes: 10 additions & 16 deletions ...ver/Features/Search/Expressions/Visitors/QueryGenerators/SearchParameterQueryGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public override SearchParameterQueryGeneratorContext VisitSearchParameter(Search
context.StringBuilder
.Append(searchParamIdColumn, context.TableAlias)
.Append(" = ")
.AppendLine(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName)
.Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true)).AppendLine()
.Append("AND ");

return expression.Expression.AcceptVisitor(this, context);
Expand All @@ -53,7 +53,7 @@ public override SearchParameterQueryGeneratorContext VisitSortParameter(SortExpr
context.StringBuilder
.Append(searchParamIdColumn, context.TableAlias)
.Append(" = ")
.Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true).ParameterName)
.Append(context.Parameters.AddParameter(searchParamIdColumn, searchParamId, true))
.Append(" ");

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

return context;
Expand Down Expand Up @@ -187,7 +187,7 @@ protected static SearchParameterQueryGeneratorContext VisitSimpleBinary(BinaryOp
throw new ArgumentOutOfRangeException(binaryOperator.ToString());
}

context.StringBuilder.Append(context.Parameters.AddParameter(column, value, includeInParameterHash).ParameterName);
context.StringBuilder.Append(context.Parameters.AddParameter(column, value, includeInParameterHash));

return context;
}
Expand All @@ -204,17 +204,14 @@ protected static SearchParameterQueryGeneratorContext VisitSimpleString(StringEx
{
case StringOperator.Contains:
needsEscaping = TryEscapeValueForLike(ref value);
SqlParameter containsParameter = context.Parameters.AddParameter(column, $"%{value}%", true);
context.StringBuilder.Append(" LIKE ").Append(containsParameter.ParameterName);
context.StringBuilder.Append(" LIKE ").Append(context.Parameters.AddParameter(column, $"%{value}%", true));
break;
case StringOperator.EndsWith:
needsEscaping = TryEscapeValueForLike(ref value);
SqlParameter endWithParameter = context.Parameters.AddParameter(column, $"%{value}", true);
context.StringBuilder.Append(" LIKE ").Append(endWithParameter.ParameterName);
context.StringBuilder.Append(" LIKE ").Append(context.Parameters.AddParameter(column, $"%{value}", true));
break;
case StringOperator.Equals:
SqlParameter equalsParameter = context.Parameters.AddParameter(column, value, true);
context.StringBuilder.Append(" = ").Append(equalsParameter.ParameterName);
context.StringBuilder.Append(" = ").Append(context.Parameters.AddParameter(column, value, true));
break;
case StringOperator.NotContains:
context.StringBuilder.Append(" NOT ");
Expand All @@ -227,13 +224,11 @@ protected static SearchParameterQueryGeneratorContext VisitSimpleString(StringEx
goto case StringOperator.StartsWith;
case StringOperator.StartsWith:
needsEscaping = TryEscapeValueForLike(ref value);
SqlParameter startsWithParameter = context.Parameters.AddParameter(column, $"{value}%", true);
context.StringBuilder.Append(" LIKE ").Append(startsWithParameter.ParameterName);
context.StringBuilder.Append(" LIKE ").Append(context.Parameters.AddParameter(column, $"{value}%", true));
break;
case StringOperator.LeftSideStartsWith:
needsEscaping = TryEscapeValueForLike(ref value);
SqlParameter leftParameter = context.Parameters.AddParameter(column, $"{value}", true);
context.StringBuilder.Append(leftParameter.ParameterName).Append(" LIKE ");
context.StringBuilder.Append(context.Parameters.AddParameter(column, $"{value}", true)).Append(" LIKE ");
AppendColumnName(context, column, expression);
context.StringBuilder.Append("+'%'");
break;
Expand All @@ -259,8 +254,7 @@ protected static SearchParameterQueryGeneratorContext VisitSimpleString(StringEx

context.StringBuilder.Append(" AND ");
AppendColumnName(context, column, expression);
SqlParameter equalsParameter = context.Parameters.AddParameter(column, value, true);
context.StringBuilder.Append(" = ").Append(equalsParameter.ParameterName);
context.StringBuilder.Append(" = ").Append(context.Parameters.AddParameter(column, value, true));
}

context.StringBuilder.Append(" COLLATE ").Append(expression.IgnoreCase ? DefaultCaseInsensitiveCollation : DefaultCaseSensitiveCollation);
Expand Down
1 change: 1 addition & 0 deletions ....Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ private void AddHash()

StringBuilder.Append("/* HASH ");
Parameters.AppendHash(StringBuilder);
Parameters.AppendHashedParameterNames(StringBuilder);
StringBuilder.AppendLine(" */");
}
}
Expand Down
34 changes: 28 additions & 6 deletions src/Microsoft.Health.Fhir.SqlServer/Features/Search/HashingSqlQueryParameterManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Security.Cryptography;
using EnsureThat;
using Microsoft.Data.SqlClient;
using Microsoft.Health.Fhir.SqlServer.Features.Schema.Model;
using Microsoft.Health.SqlServer;
using Microsoft.Health.SqlServer.Features.Schema.Model;
using Microsoft.Health.SqlServer.Features.Storage;
Expand All @@ -35,7 +36,7 @@ public HashingSqlQueryParameterManager(SqlQueryParameterManager inner)
public bool HasParametersToHash => _setToHash.Count > 0;

/// <summary>
/// Add a parameter to the SQL command.
/// Add a parameter to the SQL command if it is not ResourceTypeId and not SearchParamId.
/// </summary>
/// <typeparam name="T">The CLR column type</typeparam>
/// <param name="column">The table column the parameter is bound to.</param>
Expand All @@ -44,24 +45,31 @@ public HashingSqlQueryParameterManager(SqlQueryParameterManager inner)
/// Whether this parameter should be included in the hash of the overall parameters.
/// If true, this parameter will prevent other identical queries with a different value for this parameter from re-using the query plan.
/// </param>
/// <returns>The SQL parameter.</returns>
public SqlParameter AddParameter<T>(Column<T> column, T value, bool includeInHash)
/// <returns>SQL parameter or input value depending on whether input was added to the list of parameters.</returns>
public object AddParameter<T>(Column<T> column, T value, bool includeInHash)
{
return AddParameter((Column)column, value, includeInHash);
}

/// <summary>
/// Add a parameter to the SQL command.
/// Add a parameter to the SQL command if it is not ResourceTypeId and not SearchParamId.
/// </summary>
/// <param name="column">The table column the parameter is bound to.</param>
/// <param name="value">The parameter value</param>
/// <param name="includeInHash">
/// Whether this parameter should be included in the hash of the overall parameters.
/// If true, this parameter will prevent other identical queries with a different value for this parameter from re-using the query plan.
/// </param>
/// <returns>The SQL parameter.</returns>
public SqlParameter AddParameter(Column column, object value, bool includeInHash)
/// <returns>SQL parameter or input value depending on whether input was added to the list of parameters.</returns>
public object AddParameter(Column column, object value, bool includeInHash)
{
if (column.Metadata.Name == VLatest.Resource.ResourceTypeId.Metadata.Name // logic uses "ResourceTypeId" string value. Resource table is chosen arbitrarily.
|| column.Metadata.Name == VLatest.ReferenceSearchParam.ReferenceResourceTypeId.Metadata.Name
|| column.Metadata.Name == VLatest.TokenSearchParam.SearchParamId.Metadata.Name) // logic uses "SearchParamId" string value. We don't have cross table column sharing concept yet, so to avoid hardcoding TokenSearchParam is arbitrarily chosen.
{
return value;
}

SqlParameter parameter = _inner.AddParameter(column, value);
if (includeInHash)
{
Expand Down Expand Up @@ -182,6 +190,20 @@ public void AppendHash(IndentedStringBuilder stringBuilder)
stringBuilder.Append(hashChars[..hashCharsLength]);
}

/// <summary>
/// Appends comma delimited list of names of hashed parameters
/// </summary>
/// <param name="stringBuilder">A string builder to append the list to.</param>
public void AppendHashedParameterNames(IndentedStringBuilder stringBuilder)
{
var first = true;
foreach (var param in _setToHash)
{
stringBuilder.Append($"{(first ? " params=" : ",")}{param}");
first = false;
}
}

private static void WriteAndAdvance<T>(Span<byte> buffer, ref int currentIndex, ref IncrementalHash incrementalHash, T element)
where T : struct
{
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,7 @@ private void LogSqlCommand(SqlCommand sqlCommand)
sb.AppendLine();
}

sb.AppendLine("OPTION (RECOMPILE)"); // enables query compilation with provided parameter values in debugging
sb.AppendLine($"-- execution timeout = {sqlCommandWrapper.CommandTimeout} sec.");
_sqlRetryService.TryLogEvent("Search", "Start", sb.ToString(), null, CancellationToken.None);
_logger.LogInformation("{SqlQuery}", sb.ToString());
Expand Down
12 changes: 4 additions & 8 deletions test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlCustomQueryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,15 +129,11 @@ private void AddSproc(string hash)
{
_fixture.SqlHelper.ExecuteSqlCmd(@$"
CREATE OR ALTER PROCEDURE [dbo].[CustomQuery_{hash}]
@p0 smallint
@p0 datetime2
,@p1 datetime2
,@p2 smallint
,@p3 datetime2
,@p4 smallint
,@p5 nvarchar(256)
,@p6 smallint
,@p7 nvarchar(256)
,@p8 int
,@p2 nvarchar(256)
,@p3 nvarchar(256)
,@p4 int
AS
set nocount on
SELECT DISTINCT r.ResourceTypeId, r.ResourceId, r.Version, r.IsDeleted, r.ResourceSurrogateId, r.RequestMethod, CAST(1 AS bit) AS IsMatch, CAST(0 AS bit) AS IsPartial, r.IsRawResourceMetaSet, r.SearchParamHash, r.RawResource
Expand Down

0 comments on commit b07b02b

Please sign in to comment.