Skip to content

Commit

Permalink
InMemory: Improve indexing in server expression
Browse files Browse the repository at this point in the history
Implement left join as client method to reduce complexity
To resolve the indexing issue stemming from #23934
Now all the projections are applied immediately to reshape the value buffer so that all our future bindings are always read value expressions which refers to a proper index.
In order to do this, we apply select for entity type with hierarchy to avoid entity check conditional expression.
For adding projection (through ReplaceProjectionMapping),
- For client projection, we apply a select and re-generate client projection as read values
- For projection mapping, we iterate the mappings, we apply a select and re-generate the mapping as read values
- If projection mapping is empty then we add a dummy 1 so that it becomes non-range-variable
When applying projection, we generate a selector lambda to form a value buffer and replace all the expressions to read from new value buffer. Overall this solves the issue of having complex expressions to map or pull. This also removed PushDownIntoSubquery method.

In order to avoid the issue of indexes changing when generating join due to iterating projection mappings, we now also have projectionMappingExpressions which remembers the all expressions inside projectionMapping (which are all read value as we generated before). So now we don't need to iterate the mapping and we use the existing expressions directly. This keeps existing indexes.

Resolves #13561
Resolves #17539
Resolves #18194
Resolves #18435
Resolves #19344
Resolves #19469
Resolves #19667
Resolves #19742
Resolves #19967
Resolves #20359
Resolves #21677
Resolves #23360
Resolves #17537
Resolves #18394
Resolves #23934

# Conflicts:
#	test/EFCore.InMemory.FunctionalTests/Query/NorthwindGroupByQueryInMemoryTest.cs
  • Loading branch information
smitpatel committed Mar 10, 2021
1 parent b00d33b commit 9a4dcb4
Show file tree
Hide file tree
Showing 16 changed files with 1,225 additions and 917 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace Microsoft.EntityFrameworkCore.InMemory.Query.Internal
/// </summary>
public class EntityProjectionExpression : Expression, IPrintableExpression
{
private readonly IDictionary<IProperty, Expression> _readExpressionMap;
private readonly IDictionary<IProperty, MethodCallExpression> _readExpressionMap;

private readonly IDictionary<INavigation, EntityShaperExpression> _navigationExpressionsCache
= new Dictionary<INavigation, EntityShaperExpression>();
Expand All @@ -36,7 +36,7 @@ private readonly IDictionary<INavigation, EntityShaperExpression> _navigationExp
/// </summary>
public EntityProjectionExpression(
[NotNull] IEntityType entityType,
[NotNull] IDictionary<IProperty, Expression> readExpressionMap)
[NotNull] IDictionary<IProperty, MethodCallExpression> readExpressionMap)
{
EntityType = entityType;
_readExpressionMap = readExpressionMap;
Expand Down Expand Up @@ -83,7 +83,7 @@ public virtual EntityProjectionExpression UpdateEntityType([NotNull] IEntityType
derivedType.DisplayName(), EntityType.DisplayName()));
}

