Skip to content

Commit

Permalink
Fix to #6647 - Left Join (GroupJoin) always materializes elements res…
Browse files Browse the repository at this point in the history
…ulting in unnecessary data pulling

Problem was that for GroupJoin we would always force materialization on participating query sources.
We were doing this because we are not always able to correctly divide outer elements of the GroupJoin into correct groups.
However, if the GroupJoin clause is wrapped around SelectMany clause the groups don't matter because they are getting flattened by SelectMany anyway.

Fix is to recognize those scenarios and only force materialization when the correct grouping actually matters.
We can avoid materialization if the GroupJoin is followed by SelectMany clause (that references the grouping) and that the grouping itself is not present anywhere else in the query.
This addresses optional navigations, which is the 80% case. Manually created GroupJoins that are not modeling LeftOuterJoins still require additional materialization, but this can be addressed later as the priority is not nearly as high.

This change also addresses #7722 - Query : error during compilation for queries with navigation properties and First/Single/client method operators inside a subquery.

Problem here was that for some queries we don't know how to properly bind to a value buffer (when the result of binding is subquery, and not qsre).
Fix/mitigation is to recognize those scenarios and force materialization on the subqueries. This can be properly addressed in later commit (i.e. by improving the binding logic)

Also fixed several minor issues that were encountered once no longer do extensive materialization.
  • Loading branch information
maumar committed Mar 3, 2017
1 parent e521030 commit 42d6f97
Show file tree
Hide file tree
Showing 15 changed files with 754 additions and 343 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
Expand All @@ -11,6 +12,7 @@
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Query.Expressions;
using Microsoft.EntityFrameworkCore.Query.Expressions.Internal;
using Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal;
using Microsoft.EntityFrameworkCore.Query.Internal;
using Microsoft.EntityFrameworkCore.Query.ResultOperators.Internal;
Expand Down Expand Up @@ -129,6 +131,21 @@ protected override Expression VisitMethodCall(MethodCallExpression node)
return base.VisitMethodCall(node);
}

protected override Expression VisitExtension(Expression node)
{
var nullConditionalExpression = node as NullConditionalExpression;
if (nullConditionalExpression != null)
{
Visit(nullConditionalExpression.AccessOperation);
Visit(nullConditionalExpression.Caller);
Visit(nullConditionalExpression.NullableCaller);

return node;
}

return base.VisitExtension(node);
}

/// <summary>
/// Visit an entity query root.
/// </summary>
Expand Down
29 changes: 0 additions & 29 deletions src/EFCore.Relational/Query/Expressions/LeftOuterJoinExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,35 +40,6 @@ protected override Expression Accept(ExpressionVisitor visitor)
: base.Accept(visitor);
}

/// <summary>
/// Determines whether or not this LeftOuterJoinExpression handles the given query source.
/// </summary>
/// <param name="querySource"> The query source. </param>
/// <returns>
/// true if the supplied query source is handled by this LeftOuterJoinExpression; otherwise false.
/// </returns>
public override bool HandlesQuerySource(IQuerySource querySource)
{
if (querySource is AdditionalFromClause additionalFromClause)
{
var subQueryModel = (additionalFromClause.FromExpression as SubQueryExpression)?.QueryModel;
if (subQueryModel != null
&& !subQueryModel.BodyClauses.Any()
&& subQueryModel.ResultOperators.Count == 1
&& subQueryModel.ResultOperators[0] is DefaultIfEmptyResultOperator
&& (subQueryModel.SelectClause.Selector as QuerySourceReferenceExpression)?.ReferencedQuerySource == subQueryModel.MainFromClause
&& (subQueryModel.MainFromClause.FromExpression as QuerySourceReferenceExpression)?.ReferencedQuerySource is GroupJoinClause targetGroupJoin)
{
if (QuerySource == targetGroupJoin.JoinClause)
{
return true;
}
}
}

return base.HandlesQuerySource(querySource);
}

