Skip to content

Commit 577a3f1

Browse files
abhishekkumamsAniruddh25seantleonard
authored
Fix Performance Issue for filtering on varchar columns in MS_SQL database (#2052)
## Why make this change? - Closes #1973 - Slow response of DAB when filtering on varchar columns in MS_SQL compared to other DBs. - Same query in mssql taking around ~1sec, compared to <50ms in other DBs for large number of rows, eg. 20 million - This was happening because the query generated by DAB when runs on Sql Server for varchar columns, it automatically assumes the filter value of type nvarchar, due to which it does implicit cast leading to slowness. - To fix this we needed to identify if the filter is for varchar column, we explitily specify it to be varchar for faster response. ## What is this change? - We are modifying the DbCommand and adding extra information that is sent to SqlClient. - We introduced a new property called `SqlDbType`, By Default for String DbType, SqlDbType was considered as `nvarchar`. now we are updating this property to the correct sql type. - This allows db to directly execute the command without type casting due to type mismatch. - Carved out the code which creates DbCommand in `QueryExecuter`, and added an override method in `MsSqlQueryExecuter`. here we set the property SqlDbType for the Parameters. - Link to Discussion which talks about this highlighting the issue : #1964 (comment) - Added a new property SqlDbType in Column class, currently we used to have only DotNet DbType which doesn't differenciate between varchar and Nvarchar. So, adding this SqlDbType gave us the freedom to modify query based on the sql db type. ## How was this tested? - Since, this is a performance issue and only a new column got introduced. The main aim here was to make sure excising functionality doesn't break. - Added few more extra tests. The performance difference is more visible when there are large number of rows. So, I added 20 million records and ran perf test. Below are the performance results with avg response time: | | varchar(before) | nvarchar(before | |---|---|---| | GraphQL | ~734ms | ~23ms | | REST | ~655ms | ~24ms | | | varchar(after) | nvarchar(after) | |---|---|---| | GraphQL | ~18ms | ~23ms | | REST | ~18ms | ~25ms | ## BEFORE CHANGE ### GRAPHQL #### VARCHAR ![image](https://github.com/Azure/data-api-builder/assets/102276754/6b8084be-1863-445a-84d3-4a81faaa5a70) ![image](https://github.com/Azure/data-api-builder/assets/102276754/9d538475-3ab5-4b3e-bd07-ae148a996332) #### NVARCHAR ![image](https://github.com/Azure/data-api-builder/assets/102276754/30ce863b-a4ff-4348-89ce-80b92d701f6e) ![image](https://github.com/Azure/data-api-builder/assets/102276754/17950827-634c-485f-b7ba-189d71991949) ### REST #### VARCHAR ![image](https://github.com/Azure/data-api-builder/assets/102276754/8db4c7dc-5a70-4b06-8e11-7a8607076926) #### NVARCHAR ![image](https://github.com/Azure/data-api-builder/assets/102276754/b5316d57-4735-4c93-8df4-210a08fa4904) ## AFTER CHANGE ### GRAPHQL #### VARCHAR ![image](https://github.com/Azure/data-api-builder/assets/102276754/95fe1f55-9a7a-4db5-852b-7a3418ce2a57) ![image](https://github.com/Azure/data-api-builder/assets/102276754/85254053-4e5a-432a-b727-89e3ced8686f) #### NVARCHAR ![image](https://github.com/Azure/data-api-builder/assets/102276754/0e741014-162a-4207-bae5-eeae26529b27) ![image](https://github.com/Azure/data-api-builder/assets/102276754/61945777-9db8-47a3-8ee6-6cc53ad82e3c) ### REST #### VARCHAR ![image](https://github.com/Azure/data-api-builder/assets/102276754/808c64d2-67c6-4c4b-9559-8c80c5431ca6) #### NVARCHAR ![image](https://github.com/Azure/data-api-builder/assets/102276754/d0217693-d2e5-4822-af1b-ac56cb668517) --------- Co-authored-by: Aniruddh Munde <anmunde@microsoft.com> Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>
1 parent aad9c17 commit 577a3f1

26 files changed

+480
-66
lines changed

src/Config/DatabasePrimitives/DatabaseObject.cs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ public class ParameterDefinition
130130
{
131131
public Type SystemType { get; set; } = null!;
132132
public DbType? DbType { get; set; }
133+
public SqlDbType? SqlDbType { get; set; }
133134
public bool HasConfigDefault { get; set; }
134135
public object? ConfigDefaultValue { get; set; }
135136
}
@@ -208,6 +209,24 @@ public bool IsAnyColumnNullable(List<string> columnsToCheck)
208209

209210
return null;
210211
}
212+
213+
/// <summary>
214+
/// Method to get the SqlDbType for:
215+
/// 1. column for table/view,
216+
/// 2. parameter for stored procedure.
217+
/// </summary>
218+
/// <param name="paramName">The parameter whose SqlDbType is to be determined.
219+
/// For table/view paramName refers to the backingColumnName if aliases are used.</param>
220+
/// <returns>SqlDbType for the parameter.</returns>
221+
public virtual SqlDbType? GetSqlDbTypeForParam(string paramName)
222+
{
223+
if (Columns.TryGetValue(paramName, out ColumnDefinition? columnDefinition))
224+
{
225+
return columnDefinition.SqlDbType;
226+
}
227+
228+
return null;
229+
}
211230
}
212231