var readExpressionMap = new Dictionary<IProperty, Expression>();
var readExpressionMap = new Dictionary<IProperty, MethodCallExpression>();
foreach (var kvp in _readExpressionMap)
{
var property = kvp.Key;
Expand All @@ -103,7 +103,7 @@ public virtual EntityProjectionExpression UpdateEntityType([NotNull] IEntityType
/// 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>
public virtual Expression BindProperty([NotNull] IProperty property)
public virtual MethodCallExpression BindProperty([NotNull] IProperty property)
{
if (!EntityType.IsAssignableFrom(property.DeclaringEntityType)
&& !property.DeclaringEntityType.IsAssignableFrom(EntityType))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1185,15 +1185,12 @@ protected override Expression VisitUnary(UnaryExpression unaryExpression)

// if the result type change was just nullability change e.g from int to int?
// we want to preserve the new type for null propagation
if (result.Type != type
return result.Type != type
&& !(result.Type.IsNullableType()
&& !type.IsNullableType()
&& result.Type.UnwrapNullableType() == type))
{
result = Expression.Convert(result, type);
}

return result;
&& result.Type.UnwrapNullableType() == type)
? Expression.Convert(result, type)
: (Expression)result;
}

if (entityReferenceExpression.SubqueryEntity != null)
Expand Down
1,273 changes: 531 additions & 742 deletions src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ private static ShapedQueryExpression CreateShapedQueryExpressionStatic(IEntityTy
if (source.ShaperExpression is GroupByShaperExpression)
{
inMemoryQueryExpression.ReplaceProjectionMapping(new Dictionary<ProjectionMember, Expression>());
inMemoryQueryExpression.PushdownIntoSubquery();
}

inMemoryQueryExpression.UpdateServerQueryExpression(
Expand Down Expand Up @@ -191,7 +190,6 @@ private static ShapedQueryExpression CreateShapedQueryExpressionStatic(IEntityTy
if (source.ShaperExpression is GroupByShaperExpression)
{
inMemoryQueryExpression.ReplaceProjectionMapping(new Dictionary<ProjectionMember, Expression>());
inMemoryQueryExpression.PushdownIntoSubquery();
}

inMemoryQueryExpression.UpdateServerQueryExpression(
Expand Down Expand Up @@ -304,7 +302,6 @@ private static ShapedQueryExpression CreateShapedQueryExpressionStatic(IEntityTy
if (source.ShaperExpression is GroupByShaperExpression)
{
inMemoryQueryExpression.ReplaceProjectionMapping(new Dictionary<ProjectionMember, Expression>());
inMemoryQueryExpression.PushdownIntoSubquery();
}

inMemoryQueryExpression.UpdateServerQueryExpression(
Expand Down Expand Up @@ -346,7 +343,6 @@ private static ShapedQueryExpression CreateShapedQueryExpressionStatic(IEntityTy

var inMemoryQueryExpression = (InMemoryQueryExpression)source.QueryExpression;

inMemoryQueryExpression.PushdownIntoSubquery();
inMemoryQueryExpression.UpdateServerQueryExpression(
Expression.Call(
EnumerableMethods.Distinct.MakeGenericMethod(inMemoryQueryExpression.CurrentParameter.Type),
Expand Down Expand Up @@ -430,13 +426,14 @@ private static ShapedQueryExpression CreateShapedQueryExpressionStatic(IEntityTy
var translatedKey = TranslateGroupingKey(remappedKeySelector);
if (translatedKey != null)
{
if (elementSelector != null)
var inMemoryQueryExpression = (InMemoryQueryExpression)source.QueryExpression;
var defaultElementSelector = elementSelector == null || elementSelector.Body == elementSelector.Parameters[0];
if (!defaultElementSelector)
{
source = TranslateSelect(source, elementSelector);
source = TranslateSelect(source, elementSelector!);
}

var inMemoryQueryExpression = (InMemoryQueryExpression)source.QueryExpression;
var groupByShaper = inMemoryQueryExpression.ApplyGrouping(translatedKey, source.ShaperExpression);
var groupByShaper = inMemoryQueryExpression.ApplyGrouping(translatedKey, source.ShaperExpression, defaultElementSelector);

if (resultSelector == null)
{
Expand All @@ -452,7 +449,6 @@ private static ShapedQueryExpression CreateShapedQueryExpressionStatic(IEntityTy

newResultSelectorBody = ExpandWeakEntities(inMemoryQueryExpression, newResultSelectorBody);
var newShaper = _projectionBindingExpressionVisitor.Translate(inMemoryQueryExpression, newResultSelectorBody);
inMemoryQueryExpression.PushdownIntoSubquery();

return source.UpdateShaperExpression(newShaper);
}
Expand Down Expand Up @@ -806,7 +802,6 @@ static bool IsConvertedToNullable(Expression outer, Expression inner)
if (source.ShaperExpression is GroupByShaperExpression)
{
inMemoryQueryExpression.ReplaceProjectionMapping(new Dictionary<ProjectionMember, Expression>());
inMemoryQueryExpression.PushdownIntoSubquery();
}

inMemoryQueryExpression.UpdateServerQueryExpression(
Expand Down Expand Up @@ -974,14 +969,9 @@ protected override ShapedQueryExpression TranslateSelect(ShapedQueryExpression s
var newSelectorBody = ReplacingExpressionVisitor.Replace(
selector.Parameters.Single(), source.ShaperExpression, selector.Body);

var groupByQuery = source.ShaperExpression is GroupByShaperExpression;
var queryExpression = (InMemoryQueryExpression)source.QueryExpression;

var newShaper = _projectionBindingExpressionVisitor.Translate(queryExpression, newSelectorBody);
if (groupByQuery)
{
queryExpression.PushdownIntoSubquery();
}

return source.UpdateShaperExpression(newShaper);
}
Expand Down Expand Up @@ -1384,7 +1374,7 @@ protected override Expression VisitExtension(Expression extensionExpression)
private Expression? TryExpand(Expression? source, MemberIdentity member)
{
source = source.UnwrapTypeConversion(out var convertedType);
if (!(source is EntityShaperExpression entityShaperExpression))
if (source is not EntityShaperExpression entityShaperExpression)
{
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,35 +14,5 @@ public ComplexNavigationsQueryInMemoryTest(ComplexNavigationsQueryInMemoryFixtur
{
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalFact(Skip = "issue #18194")]
public override void Member_pushdown_chain_3_levels_deep_entity()
{
base.Member_pushdown_chain_3_levels_deep_entity();
}

[ConditionalTheory(Skip = "issue #17620")]
public override Task Lift_projection_mapping_when_pushing_down_subquery(bool async)
{
return base.Lift_projection_mapping_when_pushing_down_subquery(async);
}

[ConditionalTheory(Skip = "issue #19344")]
public override Task Select_subquery_single_nested_subquery(bool async)
{
return base.Select_subquery_single_nested_subquery(async);
}

[ConditionalTheory(Skip = "issue #19344")]
public override Task Select_subquery_single_nested_subquery2(bool async)
{
return base.Select_subquery_single_nested_subquery2(async);
}

[ConditionalTheory(Skip = "issue #17539")]
public override Task Union_over_entities_with_different_nullability(bool async)
{
return base.Union_over_entities_with_different_nullability(async);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,83 +18,5 @@ public ComplexNavigationsSharedTypeQueryInMemoryTest(
{
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

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

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

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

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

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

[ConditionalTheory(Skip = "issue #17620")]
public override Task Lift_projection_mapping_when_pushing_down_subquery(bool async)
{
return base.Lift_projection_mapping_when_pushing_down_subquery(async);
}

[ConditionalTheory(Skip = "issue #18912")]
public override Task OrderBy_collection_count_ThenBy_reference_navigation(bool async)
{
return base.OrderBy_collection_count_ThenBy_reference_navigation(async);
}

[ConditionalTheory(Skip = "issue #19344")]
public override Task Select_subquery_single_nested_subquery(bool async)
{
return base.Select_subquery_single_nested_subquery(async);
}

[ConditionalTheory(Skip = "issue #19344")]
public override Task Select_subquery_single_nested_subquery2(bool async)
{
return base.Select_subquery_single_nested_subquery2(async);
}

[ConditionalTheory(Skip = "issue #19967")]
public override Task SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(bool async)
{
return base.SelectMany_with_outside_reference_to_joined_table_correctly_translated_to_apply(async);
}

[ConditionalTheory(Skip = "issue #19967")]
public override Task Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(bool async)
{
return base.Nested_SelectMany_correlated_with_join_table_correctly_translated_to_apply(async);
}

[ConditionalTheory(Skip = "issue #19742")]
public override Task Contains_over_optional_navigation_with_null_column(bool async)
{
return base.Contains_over_optional_navigation_with_null_column(async);
}

[ConditionalTheory(Skip = "issue #19742")]
public override Task Contains_over_optional_navigation_with_null_entity_reference(bool async)
{
return base.Contains_over_optional_navigation_with_null_entity_reference(async);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,32 +23,16 @@ public override Task Client_member_and_unsupported_string_Equals_in_the_same_que
CoreStrings.QueryUnableToTranslateMember(nameof(Gear.IsMarcus), nameof(Gear)));
}

[ConditionalFact(Skip = "issue #17537")]
public override void Include_on_GroupJoin_SelectMany_DefaultIfEmpty_with_coalesce_result1()
=> base.Include_on_GroupJoin_SelectMany_DefaultIfEmpty_with_coalesce_result1();

[ConditionalFact(Skip = "issue #17537")]
public override void Include_on_GroupJoin_SelectMany_DefaultIfEmpty_with_coalesce_result2()
=> base.Include_on_GroupJoin_SelectMany_DefaultIfEmpty_with_coalesce_result2();

[ConditionalTheory(Skip = "issue #17540")]
public override Task
Null_semantics_is_correctly_applied_for_function_comparisons_that_take_arguments_from_optional_navigation_complex(bool async)
=> base.Null_semantics_is_correctly_applied_for_function_comparisons_that_take_arguments_from_optional_navigation_complex(
async);

[ConditionalTheory(Skip = "issue #17620")]
public override Task Select_subquery_projecting_single_constant_inside_anonymous(bool async)
=> base.Select_subquery_projecting_single_constant_inside_anonymous(async);

[ConditionalTheory(Skip = "issue #19683")]
public override Task Group_by_on_StartsWith_with_null_parameter_as_argument(bool async)
=> base.Group_by_on_StartsWith_with_null_parameter_as_argument(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);

[ConditionalTheory(Skip = "issue #19584")]
public override Task Cast_to_derived_followed_by_include_and_FirstOrDefault(bool async)
=> base.Cast_to_derived_followed_by_include_and_FirstOrDefault(async);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,6 @@ public NorthwindGroupByQueryInMemoryTest(
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

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

[ConditionalTheory(Skip = "Issue#24324")]
public override Task Complex_query_with_groupBy_in_subquery4(bool async)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,6 @@ public OwnedQueryInMemoryTest(OwnedQueryInMemoryFixture fixture, ITestOutputHelp
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalTheory(Skip = "issue #19742")]
public override Task Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(bool async)
{
return base.Projecting_collection_correlated_with_keyless_entity_after_navigation_works_using_parent_identifiers(async);
}

public class OwnedQueryInMemoryFixture : OwnedQueryFixtureBase
{
protected override ITestStoreFactory TestStoreFactory
Expand Down
Loading

0 comments on commit 9a4dcb4

Please sign in to comment.