Skip to content

Commit

Permalink
Fix to #18871 - Support for indirect joins which are using non-equal …
Browse files Browse the repository at this point in the history 
…conditions

When validating join key expression we now allow non-equality binary ops.

Fixes #18871
  • Loading branch information
@maumar
maumar committed Jul 2, 2020
1 parent e327d2c commit 7a2c37e
Show file tree
Hide file tree
Showing 28 changed files with 617 additions and 19 deletions.
49 changes: 39 additions & 10 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ namespace Microsoft.EntityFrameworkCore.Query.SqlExpressions
// Class is sealed because there are no public/protected constructors. Can be unsealed if this is changed.
public sealed class SelectExpression : TableExpressionBase
{
private static readonly Dictionary<ExpressionType, ExpressionType> _mirroredOperationMap = new Dictionary<ExpressionType, ExpressionType>
{
{ ExpressionType.Equal, ExpressionType.Equal },
{ ExpressionType.NotEqual, ExpressionType.NotEqual },
{ ExpressionType.LessThan, ExpressionType.GreaterThan },
{ ExpressionType.LessThanOrEqual, ExpressionType.GreaterThanOrEqual },
{ ExpressionType.GreaterThan, ExpressionType.LessThan },
{ ExpressionType.GreaterThanOrEqual, ExpressionType.LessThanOrEqual },
};

private readonly IDictionary<EntityProjectionExpression, IDictionary<IProperty, int>> _entityProjectionCache
= new Dictionary<EntityProjectionExpression, IDictionary<IProperty, int>>();

Expand Down Expand Up @@ -1371,14 +1381,20 @@ private static SqlExpression MakeNullable(SqlExpression sqlExpression)
return (NewArrayInit(typeof(object), updatedExpressions), comparers);
}

