From b9aed79cfe162c7acdd1b2cfed24b96b02c23cb3 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 1 Jul 2019 18:45:53 +0200 Subject: [PATCH 1/2] Handle IN optimizer transformation of ordering to constant (take 2) Our InExpressionValuesExpanding visitor can turn an an IN expression to a constant (i.e. when the values list is empty), which is unsupported in orderings in SqlServer. Detect this and turn these expressions to a simple constant expression detectable by the SQL generator, which will generate (SELECT 1). Fixes #15713 --- ...ressionValuesExpandingExpressionVisitor.cs | 56 ++++++++++++++++++ .../SqlExpressions/SelectExpression.cs | 6 +- .../Query/SimpleQueryCosmosTest.cs | 13 +++++ .../Query/GearsOfWarQueryTestBase.cs | 2 +- .../Query/IncludeTestBase.cs | 4 +- .../Query/SimpleQueryTestBase.cs | 16 ++++- .../Query/GearsOfWarQuerySqlServerTest.cs | 16 ++--- .../Query/IncludeSqlServerTest.cs | 58 ++++++++----------- .../Query/SimpleQuerySqlServerTest.cs | 18 ++++++ 9 files changed, 135 insertions(+), 54 deletions(-) diff --git a/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs b/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs index 1191f992ad2..8c0596c091a 100644 --- a/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs @@ -1,6 +1,7 @@ // 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; using System.Collections; using System.Collections.Generic; using System.Linq.Expressions; @@ -15,12 +16,15 @@ private class InExpressionValuesExpandingExpressionVisitor : ExpressionVisitor { private readonly ISqlExpressionFactory _sqlExpressionFactory; private IReadOnlyDictionary _parametersValues; + private ConstantExpressionIdentifyingExpressionVisitor _constantIdentifyingVisitor; + private bool _innerExpressionChangedToConstant; public InExpressionValuesExpandingExpressionVisitor( ISqlExpressionFactory sqlExpressionFactory, IReadOnlyDictionary parametersValues) { _sqlExpressionFactory = sqlExpressionFactory; _parametersValues = parametersValues; + _constantIdentifyingVisitor = new ConstantExpressionIdentifyingExpressionVisitor(); } public override Expression Visit(Expression expression) @@ -34,6 +38,7 @@ public override Expression Visit(Expression expression) switch (inExpression.Values) { + // TODO: This shouldn't be here - should be part of compilation instead (#16375) case SqlConstantExpression sqlConstant: { typeMapping = sqlConstant.TypeMapping; @@ -91,6 +96,7 @@ public override Expression Visit(Expression expression) if (updatedInExpression == null && nullCheckExpression == null) { + _innerExpressionChangedToConstant = true; return _sqlExpressionFactory.Equal(_sqlExpressionFactory.Constant(true), _sqlExpressionFactory.Constant(inExpression.Negated)); } @@ -99,6 +105,56 @@ public override Expression Visit(Expression expression) return base.Visit(expression); } + + protected override Expression VisitExtension(Expression extensionExpression) + { + var visited = base.VisitExtension(extensionExpression); + if (_innerExpressionChangedToConstant && visited is OrderingExpression orderingExpression) + { + // Our rewriting of InExpression above may have left an ordering which contains only constants - + // invalid in SqlServer. If so, rewrite the entire ordering to a single constant expression which + // QuerySqlGenerator will recognize and render as (SELECT 1). + _innerExpressionChangedToConstant = false; + _constantIdentifyingVisitor.Reset(); + _constantIdentifyingVisitor.Visit(orderingExpression); + return _constantIdentifyingVisitor.WasConstant + ? new OrderingExpression( + new SqlConstantExpression(Expression.Constant(1), _sqlExpressionFactory.FindMapping(typeof(int))), + true) + : orderingExpression; + } + + return visited; + } + } + + /// + /// Searches an expression tree for the first occurrence of a node meeting the given criteria, and returns that node. + /// + private class ConstantExpressionIdentifyingExpressionVisitor : ExpressionVisitor + { + public bool WasConstant { get; private set; } + + /// + /// Resets the visitor, making it ready to run again. + /// + public void Reset() => WasConstant = true; + + public override Expression Visit(Expression node) + { + // TODO: can be optimized by immediately returning null when a matching node is found (but must be handled everywhere...) + + switch (node) + { + case ColumnExpression _: + case SqlParameterExpression _: + case SqlFragmentExpression _: // May or may not be constant, but better to error than to give wrong results + WasConstant = false; + break; + } + + return base.Visit(node); + } } } } diff --git a/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs index fb5d9ba181c..eac686771ff 100644 --- a/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs @@ -1181,9 +1181,9 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) var orderings = new List(); foreach (var ordering in _orderings) { - var orderingExpression = (SqlExpression)visitor.Visit(ordering.Expression); - changed |= orderingExpression != ordering.Expression; - orderings.Add(ordering.Update(orderingExpression)); + var newOrdering = (OrderingExpression)visitor.Visit(ordering); + changed |= newOrdering != ordering; + orderings.Add(newOrdering); } var offset = (SqlExpression)visitor.Visit(Offset); diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs index 8cfcf09c276..50ce94d4c36 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs @@ -3642,6 +3642,7 @@ FROM root c WHERE ((c[""Discriminator""] = ""Order"") AND ((c[""OrderID""] > 690) AND (c[""OrderID""] < 710)))"); } + [ConditionalTheory(Skip = "Should work, requires testing etc.")] public override async Task OrderBy_empty_list_contains(bool isAsync) { await base.OrderBy_empty_list_contains(isAsync); @@ -3652,6 +3653,18 @@ FROM root c WHERE (c[""Discriminator""] = ""Customer"")"); } + [ConditionalTheory(Skip = "Should work, requires testing etc.")] + public override async Task OrderBy_contains_and_count(bool isAsync) + { + await base.OrderBy_contains_and_count(isAsync); + + AssertSql( + @"SELECT c +FROM root c +WHERE (c[""Discriminator""] = ""Customer"")"); + } + + [ConditionalTheory(Skip = "Should work, requires testing etc.")] public override async Task OrderBy_empty_list_does_not_contains(bool isAsync) { await base.OrderBy_empty_list_does_not_contains(isAsync); diff --git a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs index fe88ee904a7..857c4e44f76 100644 --- a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs @@ -6794,7 +6794,7 @@ public virtual Task Cast_ordered_subquery_to_base_type_using_typed_ToArray(bool elementAsserter: CollectionAsserter(e => e.Nickname, (e, a) => Assert.Equal(e.Nickname, a.Nickname))); } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Correlated_collection_with_complex_order_by_funcletized_to_constant_bool(bool isAsync) { diff --git a/test/EFCore.Specification.Tests/Query/IncludeTestBase.cs b/test/EFCore.Specification.Tests/Query/IncludeTestBase.cs index 4fea65ad523..61c08b19941 100644 --- a/test/EFCore.Specification.Tests/Query/IncludeTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/IncludeTestBase.cs @@ -3933,7 +3933,7 @@ var orders } } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [InlineData(false)] [InlineData(true)] public virtual void Include_collection_OrderBy_empty_list_contains(bool useString) @@ -3968,7 +3968,7 @@ var customers } } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [InlineData(false)] [InlineData(true)] public virtual void Include_collection_OrderBy_empty_list_does_not_contains(bool useString) diff --git a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs index f5c5bdd0d1e..a8cd15f5203 100644 --- a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs @@ -5735,7 +5735,7 @@ on o.CustomerID equals c.CustomerID .Take(5)); } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task OrderBy_empty_list_contains(bool isAsync) { @@ -5747,7 +5747,19 @@ public virtual Task OrderBy_empty_list_contains(bool isAsync) entryCount: 91); } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task OrderBy_contains_and_count(bool isAsync) + { + var list = new List(); + + return AssertQuery( + isAsync, + cs => cs.OrderBy(c => (list.Contains(c.CustomerID) ? 1 : 0) + cs.Count()).Select(c => c), + entryCount: 91); + } + + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task OrderBy_empty_list_does_not_contains(bool isAsync) { diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs index 58919d135a1..b716aa5d9da 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs @@ -7343,19 +7343,11 @@ public override async Task Correlated_collection_with_complex_order_by_funcletiz await base.Correlated_collection_with_complex_order_by_funcletized_to_constant_bool(isAsync); AssertSql( - @"SELECT [g].[Nickname], [g].[FullName] + @"SELECT [g].[Nickname], [g].[SquadId], [w].[Name], [w].[Id], [w].[OwnerFullName] FROM [Gears] AS [g] -WHERE [g].[Discriminator] IN (N'Officer', N'Gear') -ORDER BY [g].[Nickname], [g].[SquadId], [g].[FullName]", - // - @"SELECT [t].[c], [t].[Nickname], [t].[SquadId], [t].[FullName], [g.Weapons].[Name], [g.Weapons].[OwnerFullName] -FROM [Weapons] AS [g.Weapons] -INNER JOIN ( - SELECT CAST(0 AS bit) AS [c], [g0].[Nickname], [g0].[SquadId], [g0].[FullName] - FROM [Gears] AS [g0] - WHERE [g0].[Discriminator] IN (N'Officer', N'Gear') -) AS [t] ON [g.Weapons].[OwnerFullName] = [t].[FullName] -ORDER BY [t].[c] DESC, [t].[Nickname], [t].[SquadId], [t].[FullName]"); +LEFT JOIN [Weapons] AS [w] ON [g].[FullName] = [w].[OwnerFullName] +WHERE [g].[Discriminator] IN (N'Gear', N'Officer') +ORDER BY [g].[Nickname], [g].[SquadId], [w].[Id]"); } public override async Task Double_order_by_on_nullable_bool_coming_from_optional_navigation(bool isAsync) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/IncludeSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/IncludeSqlServerTest.cs index d0a4f22f991..357d1c3ad58 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/IncludeSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/IncludeSqlServerTest.cs @@ -1120,24 +1120,19 @@ public override void Include_collection_OrderBy_empty_list_contains(bool useStri AssertSql( @"@__p_1='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] -WHERE [c].[CustomerID] LIKE N'A%' -ORDER BY (SELECT 1), [c].[CustomerID] -OFFSET @__p_1 ROWS", - // - @"@__p_1='1' - -SELECT [c.Orders].[OrderID], [c.Orders].[CustomerID], [c.Orders].[EmployeeID], [c.Orders].[OrderDate] -FROM [Orders] AS [c.Orders] -INNER JOIN ( - SELECT [c0].[CustomerID], CAST(0 AS bit) AS [c] - FROM [Customers] AS [c0] - WHERE [c0].[CustomerID] LIKE N'A%' - ORDER BY [c], [c0].[CustomerID] +SELECT [t].[CustomerID], [t].[Address], [t].[City], [t].[CompanyName], [t].[ContactName], [t].[ContactTitle], [t].[Country], [t].[Fax], [t].[Phone], [t].[PostalCode], [t].[Region], [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate] +FROM ( + SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region], CASE + WHEN CAST(1 AS bit) = CAST(0 AS bit) THEN CAST(1 AS bit) + ELSE CAST(0 AS bit) + END AS [c] + FROM [Customers] AS [c] + WHERE [c].[CustomerID] LIKE N'A%' + ORDER BY (SELECT 1) OFFSET @__p_1 ROWS -) AS [t] ON [c.Orders].[CustomerID] = [t].[CustomerID] -ORDER BY [t].[c], [t].[CustomerID]"); +) AS [t] +LEFT JOIN [Orders] AS [o] ON [t].[CustomerID] = [o].[CustomerID] +ORDER BY [t].[c], [t].[CustomerID], [o].[OrderID]"); } public override void Include_collection_OrderBy_empty_list_does_not_contains(bool useString) @@ -1147,24 +1142,19 @@ public override void Include_collection_OrderBy_empty_list_does_not_contains(boo AssertSql( @"@__p_1='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] -WHERE [c].[CustomerID] LIKE N'A%' -ORDER BY (SELECT 1), [c].[CustomerID] -OFFSET @__p_1 ROWS", - // - @"@__p_1='1' - -SELECT [c.Orders].[OrderID], [c.Orders].[CustomerID], [c.Orders].[EmployeeID], [c.Orders].[OrderDate] -FROM [Orders] AS [c.Orders] -INNER JOIN ( - SELECT [c0].[CustomerID], CAST(1 AS bit) AS [c] - FROM [Customers] AS [c0] - WHERE [c0].[CustomerID] LIKE N'A%' - ORDER BY [c], [c0].[CustomerID] +SELECT [t].[CustomerID], [t].[Address], [t].[City], [t].[CompanyName], [t].[ContactName], [t].[ContactTitle], [t].[Country], [t].[Fax], [t].[Phone], [t].[PostalCode], [t].[Region], [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate] +FROM ( + SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region], CASE + WHEN CAST(1 AS bit) = CAST(1 AS bit) THEN CAST(1 AS bit) + ELSE CAST(0 AS bit) + END AS [c] + FROM [Customers] AS [c] + WHERE [c].[CustomerID] LIKE N'A%' + ORDER BY (SELECT 1) OFFSET @__p_1 ROWS -) AS [t] ON [c.Orders].[CustomerID] = [t].[CustomerID] -ORDER BY [t].[c], [t].[CustomerID]"); +) AS [t] +LEFT JOIN [Orders] AS [o] ON [t].[CustomerID] = [o].[CustomerID] +ORDER BY [t].[c], [t].[CustomerID], [o].[OrderID]"); } public override void Include_collection_OrderBy_list_contains(bool useString) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs index 25884ae75ed..6a99432afe0 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs @@ -4681,6 +4681,24 @@ public override async Task OrderBy_empty_list_contains(bool isAsync) FROM [Customers] AS [c]"); } + public override async Task OrderBy_contains_and_count(bool isAsync) + { + await base.OrderBy_contains_and_count(isAsync); + + AssertSql( + @"SELECT COUNT(*) +FROM [Customers] AS [c]", + // + @"@__Count_1='91' + +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 CASE + WHEN CAST(1 AS bit) = CAST(0 AS bit) THEN 1 + ELSE 0 +END + @__Count_1"); + } + public override async Task OrderBy_empty_list_does_not_contains(bool isAsync) { await base.OrderBy_empty_list_does_not_contains(isAsync); From 71276f352e36d90b6655b5b6c03ee46afc849c1c Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Thu, 4 Jul 2019 14:01:54 +0200 Subject: [PATCH 2/2] Address review comments Also stop treating parameters as non-constant --- ...ressionValuesExpandingExpressionVisitor.cs | 27 +++++++++---------- .../Query/SimpleQuerySqlServerTest.cs | 10 ++----- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs b/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs index 8c0596c091a..5f31822823d 100644 --- a/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs @@ -16,7 +16,7 @@ private class InExpressionValuesExpandingExpressionVisitor : ExpressionVisitor { private readonly ISqlExpressionFactory _sqlExpressionFactory; private IReadOnlyDictionary _parametersValues; - private ConstantExpressionIdentifyingExpressionVisitor _constantIdentifyingVisitor; + private ConstantExpressionIdentifier _constantExpressionIdentifier; private bool _innerExpressionChangedToConstant; public InExpressionValuesExpandingExpressionVisitor( @@ -24,7 +24,7 @@ public InExpressionValuesExpandingExpressionVisitor( { _sqlExpressionFactory = sqlExpressionFactory; _parametersValues = parametersValues; - _constantIdentifyingVisitor = new ConstantExpressionIdentifyingExpressionVisitor(); + _constantExpressionIdentifier = new ConstantExpressionIdentifier(); } public override Expression Visit(Expression expression) @@ -115,9 +115,7 @@ protected override Expression VisitExtension(Expression extensionExpression) // invalid in SqlServer. If so, rewrite the entire ordering to a single constant expression which // QuerySqlGenerator will recognize and render as (SELECT 1). _innerExpressionChangedToConstant = false; - _constantIdentifyingVisitor.Reset(); - _constantIdentifyingVisitor.Visit(orderingExpression); - return _constantIdentifyingVisitor.WasConstant + return _constantExpressionIdentifier.IsExpressionConstant(orderingExpression) ? new OrderingExpression( new SqlConstantExpression(Expression.Constant(1), _sqlExpressionFactory.FindMapping(typeof(int))), true) @@ -129,16 +127,18 @@ protected override Expression VisitExtension(Expression extensionExpression) } /// - /// Searches an expression tree for the first occurrence of a node meeting the given criteria, and returns that node. + /// Visits an expression tree returns identifies whether it contains any non-constant nodes, e.g. columns. /// - private class ConstantExpressionIdentifyingExpressionVisitor : ExpressionVisitor + private class ConstantExpressionIdentifier : ExpressionVisitor { - public bool WasConstant { get; private set; } + private bool _wasConstant; - /// - /// Resets the visitor, making it ready to run again. - /// - public void Reset() => WasConstant = true; + public bool IsExpressionConstant(Expression node) + { + _wasConstant = true; + Visit(node); + return _wasConstant; + } public override Expression Visit(Expression node) { @@ -147,9 +147,8 @@ public override Expression Visit(Expression node) switch (node) { case ColumnExpression _: - case SqlParameterExpression _: case SqlFragmentExpression _: // May or may not be constant, but better to error than to give wrong results - WasConstant = false; + _wasConstant = false; break; } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs index 6a99432afe0..638729276d7 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs @@ -4689,14 +4689,8 @@ public override async Task OrderBy_contains_and_count(bool isAsync) @"SELECT COUNT(*) FROM [Customers] AS [c]", // - @"@__Count_1='91' - -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 CASE - WHEN CAST(1 AS bit) = CAST(0 AS bit) THEN 1 - ELSE 0 -END + @__Count_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]"); } public override async Task OrderBy_empty_list_does_not_contains(bool isAsync)