Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

InMemory support for DefaultIfEmpty #17278

Closed
wants to merge 1 commit into from
Closed

InMemory support for DefaultIfEmpty #17278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,19 @@ private Expression BindProperty(Expression source, string propertyName, Type typ
}

var result = BindProperty(entityProjection, entityType.FindProperty(propertyName));

// If the entity projection is nullable (e.g. because of DefaultIfEmpty) but the member is a non-nullable value type,
// we coalesce to its default value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be right thing to do.
There is a lot of overlap in this area between work we all are doing, (mainly @maumar is tackling it).

Due to null propagation needs to introduced by EF core on nav expansion and depending on translation,
Translation of certain expression can return null. (mainly for unmapped property. Also see #17050), That means every expression composition when visiting children needs to take care of encountering null and cascade null forward. I made some changes in the one of the PR I merged.
Translation of certain non-null expression can return nullable expression back. This is due to the fact that property may be non-nullable but it may end up being null due to optional navigation path. In such cases we should be rewriting the expression tree to make things nullable. Coalesce may be ok in some cases but may not represent exact match.
e.g. (a => a.OptionalNav.Counter == 0), if you use coalesce it gives true back. But is counter really 0?

if (IsNullableValueType(result.Type) && !IsNullableValueType(type))
{
result = Expression.Coalesce(result, Expression.Default(result.Type.UnwrapNullableType()));
}

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

static bool IsNullableValueType(Type t) => t.IsGenericType && t.GetGenericTypeDefinition() == typeof(Nullable<>);
}

