Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OrderBy lifting specification #16226

Closed
divega opened this issue Jun 24, 2019 · 7 comments
Closed

OrderBy lifting specification #16226

divega opened this issue Jun 24, 2019 · 7 comments
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@divega
Copy link
Contributor

divega commented Jun 24, 2019

Copying here our old spec from EF 4 (produced originally by Colin Meek and I), because I think it can still help set direction for EF Core every time we need to make a decision, e.g. recently in #16144. I have made a few edits to remove details that aren't relevant, like ASP.NET WebForms query extenders or EF DbExpression.

OrderBy Lifting for LINQ to Entities

Current behavior

The current design of LINQ to Entities requires that OrderBy is specified immediately before paging operators like Skip and Take/Top. Also, when certain operators (especially filters) are applied on top of a query containing an OderBy operator, the OrderBy is often ignored.

The basic contract is that OrderBy applies only to the next operation in the query tree, which is in alignment with the behavior of the underlying relational database. For instance, in SQL, when composing a query on top of another query, it is illegal for the inner query to be ordered:

SELECT TOP(1) *
FROM (SELECT *
      FROM Products 
      ORDER BY UnitPrice DESC) AS OrderedProducts 
WHERE ProductName LIKE 'c%'

In theory, this query could be modified to preserve the order by placing a SELECT TOP (100) PERCENT in the inner query, but there are no guarantees that further operations applied in the outer query will preserve the ordering (i.e. the database engine could choose a completely different index to optimize a filter operation), and therefore the end result could be that this query would be forcing an expensive sorting operation to happen in vain. For this reason, Entity Framework chooses to assume that the OrderBy in an inner query like this can be ignored.

The following query is not an actual query translated from EF, but it server the purpose of illustrating how even if the inner query specifies ordering, this one is overridden by the filter in the outer query:

SELECT TOP(1) *
FROM (SELECT TOP (100) PERCENT *
      FROM Products 
      ORDER BY UnitPrice DESC) AS OrderedProducts 
WHERE ProductName LIKE 'c%'

Here the results of for the inner query when executed alone on a version of the Northwind sample database:

ProductID ProductName SupplierID CategoryID QuantityPerUnit UnitPrice
38 Côte de Blaye 18 1 12 - 75 cl bottles 263.50
29 Thüringer Rostbratwurst 12 6 50 bags x 30 sausgs. 123.79
9 Mishi Kobe Niku 4 6 18 - 500 g pkgs. 97.00

However, the results for the full query are like this:

ProductID ProductName SupplierID CategoryID QuantityPerUnit UnitPrice
1 Chai 1 1 10 boxes x 20 bags 18.00

Expected behavior

The current design differs from common user expectations and intent, in particular, of LINQ users who are accustomed to a contract in which the query processor tries to honor the semantics of IEnumerable<T>.

In this contract, OrderBy becomes more of a declaration of the order in which results need to be returned thereafter, rather than as a query operator that is applied to the query results at a certain point.

LINQ to Objects produces ordered results trivially because operators are applied directly to an actual in-memory sequence. LINQ to SQL on the other side will very often lift the OrderBy operation in the query expression to produce a behavior that is consistent with LINQ to Objects. For the aforementioned example, LINQ to SQL will most likely produce a query similar to this:

SELECT TOP (1) *
FROM Products 
WHERE ProductName LIKE 'c%'
ORDER BY UnitPrice DESC

This will return the expected row:

ProductID ProductName SupplierID CategoryID QuantityPerUnit UnitPrice
38 Côte de Blaye 18 1 12 - 75 cl bottles 263.50

Here is another way to write this query using LINQ that clearly illustrates the significance of the gap between the current and the expected behavior:

context.Products
    .OrderBy(p => -p.UnitPrice)
    .First(p => p.ProdcutName.StartsWith("c"));

In LINQ to SQL or LINQ to Objects, First() will correctly return the most expensive product with a name that starts with “c”.

