InMemory support for DefaultIfEmpty #17278
Conversation
| // 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) |
There was a problem hiding this comment.
This is the part I mentioned above, feels problematic.
| .ToDictionary( | ||
| p => p, | ||
| p => CreateReadValueExpression( | ||
| nullableReadValueExpressionVisitor.Visit(entityProjection.BindProperty(p)).Type, index++, p)); |
There was a problem hiding this comment.
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.
| return source; | ||
| } | ||
|
|
||
| throw new InvalidOperationException(CoreStrings.TranslationFailed(defaultValue.Print())); |
| @@ -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. | |||
There was a problem hiding this comment.
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?
Would you prefer me to leave this work for you and @maumar? It may be a better idea if you're working on related things.
I see what you mean. In any case, let me know if you want me to continue thinking about this. |
| } | ||
| else | ||
| { | ||
| projectionMapping[projection.Key] = CreateReadValueExpression( |
There was a problem hiding this comment.
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.
|
I am not sure when is @maumar submitting PR for nullable conversion. Let's wait for that and we can revisit afterwards so you don't end up doing duplicated work as him. |
|
I hope to have something out tomorrow @roji @smitpatel |
|
Replaced by #17484 |
Part of #16963
First time going into InMemory so would appreciate a good review :)
One particular complication: for Default_if_empty_top_level_projection we need to apply Coalesce if a non-nullable value type member is accessed on a nullable entity projection (i.e. after DefaultIfEmpty). However, GroupJoin_DefaultIfEmpty_Project has the same structure within a Convert to object, which is then identified in VisitUnary and handled in a special way. I rewrote the pattern recognition to identify and remove the Coalesce for this particular case, but am not sure it's the right thing to do.