/// <summary>
/// Creates a <see cref="string" /> representation of the Expression.
/// </summary>
Expand Down
4 changes: 3 additions & 1 deletion src/EFCore.Relational/Query/Expressions/SelectExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,9 @@ public override bool HandlesQuerySource([NotNull] IQuerySource querySource)
{
Check.NotNull(querySource, nameof(querySource));

return _tables.Any(te => te.QuerySource == querySource || te.HandlesQuerySource(querySource));
var processedQuerySource = PreProcessQuerySource(querySource);

return _tables.Any(te => te.QuerySource == processedQuerySource || te.HandlesQuerySource(processedQuerySource));
}

/// <summary>
Expand Down
33 changes: 32 additions & 1 deletion src/EFCore.Relational/Query/Expressions/TableExpressionBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using System.Linq.Expressions;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.Utilities;
using Remotion.Linq.Clauses;
using Remotion.Linq.Clauses.Expressions;
using Remotion.Linq.Clauses.ResultOperators;

namespace Microsoft.EntityFrameworkCore.Query.Expressions
{
Expand Down Expand Up @@ -88,7 +91,35 @@ public virtual bool HandlesQuerySource([NotNull] IQuerySource querySource)
{
Check.NotNull(querySource, nameof(querySource));

return _querySource == querySource;
return _querySource == PreProcessQuerySource(querySource);
}

protected virtual IQuerySource PreProcessQuerySource([NotNull] IQuerySource querySource)
{
Check.NotNull(querySource, nameof(querySource));

var newQuerySource = ExtractGroupJoinFromSelectManySubquery(querySource);

return (newQuerySource as GroupJoinClause)?.JoinClause ?? newQuerySource;
}

private IQuerySource ExtractGroupJoinFromSelectManySubquery(IQuerySource querySource)
{
if (querySource is AdditionalFromClause additionalFromClause)
{
var subQueryModel = (additionalFromClause.FromExpression as SubQueryExpression)?.QueryModel;
if (subQueryModel != null
&& !subQueryModel.BodyClauses.Any()
&& subQueryModel.ResultOperators.Count == 1
&& subQueryModel.ResultOperators[0] is DefaultIfEmptyResultOperator
&& (subQueryModel.SelectClause.Selector as QuerySourceReferenceExpression)?.ReferencedQuerySource == subQueryModel.MainFromClause
&& (subQueryModel.MainFromClause.FromExpression as QuerySourceReferenceExpression)?.ReferencedQuerySource is GroupJoinClause targetGroupJoin)
{
return targetGroupJoin;
}
}

return querySource;
}

/// <summary>
Expand Down
3 changes: 0 additions & 3 deletions src/EFCore.Relational/Query/RelationalQueryModelVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,6 @@ public virtual SelectExpression TryGetQuery([NotNull] IQuerySource querySource)
{
Check.NotNull(querySource, nameof(querySource));

querySource
= (querySource as GroupJoinClause)?.JoinClause ?? querySource;

SelectExpression selectExpression;
return QueriesBySource.TryGetValue(querySource, out selectExpression)
? selectExpression
Expand Down
155 changes: 153 additions & 2 deletions src/EFCore.Specification.Tests/ComplexNavigationsQueryTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3119,6 +3119,157 @@ public virtual void Complex_query_with_optional_navigations_and_client_side_eval
(e, a) => Assert.Equal(e.Id, a.Id));
}

[ConditionalFact]
public virtual void Required_navigation_on_a_subquery_with_First_in_projection()
{
AssertQuery<Level2>(
l2s => l2s
.Where(l2o => l2o.Id == 7)
.Select(l2o => l2s.OrderBy(l2i => l2i.Id).First().OneToOne_Required_FK_Inverse.Name));
}

[ConditionalFact]
public virtual void Required_navigation_on_a_subquery_with_complex_projection_and_First()
{
AssertQuery<Level1, Level2>(
(l1s, l2s) =>
from l2o in l2s
where l2o.Id == 7
select
(from l2i in l2s
join l1i in l1s
on l2i.Level1_Required_Id equals l1i.Id
orderby l2i.Id ascending
select new { Navigation = l2i.OneToOne_Required_FK_Inverse, Contant = 7 }).First().Navigation.Name);
}