throw new InvalidOperationException(CoreStrings.TranslationFailed(source.Print()));
Expand Down Expand Up @@ -315,24 +325,37 @@ private static T GetParameterValue<T>(QueryContext queryContext, string paramete
protected override Expression VisitUnary(UnaryExpression unaryExpression)
{
var result = base.VisitUnary(unaryExpression);

if (result is UnaryExpression outerUnary
&& outerUnary.NodeType == ExpressionType.Convert
&& outerUnary.Operand is UnaryExpression innerUnary
&& innerUnary.NodeType == ExpressionType.Convert)
&& outerUnary.NodeType == ExpressionType.Convert)
{
var innerMostType = innerUnary.Operand.Type;
var intermediateType = innerUnary.Type;
var outerMostType = outerUnary.Type;

if (outerMostType == innerMostType
&& intermediateType == innerMostType.UnwrapNullableType())
if (outerUnary.Operand is UnaryExpression innerUnary
&& innerUnary.NodeType == ExpressionType.Convert)
{
result = innerUnary.Operand;
var innerMostType = innerUnary.Operand.Type;
var intermediateType = innerUnary.Type;

if (outerMostType == innerMostType
&& intermediateType == innerMostType.UnwrapNullableType())
{
return innerUnary.Operand;
}
}
else if (outerMostType == typeof(object)
&& intermediateType == innerMostType.UnwrapNullableType())

// VisitMember introduces a coalesce to default for value-type members being accessed on nullable entities.
// However, if the member access was immediately wrapped by a Convert to object, we don't want the coalesce.
// See test GroupJoin_DefaultIfEmpty_Project
if (outerMostType == typeof(object)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part I mentioned above, feels problematic.

&& outerUnary.Operand is BinaryExpression binary
&& binary.NodeType == ExpressionType.Coalesce
&& binary.Right is DefaultExpression
// Only discard the coalesce if it was introduced when translating MemberExpression
// (user may have explicitly introduced it)
&& unaryExpression.Operand is MemberExpression)
{
result = Expression.Convert(innerUnary.Operand, typeof(object));
return Expression.Convert(binary.Left, typeof(object));
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/EFCore.InMemory/Query/Internal/InMemoryLinqOperatorProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ private static IEnumerable<MethodInfo> GetMethods(string name, int parameterCoun

public static MethodInfo Join = GetMethod(nameof(Enumerable.Join), 4);
public static MethodInfo GroupJoin = GetMethod(nameof(Enumerable.GroupJoin), 4);
public static MethodInfo DefaultIfEmptyWithArg = GetMethod(nameof(Enumerable.DefaultIfEmpty), 1);
public static MethodInfo DefaultIfEmptyWithoutArgument = GetMethod(nameof(Enumerable.DefaultIfEmpty));
public static MethodInfo DefaultIfEmptyWithArgument = GetMethod(nameof(Enumerable.DefaultIfEmpty), 1);
public static MethodInfo SelectMany = GetMethods(nameof(Enumerable.SelectMany), 2)
.Single(mi => mi.GetParameters()[1].ParameterType.GetGenericArguments().Length == 2);
public static MethodInfo Contains = GetMethod(nameof(Enumerable.Contains), 1);
Expand Down
1 change: 0 additions & 1 deletion src/EFCore.InMemory/Query/Internal/InMemoryProjectionBindingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ public override Expression Visit(Expression expression)

return new ProjectionBindingExpression(_queryExpression, _projectionMembers.Peek(), expression.Type);
}

}

return base.Visit(expression);
Expand Down
35 changes: 34 additions & 1 deletion src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,39 @@ private Expression CreateReadValueExpression(
Constant(index),
Constant(property, typeof(IPropertyBase)));

public virtual void ApplyDefaultIfEmpty()
{
var nullableReadValueExpressionVisitor = new NullableReadValueExpressionVisitor();
var projectionMapping = new Dictionary<ProjectionMember, Expression>();
var index = 0;
foreach (var projection in _projectionMapping)
{
if (projection.Value is EntityProjectionExpression entityProjection)
{
var readExpressionMap = GetAllPropertiesInHierarchy(entityProjection.EntityType)
.ToDictionary(
p => p,
p => CreateReadValueExpression(
nullableReadValueExpressionVisitor.Visit(entityProjection.BindProperty(p)).Type, index++, p));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is somewhat overkill. You are just using type. You can pass nullable type directly. Also it is coming from EntityProjection so it would be readValueMethod. NullableReadValueEV is for complex expression which may have nested ReadValue calls which needs to be converted to null. Further, for DefaultIfEmpty. you just need to update current projection Mapping rather than change anything going inside so you can just create nullable calls.


projectionMapping[projection.Key] = new EntityProjectionExpression(entityProjection.EntityType, readExpressionMap);
}
else
{
projectionMapping[projection.Key] = CreateReadValueExpression(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also incorrect.
This assumes that projection would be a CreateReadValue only. If it is CreateReadValue(0) + CreateReadValue(1) then you never added it to projection but then you decided to read 0 slot from inner.
The implementation should apply a Selector on ServerQuery before DefaultIfEmpty then based on selector applied pass an array of nulls containing ValueBuffer and reconstruct projection mapping like how it is happening here.

nullableReadValueExpressionVisitor.Visit(projection.Value).Type, index++, InferPropertyFromInner(projection.Value));
}
}

_projectionMapping = projectionMapping;

// Rewrite 0-arg DefaultOrEmpty to DefaultOrEmpty to a ValueBuffer containing all nulls
ServerQueryExpression = Call(
InMemoryLinqOperatorProvider.DefaultIfEmptyWithArgument.MakeGenericMethod(typeof(ValueBuffer)),
ServerQueryExpression,
New(_valueBufferConstructor, NewArrayInit(typeof(object), Enumerable.Repeat(Constant(null), index))));
}

public virtual void AddInnerJoin(
InMemoryQueryExpression innerQueryExpression,
LambdaExpression outerKeySelector,
Expand Down Expand Up @@ -486,7 +519,7 @@ public virtual void AddLeftJoin(

var collectionSelector = Lambda(
Call(
InMemoryLinqOperatorProvider.DefaultIfEmptyWithArg.MakeGenericMethod(typeof(ValueBuffer)),
InMemoryLinqOperatorProvider.DefaultIfEmptyWithArgument.MakeGenericMethod(typeof(ValueBuffer)),
collection,
New(
_valueBufferConstructor,
Expand Down
18 changes: 13 additions & 5 deletions src/EFCore.InMemory/Query/Internal/InMemoryQueryableMethodTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,18 @@ protected override ShapedQueryExpression TranslateCount(ShapedQueryExpression so
return source;
}

protected override ShapedQueryExpression TranslateDefaultIfEmpty(ShapedQueryExpression source, Expression defaultValue) => throw new NotImplementedException();
protected override ShapedQueryExpression TranslateDefaultIfEmpty(ShapedQueryExpression source, Expression defaultValue)
{
if (defaultValue == null)
{
((InMemoryQueryExpression)source.QueryExpression).ApplyDefaultIfEmpty();
source.ShaperExpression = MarkShaperNullable(source.ShaperExpression);

return source;
}

throw new InvalidOperationException(CoreStrings.TranslationFailed(defaultValue.Print()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returns null.

}

protected override ShapedQueryExpression TranslateDistinct(ShapedQueryExpression source)
{
Expand Down Expand Up @@ -319,9 +330,6 @@ protected override ShapedQueryExpression TranslateSelect(ShapedQueryExpression s
return source;
}

private static readonly MethodInfo _defaultIfEmptyWithoutArgMethodInfo = typeof(Enumerable).GetTypeInfo()
.GetDeclaredMethods(nameof(Enumerable.DefaultIfEmpty)).Single(mi => mi.GetParameters().Length == 1);

protected override ShapedQueryExpression TranslateSelectMany(
ShapedQueryExpression source, LambdaExpression collectionSelector, LambdaExpression resultSelector)
{
Expand All @@ -330,7 +338,7 @@ protected override ShapedQueryExpression TranslateSelectMany(

if (collectionSelectorBody is MethodCallExpression collectionEndingMethod
&& collectionEndingMethod.Method.IsGenericMethod
&& collectionEndingMethod.Method.GetGenericMethodDefinition() == _defaultIfEmptyWithoutArgMethodInfo)
&& collectionEndingMethod.Method.GetGenericMethodDefinition() == InMemoryLinqOperatorProvider.DefaultIfEmptyWithoutArgument)
{
//defaultIfEmpty = true;
collectionSelectorBody = collectionEndingMethod.Arguments[0];
Expand Down
30 changes: 5 additions & 25 deletions test/EFCore.InMemory.FunctionalTests/Query/SimpleQueryInMemoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,35 +150,15 @@ public override void Select_nested_collection_multi_level()
{
}

[ConditionalTheory(Skip = "Issue #16963")]
[ConditionalTheory(Skip = "Issue #16963 (SelectMany)")]
public override Task DefaultIfEmpty_in_subquery(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
[ConditionalTheory(Skip = "Issue #16963 (SelectMany)")]
public override Task DefaultIfEmpty_in_subquery_nested(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
[ConditionalTheory(Skip = "Issue #16963 (SelectMany)")]
public override Task DefaultIfEmpty_in_subquery_not_correlated(bool isAsync) => null;

[ConditionalFact(Skip = "Issue #16963")]
public override void DefaultIfEmpty_without_group_join()
{
}

[ConditionalTheory(Skip = "Issue #16963")]
public override Task Default_if_empty_top_level(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
public override Task Default_if_empty_top_level_followed_by_projecting_constant(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
public override Task Default_if_empty_top_level_positive(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
public override Task Default_if_empty_top_level_projection(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
public override Task Join_with_default_if_empty_on_both_sources(bool isAsync) => null;

[ConditionalFact(Skip = "Issue #16963")]
public override void OfType_Select()
{
Expand Down Expand Up @@ -228,10 +208,10 @@ public override void Client_code_using_instance_in_anonymous_type()
[ConditionalTheory(Skip = "Issue #16963")]
public override Task SelectMany_Joined(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
[ConditionalTheory(Skip = "Issue #16963 (SelectMany)")]
public override Task SelectMany_Joined_DefaultIfEmpty(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
[ConditionalTheory(Skip = "Issue #16963 (SelectMany)")]
public override Task SelectMany_Joined_DefaultIfEmpty2(bool isAsync) => null;

[ConditionalTheory(Skip = "Issue #16963")]
Expand Down