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

Set operations: translate OrderBy/Take/Skip directly on set operation instead of pushing down #16244

Open
roji opened this issue Jun 25, 2019 · 3 comments

Comments

@roji
Copy link
Member

roji commented Jun 25, 2019

We currently pushdown to subquery whenever an OrderBy, Take or Offset is applied to it - but these can be applied directly on the set operation, without a subquery (but see #16238 for Take on SqlServer).

For example, for test Select_Union_different_fields_in_anonymous_with_subquery, we currently translate to:

SELECT [t0].[Foo], [t0].[CustomerID], [t0].[Address], [t0].[City], [t0].[CompanyName], [t0].[ContactName], [t0].[ContactTitle], [t0].[Country], [t0].[Fax], [t0].[Phone], [t0].[PostalCode], [t0].[Region]
FROM (
    SELECT [t].[Foo], [t].[CustomerID], [t].[Address], [t].[City], [t].[CompanyName], [t].[ContactName], [t].[ContactTitle], [t].[Country], [t].[Fax], [t].[Phone], [t].[PostalCode], [t].[Region]
    FROM (
        SELECT [c].[City] AS [Foo], [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].[City] = N'Berlin') AND [c].[City] IS NOT NULL
        UNION
        SELECT [c0].[Region] AS [Foo], [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region]
        FROM [Customers] AS [c0]
        WHERE ([c0].[City] = N'London') AND [c0].[City] IS NOT NULL
    ) AS [t]
    ORDER BY [t].[Foo]
    OFFSET @__p_0 ROWS FETCH NEXT @__p_1 ROWS ONLY
) AS [t0]
WHERE ([t0].[Foo] = N'Berlin') AND [t0].[Foo] IS NOT NULL
ORDER BY [t0].[Foo]

We should translate to:

SELECT [t].[Foo], [t].[CustomerID], [t].[Address], [t].[City], [t].[CompanyName], [t].[ContactName], [t].[ContactTitle], [t].[Country], [t].[Fax], [t].[Phone], [t].[PostalCode], [t].[Region]
FROM (
    SELECT [c].[City] AS [Foo], [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].[City] = N'Berlin') AND [c].[City] IS NOT NULL
    UNION
    SELECT [c0].[Region] AS [Foo], [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region]
    FROM [Customers] AS [c0]
    WHERE ([c0].[City] = N'London') AND [c0].[City] IS NOT NULL
    ORDER BY [Foo]
    OFFSET @__p_0 ROWS FETCH NEXT @__p_1 ROWS ONLY
) AS [t]
WHERE ([t].[Foo] = N'Berlin') AND [t].[Foo] IS NOT NULL
ORDER BY [t].[Foo]

The reason we don't currently do this, is that we have no good way for the ORDER BY clause to reference a column alias that has no table alias ([Foo]).

roji added a commit to roji/efcore that referenced this issue Jun 25, 2019
* Include/nav rewriting is now supported.
* We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. dotnet#16244 was opened to track for post-3.0.
* Added missing support for union over subselect projection mappings.
* Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL `INTERSECT ALL`, `EXCEPT ALL`).

Completes dotnet#6812
Fixes dotnet#13196
Fixes dotnet#16065
Fixes dotnet#16165
roji added a commit that referenced this issue Jun 25, 2019
* Include/nav rewriting is now supported.
* We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. #16244 was opened to track for post-3.0.
* Added missing support for union over subselect projection mappings.
* Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL `INTERSECT ALL`, `EXCEPT ALL`).

Completes #6812
Fixes #13196
Fixes #16065
Fixes #16165
@mburbea
Copy link

mburbea commented Jun 25, 2019

positional works in order by clause? So if your concern is the unspeakable name for the union-ed expression, you can always reference it by position.

@roji
Copy link
Member Author

roji commented Jun 25, 2019

@mburbea we actually don't really need positional - the union-ed expression does have a name. The difficulty is an EF Core implementation detail rather than an SQL issue, we simply don't have the infrastructure to make the ordering clause refer to the union-ed expression alias (or position for that matter).