213232
/// <summary>
@@ -235,6 +254,7 @@ public class ColumnDefinition
235254
/// </summary>
236255
public Type SystemType { get; set; } = typeof(object);
237256
public DbType? DbType { get; set; }
257+
public SqlDbType? SqlDbType { get; set; }
238258
public bool HasDefault { get; set; }
239259
public bool IsAutoGenerated { get; set; }
240260
public bool IsNullable { get; set; }

src/Core/Models/DbConnectionParam.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,11 @@ namespace Azure.DataApiBuilder.Core.Models;
1010
/// </summary>
1111
public class DbConnectionParam
1212
{
13-
public DbConnectionParam(object? value, DbType? dbType = null)
13+
public DbConnectionParam(object? value, DbType? dbType = null, SqlDbType? sqlDbType = null)
1414
{
1515
Value = value;
1616
DbType = dbType;
17+
SqlDbType = sqlDbType;
1718
}
1819

1920
/// <summary>
@@ -26,4 +27,8 @@ public DbConnectionParam(object? value, DbType? dbType = null)
2627
// identically and then implicit conversion cannot happen.
2728
// For more details refer: https://github.com/Azure/data-api-builder/pull/1442.
2829
public DbType? DbType { get; set; }
30+
31+
// This is being made nullable
32+
// because it's not populated for DB's other than MSSQL.
33+
public SqlDbType? SqlDbType { get; set; }
2934
}

src/Core/Models/GraphQLFilterParsers.cs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -242,12 +242,12 @@ public Predicate Parse(
242242
ParseScalarType(
243243
ctx,
244244
argumentSchema: filterArgumentObject.Fields[name],
245-
backingColumnName,
246-
subfields,
247-
schemaName,
248-
sourceName,
249-
sourceAlias,
250-
queryStructure.MakeDbConnectionParam)));
245+
fieldName: backingColumnName,
246+
fields: subfields,
247+
schemaName: schemaName,
248+
tableName: sourceName,
249+
tableAlias: sourceAlias,
250+
processLiterals: queryStructure.MakeDbConnectionParam)));
251251
}
252252
}
253253
}
@@ -476,7 +476,7 @@ public HttpContext GetHttpContextFromMiddlewareContext(IMiddlewareContext ctx)
476476
/// </summary>
477477
/// <param name="ctx">The GraphQL context, used to get the query variables</param>
478478
/// <param name="argumentSchema">An IInputField object which describes the schema of the scalar input argument (e.g. IntFilterInput)</param>
479-
/// <param name="name">The name of the field</param>
479+
/// <param name="fieldName">The name of the field</param>
480480
/// <param name="fields">The subfields of the scalar field</param>
481481
/// <param name="schemaName">The db schema name to which the table belongs</param>
482482
/// <param name="tableName">The name of the table underlying the *FilterInput being processed</param>
@@ -485,14 +485,14 @@ public HttpContext GetHttpContextFromMiddlewareContext(IMiddlewareContext ctx)
485485
private static Predicate ParseScalarType(
486486
IMiddlewareContext ctx,
487487
IInputField argumentSchema,
488-
string name,
488+
string fieldName,
489489
List<ObjectFieldNode> fields,
490490
string schemaName,
491491
string tableName,
492492
string tableAlias,
493493
Func<object, string?, string> processLiterals)
494494
{
495-
Column column = new(schemaName, tableName, columnName: name, tableAlias);
495+
Column column = new(schemaName, tableName, columnName: fieldName, tableAlias);
496496

497497
return FieldFilterParser.Parse(ctx, argumentSchema, column, fields, processLiterals);
498498
}

src/Core/Parsers/ODataASTVisitor.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,7 @@ private void PopulateDbTypeForProperty(BinaryOperatorNode nodeIn)
277277
string? paramName = BaseQueryStructure.GetEncodedParamName(_struct.Counter.Current() - 1);
278278
_metadataProvider.TryGetBackingColumn(_struct.EntityName, propertyNode.Property.Name, out string? backingColumnName);
279279
_struct.Parameters[paramName].DbType = _struct.GetUnderlyingSourceDefinition().Columns[backingColumnName!].DbType;
280+
_struct.Parameters[paramName].SqlDbType = _struct.GetUnderlyingSourceDefinition().Columns[backingColumnName!].SqlDbType;
280281
}
281282

282283
/// <summary>

src/Core/Resolvers/BaseQueryStructure.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,13 +107,16 @@ public BaseQueryStructure(
107107
/// Add parameter to Parameters and return the name associated with it
108108
/// </summary>
109109
/// <param name="value">Value to be assigned to parameter, which can be null for nullable columns.</param>
110-
/// <param name="paramName"> The name of the parameter - column name for table/views or parameter name for stored procedures.</param>
110+
/// <param name="paramName"> The name of the parameter - backing column name for table/views or parameter name for stored procedures.</param>
111111
public virtual string MakeDbConnectionParam(object? value, string? paramName = null)
112112
{
113113
string encodedParamName = GetEncodedParamName(Counter.Next());
114114
if (!string.IsNullOrEmpty(paramName))
115115
{
116-
Parameters.Add(encodedParamName, new(value, GetUnderlyingSourceDefinition().GetDbTypeForParam(paramName)));
116+
Parameters.Add(encodedParamName,
117+
new(value,
118+
dbType: GetUnderlyingSourceDefinition().GetDbTypeForParam(paramName),
119+
sqlDbType: GetUnderlyingSourceDefinition().GetSqlDbTypeForParam(paramName)));
117120
}
118121
else
119122
{

src/Core/Resolvers/MsSqlQueryExecutor.cs

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -305,12 +305,51 @@ public override async Task<DbResultSet> GetMultipleResultSetsIfAnyAsync(
305305
return dbResultSet;
306306
}
307307

308+
/// <inheritdoc />
309+
public override SqlCommand PrepareDbCommand(
310+
SqlConnection conn,
311+
string sqltext,
312+
IDictionary<string, DbConnectionParam> parameters,
313+
HttpContext? httpContext,
314+
string dataSourceName)
315+
{
316+
SqlCommand cmd = conn.CreateCommand();
317+
cmd.CommandType = CommandType.Text;
318+
319+
// Add query to send user data from DAB to the underlying database to enable additional security the user might have configured
320+
// at the database level.
321+
string sessionParamsQuery = GetSessionParamsQuery(httpContext, parameters, dataSourceName);
322+
323+
cmd.CommandText = sessionParamsQuery + sqltext;
324+
if (parameters is not null)
325+
{
326+
foreach (KeyValuePair<string, DbConnectionParam> parameterEntry in parameters)
327+
{
328+
SqlParameter parameter = cmd.CreateParameter();
329+
parameter.ParameterName = parameterEntry.Key;
330+
parameter.Value = parameterEntry.Value.Value ?? DBNull.Value;
331+
PopulateDbTypeForParameter(parameterEntry, parameter);
332+
cmd.Parameters.Add(parameter);
333+
}
334+
}
335+
336+
return cmd;
337+
}
338+
308339
/// <inheritdoc/>
309-
public override void PopulateDbTypeForParameter(KeyValuePair<string, DbConnectionParam> parameterEntry, DbParameter parameter)
340+
public static void PopulateDbTypeForParameter(KeyValuePair<string, DbConnectionParam> parameterEntry, SqlParameter parameter)
310341
{
311-
if (parameterEntry.Value is not null && parameterEntry.Value.DbType is not null)
342+
if (parameterEntry.Value is not null)
312343
{
313-
parameter.DbType = (DbType)parameterEntry.Value.DbType;
344+
if (parameterEntry.Value.DbType is not null)
345+
{
346+
parameter.DbType = (DbType)parameterEntry.Value.DbType;
347+
}
348+
349+
if (parameterEntry.Value.SqlDbType is not null)
350+
{
351+
parameter.SqlDbType = (SqlDbType)parameterEntry.Value.SqlDbType;
352+
}
314353
}
315354
}
316355
}

src/Core/Resolvers/QueryExecutor.cs

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -153,25 +153,7 @@ public QueryExecutor(DbExceptionParser dbExceptionParser,
153153
List<string>? args = null)
154154
{
155155
await conn.OpenAsync();
156-
DbCommand cmd = conn.CreateCommand();
157-
cmd.CommandType = CommandType.Text;
158-
159-
// Add query to send user data from DAB to the underlying database to enable additional security the user might have configured
160-
// at the database level.
161-
string sessionParamsQuery = GetSessionParamsQuery(httpContext, parameters, dataSourceName);
162-
163-
cmd.CommandText = sessionParamsQuery + sqltext;
164-
if (parameters is not null)
165-
{
166-
foreach (KeyValuePair<string, DbConnectionParam> parameterEntry in parameters)
167-
{
168-
DbParameter parameter = cmd.CreateParameter();
169-
parameter.ParameterName = parameterEntry.Key;
170-
parameter.Value = parameterEntry.Value.Value ?? DBNull.Value;
171-
PopulateDbTypeForParameter(parameterEntry, parameter);
172-
cmd.Parameters.Add(parameter);
173-
}
174-
}
156+
DbCommand cmd = PrepareDbCommand(conn, sqltext, parameters, httpContext, dataSourceName);
175157

176158
try
177159
{
@@ -197,6 +179,45 @@ public QueryExecutor(DbExceptionParser dbExceptionParser,
197179
}
198180
}
199181

182+
/// <summary>
183+
/// Prepares a database command for execution.
184+
/// </summary>
185+
/// <param name="conn">Connection object used to connect to database.</param>
186+
/// <param name="sqltext">Sql text to be executed.</param>
187+
/// <param name="parameters">The parameters used to execute the SQL text.</param>
188+
/// <param name="httpContext">Current user httpContext.</param>
189+
/// <param name="dataSourceName">The name of the data source.</param>
190+
/// <returns>A DbCommand object ready for execution.</returns>
191+
public virtual DbCommand PrepareDbCommand(
192+
TConnection conn,
193+
string sqltext,
194+
IDictionary<string, DbConnectionParam> parameters,
195+
HttpContext? httpContext,
196+
string dataSourceName)
197+
{
198+
DbCommand cmd = conn.CreateCommand();
199+
cmd.CommandType = CommandType.Text;
200+
201+
// Add query to send user data from DAB to the underlying database to enable additional security the user might have configured
202+
// at the database level.
203+
string sessionParamsQuery = GetSessionParamsQuery(httpContext, parameters, dataSourceName);
204+
205+
cmd.CommandText = sessionParamsQuery + sqltext;
206+
if (parameters is not null)
207+
{
208+
foreach (KeyValuePair<string, DbConnectionParam> parameterEntry in parameters)
209+
{
210+
DbParameter parameter = cmd.CreateParameter();
211+
parameter.ParameterName = parameterEntry.Key;
212+
parameter.Value = parameterEntry.Value.Value ?? DBNull.Value;
213+
PopulateDbTypeForParameter(parameterEntry, parameter);
214+
cmd.Parameters.Add(parameter);
215+
}
216+
}
217+
218+
return cmd;
219+
}
220+
200221
/// <inheritdoc />
201222
public virtual string GetSessionParamsQuery(HttpContext? httpContext, IDictionary<string, DbConnectionParam> parameters, string dataSourceName = "")
202223
{

src/Core/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ public void AddNullifiedUnspecifiedFields(
124124
/// <summary>
125125
/// Get column type from table underlying the query structure
126126
/// </summary>
127+
/// <param name="columnName">backing column name</param>
127128
public Type GetColumnSystemType(string columnName)
128129
{
129130
if (GetUnderlyingSourceDefinition().Columns.TryGetValue(columnName, out ColumnDefinition? column))
@@ -275,8 +276,8 @@ protected static IEnumerable<Predicate> CreateJoinPredicates(
275276
(leftColumnName, rightColumnName) =>
276277
{
277278
// no table name or schema here is needed because this is a subquery that joins on table alias
278-
Column leftColumn = new(tableSchema: string.Empty, tableName: string.Empty, leftColumnName, leftTableAlias);
279-
Column rightColumn = new(tableSchema: string.Empty, tableName: string.Empty, rightColumnName, rightTableAlias);
279+
Column leftColumn = new(tableSchema: string.Empty, tableName: string.Empty, columnName: leftColumnName, tableAlias: leftTableAlias);
280+
Column rightColumn = new(tableSchema: string.Empty, tableName: string.Empty, columnName: rightColumnName, tableAlias: rightTableAlias);
280281
return new Predicate(
281282
new PredicateOperand(leftColumn),
282283
PredicateOperation.Equal,

src/Core/Resolvers/Sql Query Structures/SqlUpdateQueryStructure.cs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,13 @@ private Predicate CreatePredicateForParam(KeyValuePair<string, object?> param)
159159
Predicate predicate;
160160
// since we have already validated param we know backing column exists
161161
MetadataProvider.TryGetBackingColumn(EntityName, param.Key, out string? backingColumn);
162-
if (param.Value is null && !sourceDefinition.Columns[backingColumn!].IsNullable)
162+
if (backingColumn is null)
163+
{
164+
// If param.Key was not present in the ExposedToBackingColumnMap then provided param.Key is already the backing column
165+
backingColumn = param.Key;
166+
}
167+
168+
if (param.Value is null && !sourceDefinition.Columns[backingColumn].IsNullable)
163169
{
164170
throw new DataApiBuilderException(
165171
message: $"Cannot set argument {param.Key} to null.",
@@ -170,7 +176,7 @@ private Predicate CreatePredicateForParam(KeyValuePair<string, object?> param)
170176
{
171177
predicate = new(
172178
new PredicateOperand(
173-
new Column(tableSchema: DatabaseObject.SchemaName, tableName: DatabaseObject.Name, backingColumn!)),
179+
new Column(tableSchema: DatabaseObject.SchemaName, tableName: DatabaseObject.Name, backingColumn)),
174180
PredicateOperation.Equal,
175181
new PredicateOperand($"{MakeDbConnectionParam(null, backingColumn)}")
176182
);

src/Core/Resolvers/Sql Query Structures/SqlUpsertQueryStructure.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ private void PopulateColumns(
128128
{
129129
// since we have already validated mutationParams we know backing column exists
130130
MetadataProvider.TryGetBackingColumn(EntityName, param.Key, out string? backingColumn);
131+
131132
// Create Parameter and map it to column for downstream logic to utilize.
132133
string paramIdentifier;
133134
if (param.Value is not null)

0 commit comments

Comments
 (0)