Skip to content

Commit

Permalink
Pushdown subquery when applying distinct in presence of limit
Browse files Browse the repository at this point in the history 
Distinct clears ordering and in the presence of limit it can create incorrect results.
Add counter in RowNumberPagingExpressionVisitor because for a multi level nested subquery, inner subquery can provide column with RowNumber while we are adding it on outer level which causes name clash for a subquery
Counter make sure to unique-fy them for given select expression.
Moved the visitor to constructor
  • Loading branch information
@smitpatel
smitpatel committed Apr 19, 2017
1 parent eabc74e commit 57b4cb9
Show file tree
Hide file tree
Showing 6 changed files with 434 additions and 126 deletions.
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/Expressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public virtual bool IsDistinct
get => _isDistinct;
set
{
if (_offset != null)
if (_offset != null || _limit != null)
{
PushDownSubquery();
}
Expand Down
84 changes: 75 additions & 9 deletions src/EFCore.Specification.Tests/QueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7188,7 +7188,7 @@ from o2 in os.Where(o => o.CustomerID == c.CustomerID).DefaultIfEmpty()
}

[ConditionalFact]
public virtual void OrderBy_skip_take_level_1()
public virtual void OrderBy_skip_take()
{
AssertQuery<Customer>(
cs => cs.OrderBy(c => c.ContactTitle)
Expand All @@ -7200,7 +7200,7 @@ public virtual void OrderBy_skip_take_level_1()
}

[ConditionalFact]
public virtual void OrderBy_skip_take_level_2()
public virtual void OrderBy_skip_take_take()
{
AssertQuery<Customer>(
cs => cs.OrderBy(c => c.ContactTitle)
Expand All @@ -7213,33 +7213,99 @@ public virtual void OrderBy_skip_take_level_2()
}

[ConditionalFact]
public virtual void OrderBy_skip_take_distinct()
public virtual void OrderBy_skip_take_take_take_take()
{
AssertQuery<Customer>(
cs => cs.OrderBy(c => c.ContactTitle)
.ThenBy(c => c.ContactName)
.Skip(5)
.Take(15)
.Distinct(),
assertOrder: false,
entryCount: 15);
.Take(10)
.Take(8)
.Take(5),
assertOrder: true,
entryCount: 5);
}

[ConditionalFact]
public virtual void OrderBy_skip_take_level_3()
public virtual void OrderBy_skip_take_skip_take_skip()
{
AssertQuery<Customer>(
cs => cs.OrderBy(c => c.ContactTitle)
.ThenBy(c => c.ContactName)
.Skip(5)
.Take(15)
.Take(10)
.Skip(2)
.Take(8)
.Take(5),
.Skip(5),
assertOrder: true,
entryCount: 3);
}

[ConditionalFact]
public virtual void OrderBy_skip_take_distinct()
{
AssertQuery<Customer>(
cs => cs.OrderBy(c => c.ContactTitle)
.ThenBy(c => c.ContactName)
.Skip(5)
.Take(15)
.Distinct(),
assertOrder: false,
entryCount: 15);
}

[ConditionalFact]
public virtual void OrderBy_coalesce_take_distinct()
{
AssertQuery<Product>(
ps => ps.OrderBy(p => p.UnitPrice ?? 0)
.Take(15)
.Distinct(),
assertOrder: false,
entryCount: 15);
}

[ConditionalFact]
public virtual void OrderBy_coalesce_skip_take_distinct()
{
AssertQuery<Product>(
ps => ps.OrderBy(p => p.UnitPrice ?? 0)
.Skip(5)
.Take(15)
.Distinct(),
assertOrder: false,
entryCount: 15);
}

[ConditionalFact]
public virtual void OrderBy_coalesce_skip_take_distinct_take()
{
AssertQuery<Product>(
ps => ps.OrderBy(p => p.UnitPrice ?? 0)
.Skip(5)
.Take(15)
.Distinct()
.Take(5),
assertOrder: false,
entryCount: 5);
}

[ConditionalFact]
public virtual void OrderBy_skip_take_distinct_orderby_take()
{
AssertQuery<Customer>(
cs => cs.OrderBy(c => c.ContactTitle)
.ThenBy(c => c.ContactName)
.Skip(5)
.Take(15)
.Distinct()
.OrderBy(c => c.ContactTitle)
.Take(8),
assertOrder: false,
entryCount: 8);
}

[ConditionalFact]
public virtual void No_orderby_added_for_fully_translated_manually_constructed_LOJ()
{
Expand Down
21 changes: 9 additions & 12 deletions src/EFCore.SqlServer/Query/Sql/Internal/SqlServerQuerySqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using JetBrains.Annotations;
Expand All @@ -21,9 +22,6 @@ namespace Microsoft.EntityFrameworkCore.Query.Sql.Internal
/// </summary>
public class SqlServerQuerySqlGenerator : DefaultQuerySqlGenerator, ISqlServerExpressionVisitor
{
private const string RowNumberColumnName = "__RowNumber__";
private readonly RowNumberPagingExpressionVisitor _rowNumberPagingExpressionVisitor;

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
Expand All @@ -36,17 +34,11 @@ public SqlServerQuerySqlGenerator(
{
if (rowNumberPagingEnabled)
{
_rowNumberPagingExpressionVisitor = new RowNumberPagingExpressionVisitor();
var rowNumberPagingExpressionVisitor = new RowNumberPagingExpressionVisitor();
rowNumberPagingExpressionVisitor.Visit(selectExpression);
}
}

public override Expression VisitSelect(SelectExpression selectExpression)
{
_rowNumberPagingExpressionVisitor?.Visit(selectExpression);

return base.VisitSelect(selectExpression);
}

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
Expand Down Expand Up @@ -128,6 +120,9 @@ protected override void GenerateProjection(Expression projection)

private class RowNumberPagingExpressionVisitor : ExpressionVisitorBase
{
private const string RowNumberColumnName = "__RowNumber__";
private int _counter;

public override Expression Visit(Expression expression)
{
if (expression is ExistsExpression existsExpression)
Expand Down Expand Up @@ -167,9 +162,11 @@ private Expression VisitSelectExpression(SelectExpression selectExpression)
}

var innerRowNumberExpression = new AliasExpression(
RowNumberColumnName,
RowNumberColumnName + (_counter != 0 ? $"{_counter}" : ""),
new RowNumberExpression(subQuery.OrderBy));

_counter++;

subQuery.ClearOrderBy();
subQuery.AddToProjection(innerRowNumberExpression, resetProjectStar: false);

Expand Down
106 changes: 61 additions & 45 deletions test/EFCore.SqlServer.FunctionalTests/IncludeSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,12 +282,16 @@ FROM [Orders] AS [o]
@"SELECT [o.Customer.Orders].[OrderID], [o.Customer.Orders].[CustomerID], [o.Customer.Orders].[EmployeeID], [o.Customer.Orders].[OrderDate]
FROM [Orders] AS [o.Customer.Orders]
INNER JOIN (
SELECT DISTINCT TOP(1) [o.Customer0].[CustomerID]
FROM [Orders] AS [o0]
LEFT JOIN [Customers] AS [o.Customer0] ON [o0].[CustomerID] = [o.Customer0].[CustomerID]
WHERE [o0].[OrderID] = 10248
) AS [t] ON [o.Customer.Orders].[CustomerID] = [t].[CustomerID]
ORDER BY [t].[CustomerID]");
SELECT DISTINCT [t].*
FROM (
SELECT TOP(1) [o.Customer0].[CustomerID]
FROM [Orders] AS [o0]
LEFT JOIN [Customers] AS [o.Customer0] ON [o0].[CustomerID] = [o.Customer0].[CustomerID]
WHERE [o0].[OrderID] = 10248
ORDER BY [o.Customer0].[CustomerID]
) AS [t]
) AS [t0] ON [o.Customer.Orders].[CustomerID] = [t0].[CustomerID]
ORDER BY [t0].[CustomerID]");
}

public override void Include_multi_level_collection_and_then_include_reference_predicate(bool useString)
Expand Down Expand Up @@ -826,41 +830,49 @@ OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY
SELECT [c1.Orders].[OrderID], [c1.Orders].[CustomerID], [c1.Orders].[EmployeeID], [c1.Orders].[OrderDate]
FROM [Orders] AS [c1.Orders]
INNER JOIN (
SELECT DISTINCT TOP(@__p_1) [t1].[CustomerID]
SELECT DISTINCT [t3].*
FROM (
SELECT TOP(@__p_0) [c1].*
FROM [Customers] AS [c1]
ORDER BY [c1].[CustomerID]
) AS [t1]
CROSS JOIN (
SELECT [c2].*
FROM [Customers] AS [c2]
ORDER BY [c2].[CustomerID]
OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY
) AS [t2]
) AS [t3] ON [c1.Orders].[CustomerID] = [t3].[CustomerID]
ORDER BY [t3].[CustomerID]",
SELECT TOP(@__p_1) [t1].[CustomerID]
FROM (
SELECT TOP(@__p_0) [c1].*
FROM [Customers] AS [c1]
ORDER BY [c1].[CustomerID]
) AS [t1]
CROSS JOIN (
SELECT [c2].*
FROM [Customers] AS [c2]
ORDER BY [c2].[CustomerID]
OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY
) AS [t2]
ORDER BY [t1].[CustomerID]
) AS [t3]
) AS [t4] ON [c1.Orders].[CustomerID] = [t4].[CustomerID]
ORDER BY [t4].[CustomerID]",
//
@"@__p_1: 1
@__p_0: 2
SELECT [c2.Orders].[OrderID], [c2.Orders].[CustomerID], [c2.Orders].[EmployeeID], [c2.Orders].[OrderDate]
FROM [Orders] AS [c2.Orders]
INNER JOIN (
SELECT DISTINCT TOP(@__p_1) [t5].[CustomerID], [t4].[CustomerID] AS [c0]
SELECT DISTINCT [t7].*
FROM (
SELECT TOP(@__p_0) [c3].*
FROM [Customers] AS [c3]
ORDER BY [c3].[CustomerID]
) AS [t4]
CROSS JOIN (
SELECT [c4].*
FROM [Customers] AS [c4]
ORDER BY [c4].[CustomerID]
OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY
) AS [t5]
) AS [t6] ON [c2.Orders].[CustomerID] = [t6].[CustomerID]
ORDER BY [t6].[c0], [t6].[CustomerID]");
SELECT TOP(@__p_1) [t6].[CustomerID], [t5].[CustomerID] AS [c0]
FROM (
SELECT TOP(@__p_0) [c3].*
FROM [Customers] AS [c3]
ORDER BY [c3].[CustomerID]
) AS [t5]
CROSS JOIN (
SELECT [c4].*
FROM [Customers] AS [c4]
ORDER BY [c4].[CustomerID]
OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY
) AS [t6]
ORDER BY [t5].[CustomerID], [t6].[CustomerID]
) AS [t7]
) AS [t8] ON [c2.Orders].[CustomerID] = [t8].[CustomerID]
ORDER BY [t8].[c0], [t8].[CustomerID]");
}
}

Expand Down Expand Up @@ -971,20 +983,24 @@ OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY
SELECT [c1.Orders].[OrderID], [c1.Orders].[CustomerID], [c1.Orders].[EmployeeID], [c1.Orders].[OrderDate]
FROM [Orders] AS [c1.Orders]
INNER JOIN (
SELECT DISTINCT TOP(@__p_1) [t1].[CustomerID]
SELECT DISTINCT [t3].*
FROM (
SELECT TOP(@__p_0) [c1].*
FROM [Customers] AS [c1]
ORDER BY [c1].[CustomerID]
) AS [t1]
CROSS JOIN (
SELECT [c2].*
FROM [Customers] AS [c2]
ORDER BY [c2].[CustomerID]
OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY
) AS [t2]
) AS [t3] ON [c1.Orders].[CustomerID] = [t3].[CustomerID]
ORDER BY [t3].[CustomerID]");
SELECT TOP(@__p_1) [t1].[CustomerID]
FROM (
SELECT TOP(@__p_0) [c1].*
FROM [Customers] AS [c1]
ORDER BY [c1].[CustomerID]
) AS [t1]
CROSS JOIN (
SELECT [c2].*
FROM [Customers] AS [c2]
ORDER BY [c2].[CustomerID]
OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY
) AS [t2]
ORDER BY [t1].[CustomerID]
) AS [t3]
) AS [t4] ON [c1.Orders].[CustomerID] = [t4].[CustomerID]
ORDER BY [t4].[CustomerID]");
}
}

Expand Down
Loading

0 comments on commit 57b4cb9

Please sign in to comment.