Skip to content

Commit

Permalink
Fix to #29667 - Count after Take throws "No column name was specified…
Browse files Browse the repository at this point in the history
… for column 1 of 't'."

Problem was that in some cases (e.g. count over keyless entity that has been pushed down) we now generate empty projection in the subquery, where before we were projecting some columns.
SQL Server doesn't allow unaliased projection in the subquery, so the fix is to simply add an alias to the empty projection that we generate.

Fixes #29667
  • Loading branch information
maumar committed Jan 25, 2023
1 parent 00e218a commit 2d74565
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 1 deletion.
11 changes: 10 additions & 1 deletion src/EFCore.Relational/Query/QuerySqlGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ protected override Expression VisitSelect(SelectExpression selectExpression)
}
else
{
_relationalCommandBuilder.Append("1");
GenerateEmptyProjection(selectExpression);
}

if (selectExpression.Tables.Any())
Expand Down Expand Up @@ -309,6 +309,15 @@ protected virtual void GeneratePseudoFromClause()
{
}

/// <summary>
/// Generates empty projection for a SelectExpression.
/// </summary>
/// <param name="selectExpression">SelectExpression for which the empty projection will be generated.</param>
protected virtual void GenerateEmptyProjection(SelectExpression selectExpression)
{
_relationalCommandBuilder.Append("1");
}

/// <inheritdoc />
protected override Expression VisitProjection(ProjectionExpression projectionExpression)
{
Expand Down
18 changes: 18 additions & 0 deletions src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal;
/// </summary>
public class SqlServerQuerySqlGenerator : QuerySqlGenerator
{
private static readonly bool UseOldBehavior29667
= AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue29667", out var enabled29667) && enabled29667;

private readonly IRelationalTypeMappingSource _typeMappingSource;

/// <summary>
Expand Down Expand Up @@ -72,6 +75,21 @@ protected override Expression VisitDelete(DeleteExpression deleteExpression)
RelationalStrings.ExecuteOperationWithUnsupportedOperatorInSqlGeneration(nameof(RelationalQueryableExtensions.ExecuteDelete)));
}

/// <summary>
/// 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.
/// </summary>
protected override void GenerateEmptyProjection(SelectExpression selectExpression)
{
base.GenerateEmptyProjection(selectExpression);
if (!UseOldBehavior29667 && selectExpression.Alias != null)
{
Sql.Append(" AS empty");
}
}

/// <summary>
/// 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,30 @@ SELECT c
""");
}

public override async Task Count_over_keyless_entity(bool async)
{
await base.Count_over_keyless_entity(async);

AssertSql(
"""
SELECT COUNT(1) AS c
FROM root c
WHERE (c["Discriminator"] = "Customer")
""");
}

public override async Task Count_over_keyless_entity_with_pushdown(bool async)
{
// Cosmos client evaluation. Issue #17246.
await AssertTranslationFailed(() => base.Count_over_keyless_entity_with_pushdown(async));
}

public override async Task Count_over_keyless_entity_with_pushdown_empty_projection(bool async)
{
// Cosmos client evaluation. Issue #17246.
await AssertTranslationFailed(() => base.Count_over_keyless_entity_with_pushdown_empty_projection(async));
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,25 @@ public virtual Task Collection_correlated_with_keyless_entity_in_predicate_works
.Select(pv => new { pv.City, pv.ContactName })
.OrderBy(x => x.ContactName)
.Take(2));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Count_over_keyless_entity(bool async)
=> AssertCount(
async,
ss => ss.Set<CustomerQuery>());

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Count_over_keyless_entity_with_pushdown(bool async)
=> AssertCount(
async,
ss => ss.Set<CustomerQuery>().OrderBy(x => x.ContactTitle).Take(10));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Count_over_keyless_entity_with_pushdown_empty_projection(bool async)
=> AssertCount(
async,
ss => ss.Set<CustomerQuery>().Take(10));
}
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,54 @@ public override async Task KeylessEntity_with_included_nav(bool async)
""");
}

public override async Task Count_over_keyless_entity(bool async)
{
await base.Count_over_keyless_entity(async);

AssertSql(
"""
SELECT COUNT(*)
FROM (
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]
) AS [m]
""");
}

public override async Task Count_over_keyless_entity_with_pushdown(bool async)
{
await base.Count_over_keyless_entity_with_pushdown(async);

AssertSql(
"""
@__p_0='10'
SELECT COUNT(*)
FROM (
SELECT TOP(@__p_0) [m].[ContactTitle]
FROM (
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]
) AS [m]
ORDER BY [m].[ContactTitle]
) AS [t]
""");
}

public override async Task Count_over_keyless_entity_with_pushdown_empty_projection(bool async)
{
await base.Count_over_keyless_entity_with_pushdown_empty_projection(async);

AssertSql(
"""
@__p_0='10'
SELECT COUNT(*)
FROM (
SELECT TOP(@__p_0) 1 AS empty
FROM (
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]
) AS [m]
) AS [t]
""");
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down

0 comments on commit 2d74565

Please sign in to comment.