Skip to content

Commit

Permalink
Fix to #7348 - Distinct() after Select()ing a property doesn't ge…
Browse files Browse the repository at this point in the history
…nerate correct SQL if query also contains optional navigation

Problem was that optional navigation is translated to GroupJoin which in turn forces materialization on entire entity. This causes some result operators (e.g. Distinct) to be applied on all columns of that entity, even if the customer specified a subset of columns to perform Distinct on.

Fix is to evaluate result operators on the client if their result depends on whether it was applied to a single column or entire row, and the query also contains GroupJoin.
Operators that can still be safely translated to SQL: Skip, Take, FirstOrDefault, Count, All, Any.
  • Loading branch information
maumar committed Jan 10, 2017
1 parent 14f32b2 commit 2df7540
Show file tree
Hide file tree
Showing 4 changed files with 201 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,19 @@ var handlerContext
return handlerContext.EvalOnClient();
}

if (relationalQueryModelVisitor.RequiresClientSingleColumnResultOperator
&& !(resultOperator is SkipResultOperator
|| resultOperator is TakeResultOperator
|| resultOperator is FirstResultOperator
|| resultOperator is SingleResultOperator
|| resultOperator is CountResultOperator
|| resultOperator is AllResultOperator
|| resultOperator is AnyResultOperator
|| resultOperator is GroupResultOperator))
{
return handlerContext.EvalOnClient();
}

return resultHandler(handlerContext);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ private readonly Dictionary<IQuerySource, RelationalQueryModelVisitor> _subQuery
private bool _requiresClientProjection;
private bool _requiresClientOrderBy;
private bool _requiresClientResultOperator;
private bool _requiresClientSingleColumnResultOperator;

private Dictionary<IncludeSpecification, List<int>> _navigationIndexMap = new Dictionary<IncludeSpecification, List<int>>();

Expand Down Expand Up @@ -215,6 +216,18 @@ public virtual bool RequiresClientResultOperator
set { _requiresClientResultOperator = value; }
}

/// <summary>
/// Gets or sets a value indicating whether the query requires client evaluation for result operators potentially apply to a subset of columns rather than entire row.
/// </summary>
/// <value>
/// true if the query requires client single column result operator, false if not.
/// </value>
internal virtual bool RequiresClientSingleColumnResultOperator
{
get { return _requiresClientSingleColumnResultOperator || _requiresClientResultOperator || RequiresClientEval; }
set { _requiresClientSingleColumnResultOperator = value; }
}

/// <summary>
/// Context for the query compilation.
/// </summary>
Expand Down Expand Up @@ -627,6 +640,12 @@ public override void VisitGroupJoinClause(
() => base.VisitGroupJoinClause(groupJoinClause, queryModel, index),
LinqOperatorProvider.GroupJoin,
groupJoin: true);

// Workaround until #6647 is addressed - GroupJoin requires materialization of entire entity which results in all columns of that entity being projected
// this in turn causes result operators to be applied on all of those columns, even if the query specifies a subset of columns to perform the operation on
// this could lead to incorrect results (e.g. for Distinct)
// This however is safe to do for some operators, e.g. FirstOrDefault, Count(), Take() because their result is the same whether they are applied on single column or entire row
RequiresClientSingleColumnResultOperator = true;
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2053,6 +2053,93 @@ public virtual void Complex_predicate_with_AndAlso_and_nullable_bool_property()
}
}

[ConditionalFact]
public virtual void Distinct_with_optional_navigation_is_evaluated_on_client()
{
using (var context = CreateContext())
{
var query = (from g in context.Gears
where g.Tag.Note != "Foo"
select g.HasSoulPatch).Distinct();

var result = query.ToList();
Assert.Equal(2, result.Count);
}
}

[ConditionalFact]
public virtual void Sum_with_optional_navigation_is_evaluated_on_client()
{
using (var context = CreateContext())
{
var expected = (from g in context.Gears.ToList()
select g.SquadId).Sum();

ClearLog();

var actual = (from g in context.Gears
where g.Tag.Note != "Foo"
select g.SquadId).Sum();

Assert.Equal(expected, actual);
}
}

[ConditionalFact]
public virtual void Count_with_optional_navigation_is_translated_to_sql()
{
using (var context = CreateContext())
{
var query = (from g in context.Gears
where g.Tag.Note != "Foo"
select g.HasSoulPatch).Count();

Assert.Equal(5, query);
}
}

[ConditionalFact]
public virtual void FirstOrDefault_with_manually_created_groupjoin_is_translated_to_sql()
{
using (var context = CreateContext())
{
var query = (from s in context.Squads
join g in context.Gears on s.Id equals g.SquadId into grouping
from g in grouping.DefaultIfEmpty()
where s.Name == "Kilo"
select s).FirstOrDefault();

Assert.Equal("Kilo", query.Name);
}
}

[ConditionalFact]
public virtual void Any_with_optional_navigation_as_subquery_predicate_is_translated_to_sql()
{
using (var context = CreateContext())
{
var query = from s in context.Squads
where !s.Members.Any(m => m.Tag.Note == "Dom's Tag")
select s.Name;

var result = query.ToList();

Assert.Equal("Kilo", result.Single());
}
}

