From add19569a13d57bb3146e8cad8c3a03cf347e8c1 Mon Sep 17 00:00:00 2001 From: Joakim Riedel Date: Mon, 23 Nov 2020 10:37:21 +0100 Subject: [PATCH] Query: Reuse parameters for inlined queries coming from same closure variable Resolves #23271 --- .../ParameterExtractingExpressionVisitor.cs | 47 +++++++++++++++---- .../Query/GearsOfWarQueryTestBase.cs | 17 +++++++ .../Query/GearsOfWarQuerySqlServerTest.cs | 30 +++++++++++- .../Query/TPTGearsOfWarQuerySqlServerTest.cs | 2 +- 4 files changed, 85 insertions(+), 11 deletions(-) diff --git a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs index 6ff016f902e..9b92757f082 100644 --- a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs +++ b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs @@ -34,7 +34,7 @@ public class ParameterExtractingExpressionVisitor : ExpressionVisitor private readonly EvaluatableExpressionFindingExpressionVisitor _evaluatableExpressionFindingExpressionVisitor; private readonly ContextParameterReplacingExpressionVisitor _contextParameterReplacingExpressionVisitor; - private readonly Dictionary _evaluatedValues = new(ExpressionEqualityComparer.Instance); + private readonly Dictionary _evaluatedValues = new(ExpressionEqualityComparer.Instance); private IDictionary _evaluatableExpressions; private IQueryProvider? _currentQueryProvider; @@ -76,6 +76,11 @@ public ParameterExtractingExpressionVisitor( /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public virtual Expression ExtractParameters([NotNull] Expression expression) + { + return ExtractParameters(expression, clearEvaluatedValues: true); + } + + private Expression ExtractParameters([NotNull] Expression expression, bool clearEvaluatedValues) { var oldEvaluatableExpressions = _evaluatableExpressions; _evaluatableExpressions = _evaluatableExpressionFindingExpressionVisitor.Find(expression); @@ -87,7 +92,10 @@ public virtual Expression ExtractParameters([NotNull] Expression expression) finally { _evaluatableExpressions = oldEvaluatableExpressions; - _evaluatedValues.Clear(); + if (clearEvaluatedValues) + { + _evaluatedValues.Clear(); + } } } @@ -280,30 +288,43 @@ private static Expression GenerateConstantExpression(object? value, Type returnT private Expression Evaluate(Expression expression, bool generateParameter) { + object? parameterValue; + string? parameterName; if (_evaluatedValues.TryGetValue(expression, out var cachedValue)) { - return cachedValue; - } + var existingExpression = generateParameter ? cachedValue.Parameter : cachedValue.Constant; + if (existingExpression != null) + { + return existingExpression; + } - var parameterValue = GetValue(expression, out var parameterName); + parameterValue = cachedValue.Value; + parameterName = cachedValue.CandidateParameterName; + } + else + { + parameterValue = GetValue(expression, out parameterName); + cachedValue = new EvaluatedValues { CandidateParameterName = parameterName, Value = parameterValue }; + _evaluatedValues[expression] = cachedValue; + } if (parameterValue is IQueryable innerQueryable) { - return ExtractParameters(innerQueryable.Expression); + return ExtractParameters(innerQueryable.Expression, clearEvaluatedValues: false); } if (parameterName?.StartsWith(QueryFilterPrefix, StringComparison.Ordinal) != true) { if (parameterValue is Expression innerExpression) { - return ExtractParameters(innerExpression); + return ExtractParameters(innerExpression, clearEvaluatedValues: false); } if (!generateParameter) { var constantValue = GenerateConstantExpression(parameterValue, expression.Type); - _evaluatedValues.Add(expression, constantValue); + cachedValue.Constant = constantValue; return constantValue; } @@ -337,7 +358,7 @@ var compilerPrefixIndex var parameter = Expression.Parameter(expression.Type, parameterName); - _evaluatedValues.Add(expression, parameter); + cachedValue.Parameter = parameter; return parameter; } @@ -645,5 +666,13 @@ private static bool IsQueryableMethod(Expression expression) => expression is MethodCallExpression methodCallExpression && methodCallExpression.Method.DeclaringType == typeof(Queryable); } + + private sealed class EvaluatedValues + { + public string? CandidateParameterName { get; set; } + public object? Value { get; set; } + public Expression? Constant { get; set; } + public Expression? Parameter { get; set; } + } } } diff --git a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs index 11f571e6290..65b8b5459b6 100644 --- a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs @@ -6684,6 +6684,23 @@ public virtual Task Query_reusing_parameter_doesnt_declare_duplicate_parameter(b assertOrder: true); } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Query_reusing_parameter_with_inner_query_doesnt_declare_duplicate_parameter(bool async) + { + var squadId = 1; + + return AssertQuery( + async, + ss => + { + var innerQuery = ss.Set().Where(s => s.Id == squadId); + var outerQuery = ss.Set().Where(g => innerQuery.Contains(g.Squad)); + return outerQuery.Concat(outerQuery).OrderBy(g => g.FullName); + }, + assertOrder: true); + } + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Query_reusing_parameter_doesnt_declare_duplicate_parameter_complex(bool async) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs index 59137096c07..aafa998f9e8 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs @@ -6281,6 +6281,34 @@ FROM [Gears] AS [g] ORDER BY [t].[FullName]"); } + public override async Task Query_reusing_parameter_with_inner_query_doesnt_declare_duplicate_parameter(bool async) + { + await base.Query_reusing_parameter_with_inner_query_doesnt_declare_duplicate_parameter(async); + + AssertSql( + @"@__squadId_0='1' + +SELECT [t].[Nickname], [t].[SquadId], [t].[AssignedCityName], [t].[CityOfBirthName], [t].[Discriminator], [t].[FullName], [t].[HasSoulPatch], [t].[LeaderNickname], [t].[LeaderSquadId], [t].[Rank] +FROM ( + SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank] + FROM [Gears] AS [g] + INNER JOIN [Squads] AS [s] ON [g].[SquadId] = [s].[Id] + WHERE EXISTS ( + SELECT 1 + FROM [Squads] AS [s0] + WHERE ([s0].[Id] = @__squadId_0) AND ([s0].[Id] = [s].[Id])) + UNION ALL + SELECT [g0].[Nickname], [g0].[SquadId], [g0].[AssignedCityName], [g0].[CityOfBirthName], [g0].[Discriminator], [g0].[FullName], [g0].[HasSoulPatch], [g0].[LeaderNickname], [g0].[LeaderSquadId], [g0].[Rank] + FROM [Gears] AS [g0] + INNER JOIN [Squads] AS [s1] ON [g0].[SquadId] = [s1].[Id] + WHERE EXISTS ( + SELECT 1 + FROM [Squads] AS [s2] + WHERE ([s2].[Id] = @__squadId_0) AND ([s2].[Id] = [s1].[Id])) +) AS [t] +ORDER BY [t].[FullName]"); + } + public override async Task Query_reusing_parameter_doesnt_declare_duplicate_parameter_complex(bool async) { await base.Query_reusing_parameter_doesnt_declare_duplicate_parameter_complex(async); @@ -7094,7 +7122,7 @@ public override async Task Constant_enum_with_same_underlying_value_as_previousl AssertSql( @"@__p_0='1' -SELECT TOP(@__p_0) [g].[Rank] & @__p_0 +SELECT TOP(@__p_0) [g].[Rank] & 1 FROM [Gears] AS [g] ORDER BY [g].[Nickname]"); } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs index 5917ebc7d5b..1c9596910ea 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs @@ -8242,7 +8242,7 @@ public override async Task Constant_enum_with_same_underlying_value_as_previousl AssertSql( @"@__p_0='1' -SELECT TOP(@__p_0) [g].[Rank] & @__p_0 +SELECT TOP(@__p_0) [g].[Rank] & 1 FROM [Gears] AS [g] ORDER BY [g].[Nickname]"); }