[ConditionalFact]
public virtual void Required_navigation_on_a_subquery_with_First_in_predicate()
{
AssertQuery<Level2>(
l2s => l2s
.Where(l2o => l2o.Id == 7)
.Where(l1 => EF.Property<string>(l2s.OrderBy(l2i => l2i.Id).First().OneToOne_Required_FK_Inverse, "Name") == "L1 02"),
l2s => l2s
.Where(l2o => l2o.Id == 7)
.Where(l1 => l2s.OrderBy(l2i => l2i.Id).First().OneToOne_Required_FK_Inverse.Name == "L1 02"));
}

////[ConditionalFact] issue #7761
public virtual void GroupJoin_with_complex_subquery_with_joins_does_not_get_flattened()
{
using (var ctx = CreateContext())
{
var query = from l1_outer in ctx.LevelOne
join subquery in
(
from l2_inner in ctx.LevelTwo
join l1_inner in ctx.LevelOne on l2_inner.Level1_Required_Id equals l1_inner.Id
select l2_inner
)
on l1_outer.Id equals subquery.Level1_Optional_Id into grouping
from subquery in grouping.DefaultIfEmpty()
select (int?)subquery.Id;

var result = query.ToList();
}
}

[ConditionalFact]
public virtual void GroupJoin_with_complex_subquery_with_joins_with_reference_to_grouping1()
{
AssertQueryScalar<Level1, Level2, int>(
(l1s, l2s) =>
from l1_outer in l1s
join subquery in
(
from l2_inner in l2s
join l1_inner in l1s on l2_inner.Level1_Required_Id equals l1_inner.Id
select l2_inner
)
on l1_outer.Id equals subquery.Level1_Optional_Id into grouping
where grouping.Any()
from subquery in grouping.DefaultIfEmpty()
select subquery.Id);
}

[ConditionalFact]
public virtual void GroupJoin_with_complex_subquery_with_joins_with_reference_to_grouping2()
{
AssertQueryScalar<Level1, Level2, int>(
(l1s, l2s) =>
from l1_outer in l1s
join subquery in
(
from l2_inner in l2s
join l1_inner in l1s on l2_inner.Level1_Required_Id equals l1_inner.Id
select l2_inner
)
on l1_outer.Id equals subquery.Level1_Optional_Id into grouping
from subquery in grouping.DefaultIfEmpty()
where grouping.Any()
select subquery.Id);
}

////[ConditionalFact] issue #7770
public virtual void GroupJoin_on_a_subquery_containing_another_GroupJoin_projecting_outer()
{
using (var ctx = CreateContext())
{
var query = from x in
(from l1 in ctx.LevelOne
join l2 in ctx.LevelTwo on l1.Id equals l2.Level1_Optional_Id into grouping
from l2 in grouping.DefaultIfEmpty()
select l1).Take(2)
join l2_outer in ctx.LevelTwo on x.Id equals l2_outer.Level1_Optional_Id into grouping_outer
from l2_outer in grouping_outer.DefaultIfEmpty()
select l2_outer.Name;

var result = query.ToList();
}
}

////[ConditionalFact] issue #7770
public virtual void GroupJoin_on_a_subquery_containing_another_GroupJoin_projecting_outer_with_client_method()
{
using (var ctx = CreateContext())
{
var query = from x in
(from l1 in ctx.LevelOne
join l2 in ctx.LevelTwo on l1.Id equals l2.Level1_Optional_Id into grouping
from l2 in grouping.DefaultIfEmpty()
select ClientLevel1(l1)).Take(2)
join l2_outer in ctx.LevelTwo on x.Id equals l2_outer.Level1_Optional_Id into grouping_outer
from l2_outer in grouping_outer.DefaultIfEmpty()
select l2_outer.Name;

var result = query.ToList();
}
}

private Level1 ClientLevel1(Level1 arg)
{
return arg;
}

////[ConditionalFact] issue #7770
public virtual void GroupJoin_on_a_subquery_containing_another_GroupJoin_projecting_inner()
{
using (var ctx = CreateContext())
{
var query = from x in
(from l1 in ctx.LevelOne
join l2 in ctx.LevelTwo on l1.Id equals l2.Level1_Optional_Id into grouping
from l2 in grouping.DefaultIfEmpty()
select l2).Take(2)
join l1_outer in ctx.LevelOne on x.Level1_Optional_Id equals l1_outer.Id into grouping_outer
from l1_outer in grouping_outer.DefaultIfEmpty()
select l1_outer.Name;

var result = query.ToList();
}
}