[ConditionalFact]
public virtual void All_with_optional_navigation_is_translated_to_sql()
{
using (var context = CreateContext())
{
var query = (from g in context.Gears
select g).All(g => g.Tag.Note != "Foo");

Assert.True(query);
}
}

protected GearsOfWarContext CreateContext() => Fixture.CreateContext(TestStore);

protected GearsOfWarQueryTestBase(TFixture fixture)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2049,6 +2049,88 @@ FROM [Weapon] AS [w]
Sql);
}

public override void Distinct_with_optional_navigation_is_evaluated_on_client()
{
base.Distinct_with_optional_navigation_is_evaluated_on_client();

Assert.Equal(
@"SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOrBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank], [g.Tag].[Id], [g.Tag].[GearNickName], [g.Tag].[GearSquadId], [g.Tag].[Note]
FROM [Gear] AS [g]
LEFT JOIN [CogTag] AS [g.Tag] ON ([g].[Nickname] = [g.Tag].[GearNickName]) AND ([g].[SquadId] = [g.Tag].[GearSquadId])
WHERE [g].[Discriminator] IN (N'Officer', N'Gear') AND (([g.Tag].[Note] <> N'Foo') OR [g.Tag].[Note] IS NULL)
ORDER BY [g].[Nickname], [g].[SquadId]",
Sql);
}

public override void Sum_with_optional_navigation_is_evaluated_on_client()
{
base.Sum_with_optional_navigation_is_evaluated_on_client();

Assert.Equal(
@"SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOrBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank], [g.Tag].[Id], [g.Tag].[GearNickName], [g.Tag].[GearSquadId], [g.Tag].[Note]
FROM [Gear] AS [g]
LEFT JOIN [CogTag] AS [g.Tag] ON ([g].[Nickname] = [g.Tag].[GearNickName]) AND ([g].[SquadId] = [g.Tag].[GearSquadId])
WHERE [g].[Discriminator] IN (N'Officer', N'Gear') AND (([g.Tag].[Note] <> N'Foo') OR [g.Tag].[Note] IS NULL)
ORDER BY [g].[Nickname], [g].[SquadId]",
Sql);
}

public override void Count_with_optional_navigation_is_translated_to_sql()
{
base.Count_with_optional_navigation_is_translated_to_sql();

Assert.Equal(
@"SELECT COUNT(*)
FROM [Gear] AS [g]
LEFT JOIN [CogTag] AS [g.Tag] ON ([g].[Nickname] = [g.Tag].[GearNickName]) AND ([g].[SquadId] = [g.Tag].[GearSquadId])
WHERE [g].[Discriminator] IN (N'Officer', N'Gear') AND (([g.Tag].[Note] <> N'Foo') OR [g.Tag].[Note] IS NULL)",
Sql);
}

public override void FirstOrDefault_with_manually_created_groupjoin_is_translated_to_sql()
{
base.FirstOrDefault_with_manually_created_groupjoin_is_translated_to_sql();

Assert.Equal(
@"SELECT TOP(1) [s].[Id], [s].[InternalNumber], [s].[Name], [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOrBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank]
FROM [Squad] AS [s]
LEFT JOIN [Gear] AS [g] ON [s].[Id] = [g].[SquadId]
WHERE [s].[Name] = N'Kilo'
ORDER BY [s].[Id]",
Sql);
}

public override void Any_with_optional_navigation_as_subquery_predicate_is_translated_to_sql()
{
base.Any_with_optional_navigation_as_subquery_predicate_is_translated_to_sql();

Assert.Equal(
@"SELECT [s].[Name]
FROM [Squad] AS [s]
WHERE NOT EXISTS (
SELECT 1
FROM [Gear] AS [m]
LEFT JOIN [CogTag] AS [m.Tag] ON ([m].[Nickname] = [m.Tag].[GearNickName]) AND ([m].[SquadId] = [m.Tag].[GearSquadId])
WHERE ((([m].[Discriminator] = N'Officer') OR ([m].[Discriminator] = N'Gear')) AND ([m.Tag].[Note] = N'Dom''s Tag')) AND ([s].[Id] = [m].[SquadId]))",
Sql);
}

public override void All_with_optional_navigation_is_translated_to_sql()
{
base.All_with_optional_navigation_is_translated_to_sql();

Assert.Equal(
@"SELECT CASE
WHEN NOT EXISTS (
SELECT 1
FROM [Gear] AS [g]
LEFT JOIN [CogTag] AS [g.Tag] ON ([g].[Nickname] = [g.Tag].[GearNickName]) AND ([g].[SquadId] = [g.Tag].[GearSquadId])
WHERE (([g].[Discriminator] = N'Officer') OR ([g].[Discriminator] = N'Gear')) AND (([g.Tag].[Note] = N'Foo') AND [g.Tag].[Note] IS NOT NULL))
THEN CAST(1 AS BIT) ELSE CAST(0 AS BIT)
END",
Sql);
}

protected override void ClearLog() => TestSqlLoggerFactory.Reset();

private const string FileLineEnding = @"
Expand Down

0 comments on commit 2df7540

Please sign in to comment.