This shouldn't be a critical matter, probably mostly a matter of generating cleaner SQL, so we'll probably defer this for after 3.0.

roji added a commit to roji/efcore that referenced this issue Jun 26, 2019
* Include/nav rewriting is now supported.
* We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. dotnet#16244 was opened to track for post-3.0.
* Added missing support for union over subselect projection mappings.
* Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL `INTERSECT ALL`, `EXCEPT ALL`).

Completes dotnet#6812
Fixes dotnet#13196
Fixes dotnet#16065
Fixes dotnet#16165
roji added a commit to roji/efcore that referenced this issue Jun 27, 2019
* Include/nav rewriting is now supported.
* We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. dotnet#16244 was opened to track for post-3.0.
* Added missing support for union over subselect projection mappings.
* Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL `INTERSECT ALL`, `EXCEPT ALL`).

Completes dotnet#6812
Fixes dotnet#13196
Fixes dotnet#16065
Fixes dotnet#16165
roji added a commit that referenced this issue Jun 27, 2019
* Include/nav rewriting is now supported.
* We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. #16244 was opened to track for post-3.0.
* Added missing support for union over subselect projection mappings.
* Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL `INTERSECT ALL`, `EXCEPT ALL`).

Completes #6812
Fixes #13196
Fixes #16065
Fixes #16165
roji added a commit to roji/efcore that referenced this issue Jun 27, 2019
* Include/nav rewriting is now supported.
* We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. dotnet#16244 was opened to track for post-3.0.
* Added missing support for union over subselect projection mappings.
* Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL `INTERSECT ALL`, `EXCEPT ALL`).

Completes dotnet#6812
Fixes dotnet#13196
Fixes dotnet#16065
Fixes dotnet#16165
roji added a commit that referenced this issue Jun 27, 2019
* Include/nav rewriting is now supported.
* We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. #16244 was opened to track for post-3.0.
* Added missing support for union over subselect projection mappings.
* Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL `INTERSECT ALL`, `EXCEPT ALL`).

Completes #6812
Fixes #13196
Fixes #16065
Fixes #16165
roji added a commit that referenced this issue Jun 28, 2019
* Include/nav rewriting is now supported.
* We now push down to subquery when OrderBy, Take or Skip are applied to set operations, to avoid using the hack where ColumnExpression with no table alias was used. #16244 was opened to track for post-3.0.
* Added missing support for union over subselect projection mappings.
* Added Other type to SetOperationType so that providers can define extra set operations (e.g. PostgreSQL `INTERSECT ALL`, `EXCEPT ALL`).

Completes #6812
Fixes #13196
Fixes #16065
Fixes #16165
roji added a commit that referenced this issue Jun 28, 2019
* We no longer allow ColumnExpression without table alias
* Set operations over operands with different types have been disabled
  for now.
* We now push down set operations into a subquery on Orderby, Skip or
  Take, which shouldn't be necessary (#16244).
* Some test consolidation and cleanup.

Continues #6812
roji added a commit that referenced this issue Jun 28, 2019
* We no longer allow ColumnExpression without table alias
* Set operations over operands with different types have been disabled
  for now.
* We now push down set operations into a subquery on Orderby, Skip or
  Take, which shouldn't be necessary (#16244).
* Some test consolidation and cleanup.

Continues #6812
roji added a commit that referenced this issue Jun 28, 2019
* We no longer allow ColumnExpression without table alias
* Set operations over operands with different types have been disabled
  for now.
* We now push down set operations into a subquery on Orderby, Skip or
  Take, which shouldn't be necessary (#16244).
* Some test consolidation and cleanup.

Continues #6812
@ajcvickers ajcvickers added this to the Backlog milestone Jun 28, 2019
@roji
Copy link
Member Author

roji commented Aug 21, 2019

See #17340 (comment) for an implementation proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants