From 76078933b48a4a3828d66b3e79483e7fecce9f77 Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Wed, 4 Nov 2020 23:48:44 -0800 Subject: [PATCH] SqlServer: Generate False predicate when skip/take both are 0 (#23192) Resolves #22525 --- .../Query/SqlExpressions/SelectExpression.cs | 22 ++-- .../SkipTakeCollapsingExpressionVisitor.cs | 109 ++++++++++++++++++ .../SqlServerParameterBasedSqlProcessor.cs | 5 + .../NorthwindMiscellaneousQueryCosmosTest.cs | 6 + .../Query/NorthwindGroupByQueryTestBase.cs | 42 ++++++- .../NorthwindMiscellaneousQueryTestBase.cs | 26 +++++ .../NorthwindGroupByQuerySqlServerTest.cs | 26 +++++ ...orthwindMiscellaneousQuerySqlServerTest.cs | 34 ++++++ 8 files changed, 255 insertions(+), 15 deletions(-) create mode 100644 src/EFCore.SqlServer/Query/Internal/SkipTakeCollapsingExpressionVisitor.cs diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index a8921c30beb..092d1746d64 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -3105,12 +3105,12 @@ private bool Equals(SelectExpression selectExpression) // This does not take internal states since when using this method SelectExpression should be finalized [Obsolete("Use the overload which does not require distinct & alias parameter.")] public SelectExpression Update( - [NotNull] List projections, - [NotNull] List tables, + [NotNull] IReadOnlyList projections, + [NotNull] IReadOnlyList tables, [CanBeNull] SqlExpression predicate, - [CanBeNull] List groupBy, + [NotNull] IReadOnlyList groupBy, [CanBeNull] SqlExpression having, - [CanBeNull] List orderings, + [NotNull] IReadOnlyList orderings, [CanBeNull] SqlExpression limit, [CanBeNull] SqlExpression offset, bool distinct, @@ -3118,6 +3118,8 @@ public SelectExpression Update( { Check.NotNull(projections, nameof(projections)); Check.NotNull(tables, nameof(tables)); + Check.NotNull(groupBy, nameof(groupBy)); + Check.NotNull(orderings, nameof(orderings)); var projectionMapping = new Dictionary(); foreach (var kvp in _projectionMapping) @@ -3125,7 +3127,7 @@ public SelectExpression Update( projectionMapping[kvp.Key] = kvp.Value; } - return new SelectExpression(alias, projections, tables, groupBy, orderings) + return new SelectExpression(alias, projections.ToList(), tables.ToList(), groupBy.ToList(), orderings.ToList()) { _projectionMapping = projectionMapping, Predicate = predicate, @@ -3152,12 +3154,12 @@ public SelectExpression Update( /// This expression if no children changed, or an expression with the updated children. // This does not take internal states since when using this method SelectExpression should be finalized public SelectExpression Update( - [NotNull] List projections, - [NotNull] List tables, + [NotNull] IReadOnlyList projections, + [NotNull] IReadOnlyList tables, [CanBeNull] SqlExpression? predicate, - [NotNull] List groupBy, + [NotNull] IReadOnlyList groupBy, [CanBeNull] SqlExpression? having, - [NotNull] List orderings, + [NotNull] IReadOnlyList orderings, [CanBeNull] SqlExpression? limit, [CanBeNull] SqlExpression? offset) { @@ -3172,7 +3174,7 @@ public SelectExpression Update( projectionMapping[kvp.Key] = kvp.Value; } - return new SelectExpression(Alias, projections, tables, groupBy, orderings) + return new SelectExpression(Alias, projections.ToList(), tables.ToList(), groupBy.ToList(), orderings.ToList()) { _projectionMapping = projectionMapping, Predicate = predicate, diff --git a/src/EFCore.SqlServer/Query/Internal/SkipTakeCollapsingExpressionVisitor.cs b/src/EFCore.SqlServer/Query/Internal/SkipTakeCollapsingExpressionVisitor.cs new file mode 100644 index 00000000000..fd1523fffe4 --- /dev/null +++ b/src/EFCore.SqlServer/Query/Internal/SkipTakeCollapsingExpressionVisitor.cs @@ -0,0 +1,109 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; +using System.Linq.Expressions; +using JetBrains.Annotations; +using Microsoft.EntityFrameworkCore.Query; +using Microsoft.EntityFrameworkCore.Query.SqlExpressions; +using Microsoft.EntityFrameworkCore.Utilities; + +#nullable enable + +namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal +{ + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public class SkipTakeCollapsingExpressionVisitor : ExpressionVisitor + { + private readonly ISqlExpressionFactory _sqlExpressionFactory; + + private IReadOnlyDictionary _parameterValues; + private bool _canCache; + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public SkipTakeCollapsingExpressionVisitor([NotNull] ISqlExpressionFactory sqlExpressionFactory) + { + Check.NotNull(sqlExpressionFactory, nameof(sqlExpressionFactory)); + + _sqlExpressionFactory = sqlExpressionFactory; + _parameterValues = null!; + } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public virtual SelectExpression Process( + [NotNull] SelectExpression selectExpression, + [NotNull] IReadOnlyDictionary parametersValues, + out bool canCache) + { + Check.NotNull(selectExpression, nameof(selectExpression)); + Check.NotNull(parametersValues, nameof(parametersValues)); + + _parameterValues = parametersValues; + _canCache = true; + + var result = (SelectExpression)Visit(selectExpression); + + canCache = _canCache; + + return result; + } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override Expression VisitExtension(Expression extensionExpression) + { + if (extensionExpression is SelectExpression selectExpression) + { + if (IsZero(selectExpression.Limit) + && IsZero(selectExpression.Offset)) + { + return selectExpression.Update( + selectExpression.Projection, + selectExpression.Tables, + selectExpression.GroupBy.Count > 0 ? selectExpression.Predicate : _sqlExpressionFactory.Constant(false), + selectExpression.GroupBy, + selectExpression.GroupBy.Count > 0 ? _sqlExpressionFactory.Constant(false) : null, + new List(0), + limit: null, + offset: null); + } + + bool IsZero(SqlExpression? sqlExpression) + { + switch (sqlExpression) + { + case SqlConstantExpression constant: + return ((int)constant.Value) == 0; + case SqlParameterExpression parameter: + _canCache = false; + return ((int)_parameterValues[parameter.Name]) == 0; + + default: + return false; + } + } + } + + return base.VisitExtension(extensionExpression); + } + } +} diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerParameterBasedSqlProcessor.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerParameterBasedSqlProcessor.cs index 25f7dca4b03..b483bd70968 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerParameterBasedSqlProcessor.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerParameterBasedSqlProcessor.cs @@ -48,6 +48,11 @@ public override SelectExpression Optimize( var optimizedSelectExpression = base.Optimize(selectExpression, parametersValues, out canCache); + optimizedSelectExpression = new SkipTakeCollapsingExpressionVisitor(Dependencies.SqlExpressionFactory) + .Process(optimizedSelectExpression, parametersValues, out var canCache2); + + canCache &= canCache2; + return (SelectExpression)new SearchConditionConvertingExpressionVisitor(Dependencies.SqlExpressionFactory) .Visit(optimizedSelectExpression); } diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs index 7a5b17df82c..df795b7a239 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs @@ -4202,6 +4202,12 @@ public override Task First_on_collection_in_projection(bool async) return base.First_on_collection_in_projection(async); } + [ConditionalTheory(Skip = "Non embedded collection subquery Issue#17246")] + public override Task Skip_0_Take_0_works_when_constant(bool async) + { + return base.Skip_0_Take_0_works_when_constant(async); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); diff --git a/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs index bd6e8865495..f352aad01ad 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindGroupByQueryTestBase.cs @@ -1288,9 +1288,10 @@ public virtual Task Element_selector_with_case_block_repeated_inside_another_cas async, ss => from order in ss.Set() group new - { - IsAlfki = order.CustomerID == "ALFKI", OrderId = order.OrderID > 1000 ? order.OrderID : -order.OrderID - } by + { + IsAlfki = order.CustomerID == "ALFKI", + OrderId = order.OrderID > 1000 ? order.OrderID : -order.OrderID + } by new { order.OrderID } into g select new { g.Key.OrderID, Aggregate = g.Sum(s => s.IsAlfki ? s.OrderId : -s.OrderId) }); @@ -2326,6 +2327,35 @@ public virtual Task GroupBy_aggregate_join_another_GroupBy_aggregate(bool async) elementSorter: o => o.Key); } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task GroupBy_aggregate_after_skip_0_take_0(bool async) + { + return AssertQuery( + async, + ss => ss.Set() + .Skip(0) + .Take(0) + .GroupBy(o => o.CustomerID) + .Select(g => new { g.Key, Total = g.Count() }), + elementSorter: o => o.Key); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task GroupBy_skip_0_take_0_aggregate(bool async) + { + return AssertQuery( + async, + ss => ss.Set() + .Where(e => e.OrderID > 10500) + .GroupBy(o => o.CustomerID) + .Skip(0) + .Take(0) + .Select(g => new { g.Key, Total = g.Count() }), + elementSorter: o => o.Key); + } + #endregion #region GroupByAggregateChainComposition @@ -2523,7 +2553,8 @@ public virtual Task GroupBy_group_Distinct_Select_Distinct_aggregate(bool async) g => new { - g.Key, Max = g.Distinct().Select(e => e.OrderDate).Distinct().Max(), + g.Key, + Max = g.Distinct().Select(e => e.OrderDate).Distinct().Max(), }), elementSorter: e => e.Key); } @@ -2540,7 +2571,8 @@ public virtual Task GroupBy_group_Where_Select_Distinct_aggregate(bool async) g => new { - g.Key, Max = g.Where(e => e.OrderDate.HasValue).Select(e => e.OrderDate).Distinct().Max(), + g.Key, + Max = g.Where(e => e.OrderDate.HasValue).Select(e => e.OrderDate).Distinct().Max(), }), elementSorter: e => e.Key); } diff --git a/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs index 05fe424f885..b1721ee75ea 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs @@ -6336,5 +6336,31 @@ public virtual Task SkipWhile_throws_meaningful_exception(bool async) async, ss => ss.Set().OrderBy(c => c.CustomerID).SkipWhile(c => c.CustomerID != "Foo").Skip(1))); } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Skip_0_Take_0_works_when_parameter(bool async) + { + await AssertQuery( + async, + ss => ss.Set().OrderBy(c => c.CustomerID).Skip(0).Take(0)); + + await AssertQuery( + async, + ss => ss.Set().OrderBy(c => c.CustomerID).Skip(1).Take(1), + entryCount: 1); + } + + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Skip_0_Take_0_works_when_constant(bool async) + { + return AssertQueryScalar( + async, + ss => ss.Set().Where(c => c.CustomerID.StartsWith("F")) + .OrderBy(c => c.CustomerID) + .Select(e => e.Orders.OrderBy(o => o.OrderID).Skip(0).Take(0).Any()), + assertOrder: true); + } } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs index 6b4fe41f472..2e779294975 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs @@ -2327,6 +2327,32 @@ GROUP BY [o0].[CustomerID] ) AS [t0] ON [t].[Key] = [t0].[Key]"); } + public override async Task GroupBy_aggregate_after_skip_0_take_0(bool async) + { + await base.GroupBy_aggregate_after_skip_0_take_0(async); + + AssertSql( + @"SELECT [t].[CustomerID] AS [Key], COUNT(*) AS [Total] +FROM ( + SELECT [o].[CustomerID] + FROM [Orders] AS [o] + WHERE 0 = 1 +) AS [t] +GROUP BY [t].[CustomerID]"); + } + + public override async Task GroupBy_skip_0_take_0_aggregate(bool async) + { + await base.GroupBy_skip_0_take_0_aggregate(async); + + AssertSql( + @"SELECT [o].[CustomerID] AS [Key], COUNT(*) AS [Total] +FROM [Orders] AS [o] +WHERE [o].[OrderID] > 10500 +GROUP BY [o].[CustomerID] +HAVING 0 = 1"); + } + public override async Task GroupBy_with_grouping_key_using_Like(bool async) { await base.GroupBy_with_grouping_key_using_Like(async); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs index c94b658a600..ee921bd1f2a 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs @@ -5300,6 +5300,40 @@ FROM [Employees] AS [e0] ) AS [t2]"); } + public override async Task Skip_0_Take_0_works_when_parameter(bool async) + { + await base.Skip_0_Take_0_works_when_parameter(async); + + AssertSql( + @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] +FROM [Customers] AS [c] +WHERE 0 = 1", + // + @"@__p_0='1' + +SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] +FROM [Customers] AS [c] +ORDER BY [c].[CustomerID] +OFFSET @__p_0 ROWS FETCH NEXT @__p_0 ROWS ONLY"); + } + + public override async Task Skip_0_Take_0_works_when_constant(bool async) + { + await base.Skip_0_Take_0_works_when_constant(async); + + AssertSql( + @"SELECT CASE + WHEN EXISTS ( + SELECT 1 + FROM [Orders] AS [o] + WHERE 0 = 1) THEN CAST(1 AS bit) + ELSE CAST(0 AS bit) +END +FROM [Customers] AS [c] +WHERE [c].[CustomerID] LIKE N'F%' +ORDER BY [c].[CustomerID]"); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);