In LINQ to Entities, on the other hand, the price is ignored and the first product that has a name that starts with “c” in an arbitrary order (generally the order of the most efficient index the database finds that optimizes the application of the filter predicate) is returned.

Proposed solution

The proposed solution consists of applying an OrderBy lifting operation as a step in LINQ translation . This will result in a change of the observable behavior of LINQ to Entities in the sense that the order requested will be more often preserved on query composition.

Details

The basic strategy is to rewrite expressions bottom up, lifting sorts as we go. One slightly catch: as the sorted expression is lifted, it may optionally include a projection, in which case some additional rebinding may be required. The following argument patterns to collection operators are recognized:

  1. PS: Projectp(Sorts(TheRest))
  2. S: Sorts(TheRest)
  3. expr: arbitrary expression
Operator Arguments Translation
Apply/ CrossJoin/ Join   None: if the left hand-side is sorted with an identifying expression, it is possible to preserve order. However, we don’t currently have a mechanism to determine if an expression is identifying for a collection.
Distinct (PS) None: If we attempt to pull the sort above the distinct, we risk returning duplicates: Projectp(Sorts(Distinct(TheRest))). We cannot pull the sort above the project in general because we cannot invert the projection function: Sorts compose inverse(p)(Distinct(Projectp(TheRest)))
  (S) Sorts(Distinct(TheRest))