private SqlExpression TryExtractJoinKey(SelectExpression selectExpression)
private SqlExpression TryExtractJoinKey(SelectExpression selectExpression, bool allowNonEquality)
{
if (selectExpression.Limit == null
&& selectExpression.Offset == null
&& selectExpression.Predicate != null)
{
var columnExpressions = new List<ColumnExpression>();
var joinPredicate = TryExtractJoinKey(selectExpression, selectExpression.Predicate, columnExpressions, out var predicate);
var joinPredicate = TryExtractJoinKey(
selectExpression,
selectExpression.Predicate,
columnExpressions,
allowNonEquality,
out var predicate);

if (joinPredicate != null)
{
joinPredicate = RemoveRedundantNullChecks(joinPredicate, columnExpressions);
Expand All @@ -1396,11 +1412,12 @@ private SqlExpression TryExtractJoinKey(
SelectExpression selectExpression,
SqlExpression predicate,
List<ColumnExpression> columnExpressions,
bool allowNonEquality,
out SqlExpression updatedPredicate)
{
if (predicate is SqlBinaryExpression sqlBinaryExpression)
{
var joinPredicate = ValidateKeyComparison(selectExpression, sqlBinaryExpression, columnExpressions);
var joinPredicate = ValidateKeyComparison(selectExpression, sqlBinaryExpression, columnExpressions, allowNonEquality);
if (joinPredicate != null)
{
updatedPredicate = null;
Expand All @@ -1411,9 +1428,9 @@ private SqlExpression TryExtractJoinKey(
if (sqlBinaryExpression.OperatorType == ExpressionType.AndAlso)
{
var leftJoinKey = TryExtractJoinKey(
selectExpression, sqlBinaryExpression.Left, columnExpressions, out var leftPredicate);
selectExpression, sqlBinaryExpression.Left, columnExpressions, allowNonEquality, out var leftPredicate);
var rightJoinKey = TryExtractJoinKey(
selectExpression, sqlBinaryExpression.Right, columnExpressions, out var rightPredicate);
selectExpression, sqlBinaryExpression.Right, columnExpressions, allowNonEquality, out var rightPredicate);

updatedPredicate = CombineNonNullExpressions(leftPredicate, rightPredicate);

Expand All @@ -1434,9 +1451,18 @@ private static SqlExpression CombineNonNullExpressions(SqlExpression left, SqlEx
: right;

private SqlBinaryExpression ValidateKeyComparison(
SelectExpression inner, SqlBinaryExpression sqlBinaryExpression, List<ColumnExpression> columnExpressions)
SelectExpression inner,
SqlBinaryExpression sqlBinaryExpression,
List<ColumnExpression> columnExpressions,
bool allowNonEquality)
{
if (sqlBinaryExpression.OperatorType == ExpressionType.Equal)
if (sqlBinaryExpression.OperatorType == ExpressionType.Equal
|| (allowNonEquality &&
(sqlBinaryExpression.OperatorType == ExpressionType.NotEqual
|| sqlBinaryExpression.OperatorType == ExpressionType.GreaterThan
|| sqlBinaryExpression.OperatorType == ExpressionType.GreaterThanOrEqual
|| sqlBinaryExpression.OperatorType == ExpressionType.LessThan
|| sqlBinaryExpression.OperatorType == ExpressionType.LessThanOrEqual)))
{
if (sqlBinaryExpression.Left is ColumnExpression leftColumn
&& sqlBinaryExpression.Right is ColumnExpression rightColumn)
Expand All @@ -1454,9 +1480,12 @@ private SqlBinaryExpression ValidateKeyComparison(
{
columnExpressions.Add(rightColumn);

return sqlBinaryExpression.Update(
return new SqlBinaryExpression(
_mirroredOperationMap[sqlBinaryExpression.OperatorType],
sqlBinaryExpression.Right,
sqlBinaryExpression.Left);
sqlBinaryExpression.Left,
sqlBinaryExpression.Type,
sqlBinaryExpression.TypeMapping);
}
}
}
Expand Down Expand Up @@ -1829,7 +1858,7 @@ private void AddJoin(
innerSelectExpression.Limit = null;
innerSelectExpression.Offset = null;

joinPredicate = TryExtractJoinKey(innerSelectExpression);
joinPredicate = TryExtractJoinKey(innerSelectExpression, allowNonEquality: limit == null && offset == null);
if (joinPredicate != null)
{
var containsOuterReference = new SelectExpressionCorrelationFindingExpressionVisitor(this)
Expand Down
7 changes: 6 additions & 1 deletion src/EFCore.Relational/Query/SqlNullabilityProcessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1020,7 +1020,12 @@ private SqlExpression ProcessJoinPredicate(SqlExpression predicate)
return result;
}

if (sqlBinaryExpression.OperatorType == ExpressionType.AndAlso)
if (sqlBinaryExpression.OperatorType == ExpressionType.AndAlso
|| sqlBinaryExpression.OperatorType == ExpressionType.NotEqual
|| sqlBinaryExpression.OperatorType == ExpressionType.GreaterThan
|| sqlBinaryExpression.OperatorType == ExpressionType.GreaterThanOrEqual
|| sqlBinaryExpression.OperatorType == ExpressionType.LessThan
|| sqlBinaryExpression.OperatorType == ExpressionType.LessThanOrEqual)
{
return Visit(sqlBinaryExpression, allowOptimizedExpansion: true, out _);
}
Expand Down
6 changes: 6 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2651,6 +2651,12 @@ FROM root c
WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""City""] = ""Seattle""))");
}

[ConditionalTheory(Skip = "Issue#17246")]
public override Task DefaultIfEmpty_in_subquery_nested_filter_order_comparison(bool async)
{
return base.DefaultIfEmpty_in_subquery_nested_filter_order_comparison(async);
}

[ConditionalTheory(Skip = "Issue #17246")]
public override async Task OrderBy_skip_take(bool async)
{
Expand Down
18 changes: 18 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -962,6 +962,24 @@ public override Task SelectMany_correlated_with_outer_4(bool async)
return base.SelectMany_correlated_with_outer_4(async);
}

[ConditionalTheory(Skip = "Issue#17246")]
public override Task SelectMany_correlated_with_outer_5(bool async)
{
return base.SelectMany_correlated_with_outer_5(async);
}

[ConditionalTheory(Skip = "Issue#17246")]
public override Task SelectMany_correlated_with_outer_6(bool async)
{
return base.SelectMany_correlated_with_outer_6(async);
}

[ConditionalTheory(Skip = "Issue#17246")]
public override Task SelectMany_correlated_with_outer_7(bool async)
{
return base.SelectMany_correlated_with_outer_7(async);
}

[ConditionalTheory(Skip = "Issue#17246")]
public override Task FirstOrDefault_over_empty_collection_of_value_type_returns_correct_results(bool async)
{
Expand Down
4 changes: 4 additions & 0 deletions test/EFCore.InMemory.FunctionalTests/Query/GearsOfWarQueryInMemoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,9 @@ public override Task Client_member_and_unsupported_string_Equals_in_the_same_que
[ConditionalTheory(Skip = "issue #17386")]
public override Task Client_eval_followed_by_set_operation_throws_meaningful_exception(bool async)
=> base.Client_eval_followed_by_set_operation_throws_meaningful_exception(async);

[ConditionalTheory(Skip = "issue #17537")]
public override Task SelectMany_predicate_with_non_equality_comparison_with_Take_doesnt_convert_to_join(bool async)
=> base.SelectMany_predicate_with_non_equality_comparison_with_Take_doesnt_convert_to_join(async);
}
}
154 changes: 154 additions & 0 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6353,6 +6353,42 @@ from w in g.Weapons.Where(ww => ww.IsAutomatic == isAutomatic).DefaultIfEmpty()
});
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task SelectMany_Where_DefaultIfEmpty_with_navigation_in_the_collection_selector_not_equal(bool async)
{
var isAutomatic = true;

return AssertQuery(
async,
ss => from g in ss.Set<Gear>()
from w in g.Weapons.Where(ww => ww.IsAutomatic != isAutomatic).DefaultIfEmpty()
select new
{
g.Nickname,
g.FullName,
Collection = w != null
});
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task SelectMany_Where_DefaultIfEmpty_with_navigation_in_the_collection_selector_order_comparison(bool async)
{
var prm = 1;

return AssertQuery(
async,
ss => from g in ss.Set<Gear>()
from w in g.Weapons.Where(ww => ww.Id > prm).DefaultIfEmpty()
select new
{
g.Nickname,
g.FullName,
Collection = w != null
});
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Join_with_inner_being_a_subquery_projecting_single_property(bool async)
Expand Down Expand Up @@ -7614,6 +7650,124 @@ public virtual Task Groupby_anonymous_type_with_navigations_followed_up_by_anony
assertOrder: true);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task SelectMany_predicate_with_non_equality_comparison_converted_to_inner_join(bool async)
{
return AssertQuery(
async,
ss => from g in ss.Set<Gear>()
from w in ss.Set<Weapon>().Where(x => x.OwnerFullName != g.FullName)
orderby g.Nickname, w.Id
select new { g, w },
assertOrder: true,
elementAsserter: (e, a) =>
{
AssertEqual(e.g, a.g);
AssertEqual(e.w, a.w);
});
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task SelectMany_predicate_with_non_equality_comparison_DefaultIfEmpty_converted_to_left_join(bool async)
{
return AssertQuery(
async,
ss => from g in ss.Set<Gear>()
from w in ss.Set<Weapon>().Where(x => x.OwnerFullName != g.FullName).DefaultIfEmpty()
orderby g.Nickname, w.Id
select new { g, w },
assertOrder: true,
elementAsserter: (e, a) =>
{
AssertEqual(e.g, a.g);
AssertEqual(e.w, a.w);
});
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task SelectMany_predicate_after_navigation_with_non_equality_comparison_DefaultIfEmpty_converted_to_left_join(bool async)
{
return AssertQuery(
async,
ss => from g in ss.Set<Gear>()
from w in ss.Set<Weapon>().Select(x => x.SynergyWith).Where(x => x.OwnerFullName != g.FullName).DefaultIfEmpty()
orderby g.Nickname, w.Id
select new { g, w },
ss => from g in ss.Set<Gear>()
from w in ss.Set<Weapon>().Select(x => x.SynergyWith).Where(x => x.OwnerFullName != g.FullName).MaybeDefaultIfEmpty()
orderby g.Nickname, w.MaybeScalar(xx => xx.Id)
select new { g, w },
assertOrder: true,
elementAsserter: (e, a) =>
{
AssertEqual(e.g, a.g);
AssertEqual(e.w, a.w);
});
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task SelectMany_without_result_selector_and_non_equality_comparison_converted_to_join(bool async)
{
return AssertQuery(
async,
ss => ss.Set<Gear>().SelectMany(g => ss.Set<Weapon>().Where(x => x.OwnerFullName != g.FullName).DefaultIfEmpty()));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Filtered_collection_projection_with_order_comparison_predicate_converted_to_join(bool async)
{
return AssertQuery(
async,
ss => ss.Set<Gear>().OrderBy(g => g.Nickname).Select(g => g.Weapons.Where(x => x.Id > g.SquadId).ToList()),
assertOrder: true,
elementAsserter: (e, a) => AssertCollection(e, a, elementSorter: ee => ee.Id));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Filtered_collection_projection_with_order_comparison_predicate_converted_to_join2(bool async)
{
return AssertQuery(
async,
ss => ss.Set<Gear>().OrderBy(g => g.Nickname).Select(g => g.Weapons.Where(x => x.Id >= g.SquadId).ToList()),
assertOrder: true,
elementAsserter: (e, a) => AssertCollection(e, a, elementSorter: ee => ee.Id));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Filtered_collection_projection_with_order_comparison_predicate_converted_to_join3(bool async)
{
return AssertQuery(
async,
ss => ss.Set<Gear>().OrderBy(g => g.Nickname).Select(g => g.Weapons.Where(x => x.Id <= g.SquadId).ToList()),
assertOrder: true,
elementAsserter: (e, a) => AssertCollection(e, a, elementSorter: ee => ee.Id));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task SelectMany_predicate_with_non_equality_comparison_with_Take_doesnt_convert_to_join(bool async)
{
return AssertQuery(
async,
ss => from g in ss.Set<Gear>()
from w in ss.Set<Weapon>().Where(x => x.OwnerFullName != g.FullName).Take(3)
orderby g.Nickname, w.Id
select new { g, w },
assertOrder: true,
elementAsserter: (e, a) =>
{
AssertEqual(e.g, a.g);
AssertEqual(e.w, a.w);
});
}

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

protected virtual void ClearLog()
Expand Down
11 changes: 11 additions & 0 deletions test/EFCore.Specification.Tests/Query/ManyToManyQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,17 @@ public virtual async Task Throws_when_different_filtered_then_include_via_differ
.Replace("\r", "").Replace("\n", ""));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Select_many_over_skip_navigation_where_non_equality(bool async)
{
return AssertQuery(
async,
ss => from r in ss.Set<EntityOne>()
from t in r.TwoSkip.Where(x => x.Id != r.Id).DefaultIfEmpty()
select t);
}

// When adding include test here always add a tracking version and a split version in relational layer.
// Keep this line at the bottom for next dev writing tests to see.

Expand Down
15 changes: 15 additions & 0 deletions test/EFCore.Specification.Tests/Query/NorthwindIncludeQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,21 @@ where c.CustomerID.StartsWith("F")
entryCount: 71);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Include_collection_with_outer_apply_with_filter_non_equality(bool async)
{
return AssertQuery(
async,
ss => from c in ss.Set<Customer>().Include(c => c.Orders)
from o in ss.Set<Order>().Where(o => o.CustomerID != c.CustomerID)
.OrderBy(o => c.CustomerID).Take(5).DefaultIfEmpty()
where c.CustomerID.StartsWith("F")
select c,
elementAsserter: (e, a) => AssertInclude(e, a, new ExpectedInclude<Customer>(c => c.Orders)),
entryCount: 71);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Include_collection_on_join_clause_with_order_by_and_filter(bool async)
Expand Down
Loading

0 comments on commit 7a2c37e

Please sign in to comment.