[ConditionalFact]
public virtual void GroupJoin_on_left_side_being_a_subquery()
{
Expand All @@ -3140,13 +3291,13 @@ public virtual void GroupJoin_on_right_side_being_a_subquery()
from l2 in l2s
join l1 in l1s.OrderBy(x => x.OneToOne_Optional_FK.Name).Take(2) on l2.Level1_Optional_Id equals l1.Id into grouping
from l1 in grouping.DefaultIfEmpty()
select new { Id = l2.Id, Nane = l1 != null ? l1.Name : null },
select new { Id = l2.Id, Name = l1 != null ? l1.Name : null },
(l1s, l2s) =>
from l2 in l2s
join l1 in l1s.OrderBy(x => Maybe(x.OneToOne_Optional_FK, () => x.OneToOne_Optional_FK.Name)).Take(2)
on l2.Level1_Optional_Id equals l1.Id into grouping
from l1 in grouping.DefaultIfEmpty()
select new { Id = l2.Id, Nane = l1 != null ? l1.Name : null },
select new { Id = l2.Id, Name = l1 != null ? l1.Name : null },
e => e.Id);
}

Expand Down
32 changes: 32 additions & 0 deletions src/EFCore.Specification.Tests/GearsOfWarQueryTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2171,6 +2171,38 @@ public virtual void Count_with_optional_navigation_is_translated_to_sql()
}
}

[ConditionalFact]
public virtual void Distinct_with_unflattened_groupjoin_is_evaluated_on_client()
{
using (var context = CreateContext())
{
var query = context.Gears.GroupJoin(
context.Tags,
g => new { k1 = g.Nickname, k2 = (int?)g.SquadId },
t => new { k1 = t.GearNickName, k2 = t.GearSquadId },
(g, t) => g.HasSoulPatch).Distinct();

var result = query.ToList();

Assert.Equal(5, result.Count);
}
}

////[ConditionalFact] issue #7497
public virtual void Count_with_unflattened_groupjoin_is_evaluated_on_client()
{
using (var context = CreateContext())
{
var query = context.Gears.GroupJoin(
context.Tags,
g => new { k1 = g.Nickname, k2 = (int?)g.SquadId },
t => new { k1 = t.GearNickName, k2 = t.GearSquadId },
(g, t) => g).Count();

Assert.Equal(5, query);
}
}

[ConditionalFact]
public virtual void FirstOrDefault_with_manually_created_groupjoin_is_translated_to_sql()
{
Expand Down
18 changes: 18 additions & 0 deletions src/EFCore.Specification.Tests/QueryNavigationsTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,24 @@ from subquery in result.DefaultIfEmpty()
entryCount: 91);
}

[ConditionalFact]
public virtual void GroupJoin_with_complex_subquery_and_LOJ_does_not_get_flattened2()
{
AssertQuery<Customer, Order, OrderDetail, string>(
(cs, os, ods) => (from c in cs
join subquery in
(
from od in ods
join o in os on od.OrderID equals 10260
join c2 in cs on o.CustomerID equals c2.CustomerID
select c2
)
on c.CustomerID equals subquery.CustomerID
into result
from subquery in result.DefaultIfEmpty()
select c.CustomerID));
}

protected QueryNavigationsTestBase(TFixture fixture)
{
Fixture = fixture;
Expand Down
2 changes: 2 additions & 0 deletions src/EFCore/Query/EntityQueryModelVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,8 @@ var entityEqualityRewritingExpressionVisitor
_navigationRewritingExpressionVisitorFactory
.Create(this).Rewrite(queryModel, parentQueryModel: null);

entityEqualityRewritingExpressionVisitor.Rewrite(queryModel);

QueryCompilationContext.Logger
.LogDebug(
CoreEventId.OptimizedQueryModel,
Expand Down
Loading

0 comments on commit 42d6f97

Please sign in to comment.