Except/ Intersect/ UnionAll (PS, expr) None: we can’t invert the projection to lift the sort, e.g.: Sorts compose inverse(p)(op(Projectp(TheRest), expr)
  (S, expr) The first sequence argument controls the order of the result, which is consistent with the behavior of LINQ to Objects: Sorts(op(TheRest, expr))
Filterf (PS) Projectp(Sorts(Filterf compose p(TheRest))
  (S) Sorts(Filterf(TheRest))
GroupBy   None: hopefully for obvious reasons
Limit/ Skip   We replicate the sort abovethe Limit/Skip: Sorts(Limitl(Skips(Sorts(TheRest))))
OfType (PS) Slightly interesting: this operator is mostly like a filter, but requires rebinding of elements when the target type is a supertype of the input. Sorts [compose treat] compose p(OfType(Projectp(TheRest)))
  (S) Sorts [compose treat](OfType(TheRest))
Projectq (PS) Projectq compose p(Sorts(TheRest))
  (S) None: this works already.
Sortt (PS) Projectp(Sort{s compose p, t}(TheRest))
  (S) Sort{s, t}(TheRest)
LINQSkip (S, expr) Skips(TheRest, expr) current behavior
  (PS, expr) Projectp(Skips compose p(TheRest)) new behavior to account for above patterns

Possible additional cases

  1. We might consider handling expr.OrderBy(…).Take(…).Skip(…), although it is an odd construct.
  2. LINQSkip could also get the Sort operation replicated above.
  3. We could also lift OrderBy for Project if the projection includes the Sort expression.

Note on LINQSkip

LINQSkip is just the LINQ Skip operator. In EF there is a DbSkipExpression which includes an Input, a Count and a SortOrder. The LINQ Skip operator takes only an Input and a Count. DbSkip and LINQSkip therefore have different handling.

@smitpatel
Copy link
Member

Also see #16086

@divega
Copy link
Contributor Author

divega commented Jun 24, 2019

Here is a different take on the transformations implemented in EF4, using a different notation that may be more helpful. I haven't gone trough it yet to verify if there are any incongruities.

  1. Root: source.Sort(o)
    1. source.Sort(o).Filter(f) → source.Filter(f).Sort(o)
    2. source.Sort(o).Project(p)
    3. source.Sort(o).Limit (k)
    4. source.Sort(o).Skip(k) → source.Skip(k, o)
  2. Root: source.Skip(k, o)
    1. source.Skip(k, o).Filter(f) → source.Skip(k, o).Filter(f).Sort(o)
    2. source.Skip(k, o).Project(p)
    3. source.Skip(k, o).Limit (k2)
    4. source.Skip(k, o).Skip(k2) → source.Skip(k + k2, o) or source.Skip(k, o).Skip(k2, o) when either k or k2 is not a constant.
  3. Root: source.Sort(o).Project(p)
    1. source.Sort(o).Project(p).Filter(f) → source.Filter(e => f(p(e))).Sort(o).Project(p)
    2. source.Sort(o).Project(p).Project(p2) → source.Sort(o).Project(e => p2(p(e)))
    3. source.Sort(o).Project(p). Limit (k)
    4. source.Sort(o).Project(p).Skip(k) → source.Skip(k, o).Project(p)
  4. Root: source.Skip(k, o).Project(p)
    1. source.Skip(k, o).Project(p).Filter(f) → source.Skip(k, o).Filter(e => f(p(e))).Sort(o).Project(p)
    2. source.Skip(k, o).Project(p).Project(p2) → source.Skip(k, o).Project(e => p2(p(e)))
    3. source.Skip(k, o).Project(p).Limit (k)
    4. source.Skip(k, o).Project(p).Skip(k2) → source.Skip(k + k2, o).Project(p) or source.Skip(k, o).Skip(k2, o).Project(p) when either k or k2 is not a constant.
  5. Root: source.Sort(o).Limit(k).Project(p)
    1. source.Sort(o).Limit(k).Project(p).Filter(f) → source.Sort(o).Limit(k).Filter(e => f(p(e))).Sort(o).Project(p)
    2. source.Sort(o).Limit(k).Project(p).Project(p2) → source.Sort(o).Limit(k).Project(e => p2(p(e)))
    3. source.Sort(o).Limit(k).Project(p).Limit(k2) → source.Sort(o).Limit(Min(k, k2)).Project(p) or source.Sort(o).Limit(k).Sort(o).Limit(k2).Project(p) when either k or k2 is not a constant
    4. source.Sort(o).Limit(k).Project(p).Skip(k2) → source.Sort(o).Limit(k).Skip(k2, o).Project(p)
  6. Root: source.Skip(k, o).Limit(k2).Project(p)
    1. source.Skip(k, o).Limit(k2).Project(p).Filter(f) → source.Skip(k, o).Limit(k2).Filter(e => f(p(e))).Sort(o).Project(p)
    2. source.Skip(k, o).Limit(k2).Project(p).Project(p2) → source.Skip(k, o).Limit(k2).Project(e => p2(p(e)))
    3. source.Skip(k, o).Limit(k2).Project(p).Limit(k3) → source.Skip(k, o).Limit(Min(k2, k3)).Project(p) or source.Skip(k, o).Limit(k2).Sort(o).Limit(k3).Project(p) when either k or k2 is not a constant.
    4. source.Skip(k, o).Limit(k2).Project(p).Skip(k3) → source.Skip(k + k3, o).Limit(k2 – k3).Project(p) or source.Skip(k, o).Limit(k2).Skip(k3, o).Project(p) when either k, k2 or k3 is not a constant.
  7. Root: source.Sort(o).Limit(k)
    1. source.Sort(o).Limit(k).Filter(f) → source.Sort(o).Limit(k).Filter(f).Sort(o)
    2. source.Sort(o).Limit(k).Project(p)
    3. source.Sort(o).Limit(k).Limit(k2) → source.Sort(o).Limit(Min(k, k2)) or source.Sort(o).Limit(k).Sort(o).Limit(k2) when either k or k2 is not a constant.
    4. source.Sort(o).Limit(k).Skip(k2) → source.Sort(o).Limit(k).Skip(k2, o)
  8. Root: source.Skip(k, o).Limit(k2)
    1. source.Skip(k, o).Limit(k2).Filter(f) → source.Skip(k, o).Limit(k2).Filter(f).Sort(o)
    2. source.Skip(k, o).Limit(k2).Project(p)
    3. source.Skip(k, o).Limit(k2).Limit(k3) → source.Skip(k, o).Limit(Min(k2, k3)) or source.Skip(k, o).Limit(k2).Sort(o).Limit(k3) when either k2 or k3 is not a constant.
    4. source.Skip(k, o).Limit(k2).Skip(k3) → source.Skip(k, o).Limit(k2).Skip(k3, o)
  9. Root: source.Sort(p).Project(p).Limit(k)
    1. source.Sort(o).Project(p).Limit(k).* is equivalent to transformation for source.Sort(o).Limit(k).Project(p).*
  10. Root: source.Skip(k, o).Project(p).Limit(k2)
    1. source.Skip(k, o).Project(p).Limit(k2).* is equivalent to transformation for source.Skip(k, o).Limit(k2).Project(p).*

smitpatel added a commit that referenced this issue Jun 28, 2019
Bugs fixed
- Allow ToList on collection projection to be called with less derived type
- Project out necessary identifiers for collection join when pushing down. (Except distinct case)
- Lift ordering from inner collection to generated ordered result (This would be improved further by #16226)
- Map Cast from Enumerable to Queryable
- Translate AsQueryable as no-op

Resolves #12098
Resolves #15043
Resolves #15611
Part of #15711
@ajcvickers ajcvickers added this to the 3.0.0 milestone Jun 28, 2019
smitpatel added a commit that referenced this issue Jun 28, 2019
Bugs fixed
- Allow ToList on collection projection to be called with less derived type
- Project out necessary identifiers for collection join when pushing down. (Except distinct case)
- Lift ordering from inner collection to generated ordered result (This would be improved further by #16226)
- Map Cast from Enumerable to Queryable
- Translate AsQueryable as no-op

Resolves #12098
Resolves #15043
Resolves #15611
Part of #15711
smitpatel added a commit that referenced this issue Jun 28, 2019
Bugs fixed
- Allow ToList on collection projection to be called with less derived type
- Project out necessary identifiers for collection join when pushing down. (Except distinct case)
- Lift ordering from inner collection to generated ordered result (This would be improved further by #16226)
- Map Cast from Enumerable to Queryable
- Translate AsQueryable as no-op

Resolves #12098
Resolves #15043
Resolves #15611
Part of #15711
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview7 Jul 2, 2019
@smitpatel smitpatel removed this from the 3.0.0 milestone Jul 9, 2019
@smitpatel
Copy link
Member

I have added some observations #16086 (comment)

While a lot of things in above specification seem to be working already. I don't think we need to do anything anymore unless we have a customer issue.

@ajcvickers ajcvickers added this to the Backlog milestone Jul 19, 2019
@ajcvickers
Copy link
Member

Putting this in the backlog to revisit and see if there are any other parts we should do.

@smitpatel
Copy link
Member

3 cases where we are differing from original specification

  • We don't lift ordering if Distinct is specified.

    • We clear ordering when distinct is applied (unless skip/take in which case we do a pushdown first). This was one point been discussed few times in past if we should do it or not.
    • OrderBy term needs to appear in projection to apply distinct
    • Where we deviate from spec is pushdown. Spec lifts ordering out of distinct if all the columns of distinct are present.
  • We preserve ordering from outer in case of Cross join. Spec says not possible. It just happens to be. In any join outer ordering will remain.

  • Set operations - Spec preserves ordering from source1. We currently don't. We could possibly add it.

@smitpatel smitpatel removed this from the 5.0.0 milestone Jan 7, 2020
@smitpatel
Copy link
Member

Removing from milestone to discuss with the team.

@smitpatel smitpatel added this to the 3.1.0 milestone Jan 8, 2020
@smitpatel
Copy link
Member

We decided in design meeting that we are preserving current behaviour in source code even though it does not match with specification. We will reconsider if we get a customer issue specifying current behaviour is incorrect.

@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants