Refactor RequiresMaterializationExpressionVisitor #7695

tuespetre · 2017-02-23T06:44:50Z

This is the code at the heart of the efforts I made in #7543 and #7650.

I don't know why it didn't occur to me to make a branch that had just this in it without removing the 'training wheels' in the QueryCompilationContext (the code that brute-force flags all group joins as requiring materialization.) It actually went really easy, I just had to make a tiny little patch to SelectExpression and update some tests' SQL output (one which has a MUCH better query now, another which doesn't pull unnecessary columns anymore, and the rest which just have smaller resulting SQL text.)

Whenever the team is ready to work on the things I accomplished in those other two PRs, all that would need to be done is:

Uncomment lines 416 and 417 in RequiresMaterializationExpressionVisitor.cs
Remove the code in QueryCompilationContext that forces all group joins to be materialized
Have fun

maumar · 2017-02-23T20:33:45Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Expressions/SelectExpression.cs

@@ -170,6 +170,11 @@ public virtual void AddTable([NotNull] TableExpressionBase tableExpression)
        {
            Check.NotNull(tableExpression, nameof(tableExpression));

+            if (_tables.Count == 0)


this shouldn't be needed after you rebase on current dev

I thought that may be the case, I saw @smitpatel's other change (ProjectStarTable)

maumar · 2017-02-23T20:40:45Z

...ntityFrameworkCore/Query/ExpressionVisitors/Internal/MemberAccessBindingExpressionVisitor.cs

@@ -188,11 +188,20 @@ protected override Expression VisitExtension(Expression node)
                if (newCaller != nullConditionalExpression.Caller
                    && newCaller.Type == typeof(ValueBuffer))
                {
-                    var newAccessOperation = Visit(nullConditionalExpression.AccessOperation);
+                    var convertedAccessOperation


do the conversion after the visit and only if needed

tuespetre · 2017-02-23T21:52:19Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

-                            });
+                if (node.Expression.Type.IsGrouping() && node.Member.Name == "Key")
+                {
+                    var groupResultOperator


The contents of this if block should be simplified to:

if (node.Expression is QuerySourceReferenceExpression qsre) { DemoteQuerySource(qsre.ReferencedQuerySource); }

maumar · 2017-02-23T22:57:52Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

                    {
-                        AddQuerySource(underlyingQuerySource);
+                        var parentStatistics = _querySourceReferences[parentQuerySource];


maumar · 2017-02-23T23:01:24Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+            }
+            else
+            {
+                _querySourceReferences[querySource] += 1;


maumar · 2017-02-23T23:01:29Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+
+            if (_querySourceReferences.ContainsKey(querySource))
+            {
+                _querySourceReferences[querySource] -= 1;


maumar · 2017-02-23T23:06:48Z

...ntityFrameworkCore/Query/ExpressionVisitors/Internal/MemberAccessBindingExpressionVisitor.cs

                    {
-                        return newAccessOperation;
+                        var test = ValueBufferNullComparisonCheck(newCaller);
+                        var ifTrue = newAccessOperation;


var ifTrue = newAccessOperation.Type != nullConditionalExpression.Type ? Expression.Convert(newAccessOperation, nullConditionalExpression.Type) : nullConditionalExpression;

smitpatel · 2017-02-23T23:10:58Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

@@ -10,6 +10,9 @@
 using Remotion.Linq.Clauses;
 using Remotion.Linq.Clauses.Expressions;
 using Remotion.Linq.Clauses.ResultOperators;
+using Remotion.Linq.Clauses.StreamedData;
+using System;
+using Microsoft.EntityFrameworkCore.Query.Expressions.Internal;


sort usings

smitpatel · 2017-02-23T23:16:53Z

test/Microsoft.EntityFrameworkCore.InMemory.FunctionalTests/DatabaseErrorLogStateTest.cs

@@ -107,18 +107,6 @@ public void Query_logs_DatabaseErrorLogState_during_single_async()
            Query_logs_DatabaseErrorLogState(c => c.Blogs.FirstOrDefaultAsync().Wait());
        }

-        [Fact]


Why these tests are removed?

they no longer materialize entities therefore exception is no longer thrown

maumar · 2017-02-23T23:21:02Z

...ntityFrameworkCore/Query/ExpressionVisitors/Internal/MemberAccessBindingExpressionVisitor.cs

+                        var ifTrue = newAccessOperation;
+                        var ifFalse = Expression.Default(convertedAccessOperation.Type);
+
+                        return Expression.Condition(test, ifTrue, ifFalse);


For which scenarios simply returning visited AccessOperation (after we adjust for potential type difference) is not enough?

CompexNavigationsInMemoryTest, GroupJoin_on_right_side_being_a_subquery

smitpatel · 2017-02-23T23:34:30Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+
+                if (isSubQuery && finalResultOperator is GroupResultOperator groupResultOperator)
+                {
+                    //DemoteQuerySource(referencedQuerySource);


The GroupResultOperator cannot be demoted until GROUP BY is being implemented. Those are the two lines I mentioned in my initial comments for this PR

Can you add the same in code comments? PR comments are disconnected with source (and even commit)

maumar · 2017-02-24T01:10:33Z

I hit a regression in a couple tests: ComplexNavigationsQuerySqlServerTest.Null_protection_logic_work_for_inner_key_access_of_manually_created_GroupJoin2
and
ComplexNavigationsQuerySqlServerTest.GroupJoin_on_complex_subquery_and_set_operation_on_grouping_but_nothing_from_grouping_is_projected

for the first one, with the following query model:

from Level1 l1 in DbSet<Level1>
join Level2 l2 in 
    from Level1 l1 in DbSet<Level1>
    join Level2 l1.OneToOne_Required_FK in DbSet<Level2>
    on Property(l1, "Id") equals Property(l1.OneToOne_Required_FK, "Level1_Required_Id") into l1.OneToOne_Required_FK_group
    from Level2 l1.OneToOne_Required_FK in 
        (from Level2 l1.OneToOne_Required_FK_groupItem in l1.OneToOne_Required_FK_group
        select l1.OneToOne_Required_FK_groupItem)
        .DefaultIfEmpty()
    select l1.OneToOne_Required_FK
on (Nullable<int>) l1.Id equals (?Property([l2], "Level1_Optional_Id")?) into l2s
select l1

the new algorithm marks:
from Level1 l1 in value(EntityQueryable`1[Level1]) and
join Level2 l1.OneToOne_Required_FK in value(EntityQueryable`1[Level2]) on Property([l1], "Id") equals Property([l1.OneToOne_Required_FK], "Level1_Required_Id")

for materialization.

This is incorrect, only Level1 should be marked as the result of subquery in not present in the final projection (that was the behavior before the change). The problem doesn't surface currently because the second query source gets marked for materialization anyway in QueryCompilationContext, but once we lift that excessive materialization from QCC this scenario will be blocked from being optimized

tuespetre · 2017-02-24T01:29:45Z

That is probably because I was going off of the group join clause as the query source rather than its join clause to determine what required materialization. I will adjust for that, thanks for the example

tuespetre · 2017-02-24T02:54:15Z

🆙📅

tuespetre · 2017-02-24T02:54:53Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+                        _queryModelStack.Peek().SelectClause.Selector,
+                        referencedQuerySource);
+
+                    if (traced == null || finalResultOperator is DefaultIfEmptyResultOperator)


@maumar This seems to be what was needed (the || finalResultOperator is DefaultIfEmptyResultOperator)

Disregard that. 🐓 I tested that out on my branch that has all of the optimizations and it marked several things incorrectly. This change is right. (I tested this change on that branch as well and it improved both of the queries you pointed out @maumar.)

yup, ran the updated code alongside my changes and the queries are better indeed

tuespetre · 2017-02-24T14:43:19Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+                {
+                    var choiceResultOperator = (ChoiceResultOperatorBase)finalResultOperator;
+
+                    if (isSubQuery && choiceResultOperator.ReturnDefaultWhenEmpty)


I don't think this check (choiceResultOperator.ReturnDefaultWhenEmpty) really needs to be here, so this block can be collapsed with the preceding block. I've tested this idea on my branch with all of the optimizations and everything stayed the same test-wise.

i think we are missing test coverage, there should definitely be a difference between First and FirstOrDefault when used in a subquery. FoD we can sometimes translate but first will always result in client evaluation. Wrt LastOrDefault, we can only translate it if the query also has orderby (we change the asc <-> desc and slap FirstOrDefault instead). Perhaps we can make this change as part of QueryModel optimization (before this step), so every time you encounter LoD here, you know it needs client eval.

We do not modify the LoD in optimizequery model instead we do it in HandleLast here https://github.com/aspnet/EntityFramework/blob/dev/src/Microsoft.EntityFrameworkCore.Relational/Query/Internal/RelationalResultOperatorHandler.cs#L561

Whether or not those result operators need to be client evaluated doesn't affect whether the underlying query source needing materialization or not though. customers.Select(c => c.Name).Single() should not materialize the whole customer

@smitpatel yeah, but we can move that earlier

@tuespetre this is because in your example the subquery that Single is wrapped around doesn't need the c materialized on its own (it's not projected by that particular subquery). If the query model was instead:

from c in customers select {(from c2 in customers select c2).Single()}.Name

(that thing will be optimized by relinq, but lets assume its not)

then the Single will force materialization on the inner subquery, and since result of that subquery is c2, its query source should be marked.

What is happening in the code (in AdjustForResultOperators) is, we peek into the parent selector, and if that selector can't be traced back to the result of inner query, we demote the result of that inner query. This is ok for most cases, but with Single/First etc we still need the result of that inner subquery materialized.

However one could argue that this is not a concern of the RequiresMaterialization visitor, since it shouldn't care which operators can be translated.

@maumar in the new example you gave, the customer still does not need to be materialized. But if it were to select a NotMapped property instead of Name, or call some client method on it instead of accessing the Name member, it should be materialized.

IOW: Single().Name can translate to SELECT TOP (2) c.Name FROM ..., no need for materialization. SQL example, but should hold true for any provider.

@tuespetre yeah that's because relinq optimized the case. I gave this (simplified) case with a caveat that the QM will be processed in this form. A real life example would be: (with the complex navigations model - using required navs so that QueryCompilationContext doesn't compensate)

ctx.LevelTwo.Select(l2 => ctx.LevelTwo.First().OneToOne_Required_FK_Inverse.Name)

this gets converted into the following QM:

from Level2 l1 in DbSet<Level2> select (from Level2 <generated>_1 in DbSet<Level2> join Level1 <generated>_1.OneToOne_Required_FK_Inverse in DbSet<Level1> on Property(<generated>_1, "Level1_Required_Id") equals Property(<generated>_1.OneToOne_Required_FK_Inverse, "Id") select <generated>_1.OneToOne_Required_FK_Inverse) .First().Name

now, this will be translated into the following query plan:

TRACKED: False (QueryContext queryContext) => IEnumerable<string> _Select( source: IEnumerable<ValueBuffer> _ShapedQuery( queryContext: queryContext, shaperCommandContext: SelectExpression: SELECT 1 FROM [Level2] AS [l1] , shaper: ValueBufferShaper ) , selector: (ValueBuffer l1) => Level1 First( source: IEnumerable<Level1> _Select( source: IEnumerable<TransparentIdentifier<ValueBuffer, Level1>> _ShapedQuery( queryContext: queryContext, shaperCommandContext: SelectExpression: SELECT TOP(1) [l0].[Id], [l0].[Date], [l0].[Name], [l0].[OneToMany_Optional_Self_InverseId], [l0].[OneToMany_Required_Self_InverseId], [l0].[OneToOne_Optional_SelfId] FROM [Level2] AS [l] INNER JOIN [Level1] AS [l0] ON [l].[Level1_Required_Id] = [l0].[Id] , shaper: CompositeShaper`3 ) , selector: (TransparentIdentifier<ValueBuffer, Level1> t0) => t0.Inner ) ) .Name )

Note that the inner query, inside First() is materialized into entity, so everything works fine.
However, if you remove the && choiceResultOperator.ReturnDefaultWhenEmpty from the line 417 this query will now not be marked for materialization and will fail to compile.

From the perspective on QM translation to query plan, First, Single, SingleOrDefault are indistinguishable from a simple client method that takes a collection and returns single element.

In that case though, it should not fail to compile because the MemberAccessBindingExpressionVisitor will check if the type is ValueBuffer and use BindMemberToValueBuffer

@tuespetre I think you are right. Currently we can't properly bind in this case, but that's a bug in the compilation rather than the RequiresMaterializationExpressionVisitor

maumar · 2017-02-25T03:44:47Z

@tuespetre, I found some more issues. Those are not regressions (current code is also wrong) but perhaps you might want to look into it.

Tests in question are:
Collection_select_nav_prop_single_or_default_then_nav_prop_nested
Collection_select_nav_prop_first_or_default_then_nav_prop_nested
Collection_select_nav_prop_first_or_default_then_nav_prop_nested_with_orderby

a simplified version of the query:

ctx.Customers.Select(c => ctx.Orders.SingleOrDefault(o => o.OrderID == 10643).Customer.City)

and a variation (using client-side method):

ctx.Customers.Select(c => ClientFirstOrDefault(ctx.Orders).Customer.City));

        public static T ClientFirstOrDefault<T>(IQueryable<T> source)
        {
            return source.FirstOrDefault();
        }

query model:

from Customer c in DbSet<Customer>
select 
    (from Order o in DbSet<Order>
    join Customer o.Customer in DbSet<Customer>
    on Property(o, "CustomerID") equals Property(o.Customer, "CustomerID") into o.Customer_group
    from Customer o.Customer in 
        (from Customer o.Customer_groupItem in o.Customer_group
        select o.Customer_groupItem)
        .DefaultIfEmpty()
    where o.OrderID == 10643
    select o.Customer)
    .SingleOrDefault().City

so o.Customer should be marked for materialization, because it's behind SingleOrDefault (or a client method). Like with the previous example, it's not an issue currently because we blanket-materialize all joins, but will be a problem in the future.

maumar · 2017-02-25T03:51:08Z

for the client-side variant, the query model is:

from Customer c in DbSet<Customer>
select Order ClientFirstOrDefault(
    source: DbSet<Order>
)
.Customer.City

and orders don't get marked for materialization either

maumar · 2017-02-25T07:52:43Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs


-            return node;
+        private void AddResultOperators(QueryModel queryModel)


No need to pass querymodel here, also this could be more general-purpose, e.g.

PromoteQuerySources(IEnumerable<IQuerySource> querySources)

Really the method could go away altogether, just needs to be:

if (queryModel.ResultOperators.LastOrDefault() is IQuerySource querySource) { PromoteQuerySource(querySource); }

Or rather, foreach (var querySourceResultOperator in queryModel.ResultOperators.OfType<IQuerySource>()). I mean realistically only the GroupResultOperator is an IQuerySource (I think) but for the sake of correctness? 😛

Inlining sounds good.

tuespetre · 2017-02-25T15:45:13Z

@maumar on your examples, with the SingleOrDefault, only the one field is selected so there is no materialization. The ClientFirstOrDefault is definitely an issue though.

tuespetre · 2017-02-25T20:00:05Z

@maumar I've investigated the ClientFirstOrDefault example further.

First, an exception is thrown when evaluating the query, because EntityQueryableExpressionVisitor tries to make a SelectExpression for it but the querySource is null because it is in the SelectClause. The EntityQueryableExpressionVisitor should skip over EntityQueryable expressions in such cases and leave them for evaluation during client evaluation of the projection. But if it were changed to ClientFirstOrDefault(from o in context.Orders select o), that would be different story because all of a sudden it can be assessed as a subquery instead of just an EntityQueryable.
Second, and related to that, during the first run through of determining the query sources requiring materialization (for the parent query) we would not expect to see the orders marked as requiring materialization but on the subsequent pass through RequiresMaterializationExpressionVisitor we should expect to see them marked so (happening when the projection is client-evaluated.)
Third, about how I mentioned ClientFirstOrDefault(from o in context.Orders select o), I've looked at that and there is indeed a problem there, because the orders are not marked when they should be.

EDIT

Fourth, I fixed the issue with the third point, but the query is no good anyways because of A second operation started on this context before a previous operation completed.

maumar · 2017-02-27T02:01:51Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

-                && (_model.FindEntityType(leftSubQueryExpression.Type) != null))
+        private Expression VisitBinaryOperand(Expression operand, ExpressionType comparison)
+        {
+            switch (comparison)


convert this to a single if statement instead, there is only one case + default

maumar · 2017-02-27T02:02:53Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

@@ -176,91 +218,267 @@ protected override Expression VisitBinary(BinaryExpression node)
        /// </summary>
        protected override Expression VisitSubQuery(SubQueryExpression expression)
        {
-            var oldParentSelector = _selector;
-            var oldQueryModel = _queryModel;
+            _queryModelStack.Push(expression.QueryModel);


don't we need to add group result operators here also? (like we do for the top level query)

We do that on the parent query because it absolutely has to be, but for subqueries, any group result operators should be promoted/demoted as necessary via HandleUnderlyingQuerySources

tuespetre · 2017-02-27T15:04:32Z

🆙📅

maumar · 2017-02-27T18:45:14Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+
+            var finalResultOperator = queryModel.ResultOperators.LastOrDefault();
+
+            var unreachableFromParentSelector =


compute this after the GroupBy check

Done ✔️

ksmithRenweb · 2017-02-27T20:31:42Z

@tuespetre @maumar Thank you for working on this. When it gets released it will solve some major performance issues in our app. (It should reduce one very frequent call from 1 second to 30 ms. Blame a table with 300 columns being materialized, when we only need 3 of them in the query.)

smitpatel · 2017-02-28T18:47:14Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+                // that reference an entity query source does not suggest that
+                // materialization of that entity type may be required. This is true
+                // whether in a join predicate, where predicate, selector, etc.
+                if (operand is QuerySourceReferenceExpression && isEntityTypeExpression)


Can you update comment that QSRE referencing entity type is re-written into key equality comparison.

smitpatel · 2017-02-28T18:49:26Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs


-            var leftSubQueryExpression = node.Left as SubQueryExpression;
+            return node;


Why this method return same node instead of the visited node like other methods.

Because the version before my changes did the same. This visitor doesn't actually change anything, but I guess it wouldn't hurt to return node.Update(newLeft, node.Conversion, newRight).

We should be consistent about it. Either we return same expression in everything or we send updated. Change every overridden method to return the same expression which came via parameter.

I'm changing to return node.Update(newLeft, node.Conversion, newRight)

smitpatel · 2017-02-28T18:50:39Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs


-            var rightSubQueryExpression = node.Right as SubQueryExpression;
+                if (operand is SubQueryExpression subQueryExpression && isEntityTypeExpression)


What is the different when operand is subquery & its entitytype expression? This subquery expression will be revisited in VisitSubquery when you call Visit(operand)

The previous version did the same thing, it should be the same thing as the QuerySourceReferenceExpression case -- for key equality comparisons.

Adding comments for this.

Can you try removing this whole part and see how VisitSubquery works out?

Test 'Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests.RowNumberPagingTest.Where_query_composition_is_not_null' failed: System.InvalidOperationException : Reference equality is not defined for the types 'Microsoft.EntityFrameworkCore.Storage.ValueBuffer' and 'System.Object'.

(Posted under the wrong comment at first!)

smitpatel · 2017-02-28T18:52:12Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs


-            _queryModel.TransformExpressions(Visit);
+            AdjustForResultOperators(expression.QueryModel);


What is guidance on using AdjustForResultOperators?
At very top level, we add result operator before visiting query model and then adjust result operators after visiting qm. Here we did not add result operators. Few lines above we also visiting query model but we are not doing anything with result operators.

The top-level IQuerySource result operators are added that way because it's the top level and they won't get promoted the same way they do for subqueries (via HandleUnderlyingQuerySources.)

Adding comments for this.

smitpatel · 2017-02-28T18:55:45Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

                    {
-                        AddQuerySource(underlyingQuerySource);
+                        foreach (var sourceExpression in resultSetOperators)


Will there be more than one sourceExpression matching current expression?

Shouldn't be, the old version used a foreach loop too, a more succinct version would be:

if (resultSetOperators.Any(r => r.Equals(expression)) && _querySourceReferences[parentQuerySource] > 0) { PromoteQuerySource(referencedQuerySource); }

smitpatel · 2017-02-28T18:58:01Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+                }
+
+                var elementSelectorQuerySource
+                    = (groupResultOperator.ElementSelector as QuerySourceReferenceExpression)


This is being repeated multiple times. Make a method which try safe cast to QSRE and returns referenced query source.

This line was written before the VS2017 conversion, and now the language makes it so much nicer 😉

if (groupResultOperator.KeySelector is QuerySourceReferenceExpression keySelectorQsre) { action(keySelectorQsre.ReferencedQuerySource); }

smitpatel · 2017-02-28T18:58:47Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+
+                var elementSelectorSubQueryQuerySource
+                    = ((groupResultOperator.ElementSelector as SubQueryExpression)
+                        ?.QueryModel.SelectClause.Selector as QuerySourceReferenceExpression)


Similar to above, function to get selector after safe casting to Subqueryexpression

Making a change that will make this more terse/compact but not quite enough to justify a method, IMO.

It is being repeated at multiple places in same pattern. It can be simple method TryGetSelectorFromSubquery

smitpatel · 2017-02-28T19:03:43Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+            }
+
+            var unreachableFromParentSelector =
+                 isSubQuery && new QuerySourceTracingExpressionVisitor()


new QuerySourceTracingExpressionVisitor() [](start = 31, length = 41)

This can (or should) be stored in static in this class so that we don't initialize it all the time. The same instance can serve all the invocations of the function.

It is not thread-safe so static is off limits. Caching a single instance per RequiresMaterializationExpressionVisitor may work.

smitpatel · 2017-02-28T19:05:10Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

-                return concatOperator.Source2;
+                // If not a subquery or if reachable from the parent selector
+                // we would not want to fall through to one of the next blocks.
+                if (isSubQuery && unreachableFromParentSelector)


unreachableFromParentSelector asserts isSubQuery already.

smitpatel · 2017-02-28T19:06:21Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+                // we would not want to fall through to one of the next blocks.
+                if (isSubQuery && unreachableFromParentSelector)
+                {
+                    DemoteQuerySourceWithUnderlying(referencedQuerySource);


DemoteQuerySourceWithUnderlying [](start = 20, length = 31)

rename function to something better. @maumar suggest a name.

I don't like the name either, but can't come up with anything much better. Maybe DemoteQuerySourceAndItsFromClause? Method is not very complicated, so having a bad name is not a big deal. I'm fine with either.

DemoteQuerySourceAndUnderlyingFromClause?

@tuespetre sounds good to me

smitpatel · 2017-02-28T19:19:42Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        protected override Expression VisitExtension(Expression node)


@maumar to verify where and how this is needed.

@smitpatel it's needed because we are counting references and visiting each of the Caller and the NullableCaller and the AccessOperation would throw off the count (as I found when working on materialization changes.)

Maybe we can do a check similar to the one in binary expression here - if Caller/NullableCaller is qsre of type entity, we don't promote, but otherwise we do

Don't we need VisitMember to ensure proper counting though? What if the accessed property is NotMapped

You would visit the AccessExpression, just it's done currently, but on top of that peek into the Callers. They should be just entity qsres for all the real cases, so for all those cases, the code will behave the same way. But we would also cover our bases for the weird cases (e.g. when the user manually creates NullConditionalExpression)

Can we possibly revisit that later, if it becomes an issue

Can we possibly revisit that later, if it becomes an issue

Are we removing this override fully then? If yes, then its fine to revisit later.

@smitpatel @maumar I'm not sure I understand.

If we just remove this override altogether, the base implementation skips visiting the NullConditionalExpression entirely. The override here is at least an improvement and makes sure we count the references as we should; we can take it out but then we may not be catching things we should (like NotMapped properties.)

And what about the other places that only work with AccessOperation, like ExpressionEqualityComparer and SqlTranslatingExpressionVisitor? That's why I'm asking if it can be revisited later: because places like that aren't doing anything special with Caller and NullableCaller, and if we're going to put that under a microscope here, shouldn't it be addressed in the other places too?

the base implementation skips visiting the NullConditionalExpression entirely.

Wasn't aware of that. I assumed it would call VisitChildren in class. Since it is skipping over, lets keep this. @maumar - to file issue on usage of NullConditionalExpression (based on our discussion around caller) and link it to #7520

we were under the assumptions that the VisitChildren would get called, which would visit all the nodes. However thats not the case (we special case NullConditional in the base visitor), so this is fine as it is. I will try to come up with some tests, see if I can break it, but this can be done later. Let's leave this method as is for now.

smitpatel · 2017-02-28T19:24:12Z

Loos good 😄

tuespetre · 2017-02-28T20:47:40Z

...yFrameworkCore/Query/ExpressionVisitors/Internal/RequiresMaterializationExpressionVisitor.cs

                {
-                    _querySources[querySourceReferenceExpression.ReferencedQuerySource]--;
+                    action(keySelectorQsre.ReferencedQuerySource);


@smitpatel It's only this block using this exact pattern, and only two times next to each other, just doesn't seem like enough to warrant a whole separate method...

It is being used many times like line 156 or 237. I suppose this thing is being used at multiple places even outside of this file and a good candidate for extension method. You can leave it as is though. I will update it in future refactoring.

In reply to: 103548170 [](ancestors = 103548170)

tuespetre · 2017-02-28T20:49:01Z

🆙📅 added more comments, made a few things more terse.

maumar

phew! 95 comments, I'm glad we are done here ;)

maumar · 2017-02-28T22:28:38Z

@tuespetre please squash-rebase on current dev and I will merge it

Improved the algorithm of RequiresMaterializationExpressionVisitor to provide better detection of which query sources need materialized. Currently the QueryCompilationContext forces all group joins to be materialized, but these changes produce the correct results for when the time comes to remove the forcing. These changes also prepare for the possibility of providers translating the GroupBy result operator.

tuespetre · 2017-02-28T22:30:24Z

@maumar done 🏁

maumar · 2017-02-28T23:53:29Z

@tuespetre merged in 67b4d22 Thank you for the contribution!

tuespetre · 2017-03-01T03:12:34Z

Awesome, thanks @maumar! Question now, is anyone on the team already working on the group join materialization stuff?

maumar · 2017-03-01T04:00:30Z

I'm almost done with a partial fix (one last issue left to figure out, I hope) that only requires materialization for queries when we actually introduce client GroupJoin. This fixes optional navs case where we can eliminate the client GroupJoin because the LOJ is flattened with SelectMany.

maumar · 2017-03-01T04:04:10Z

Also a while back @anpete was working on a more general solution but hit some roadblocks. IIRC the problematic case was when there was nothing from the outer in the final result, so you couldn't figure out how to split the groups. We put that solution on hold for now :)

dnfclas added the cla-already-signed label Feb 23, 2017

tuespetre force-pushed the requires-materialization branch 2 times, most recently from 2b2ba5c to 62c9810 Compare February 23, 2017 06:55

tuespetre mentioned this pull request Feb 23, 2017

Implement GROUP BY #7697

Closed

maumar self-assigned this Feb 23, 2017

maumar reviewed Feb 23, 2017

View reviewed changes

tuespetre commented Feb 23, 2017

View reviewed changes

maumar reviewed Feb 23, 2017

View reviewed changes

smitpatel reviewed Feb 23, 2017

View reviewed changes

maumar reviewed Feb 23, 2017

View reviewed changes

smitpatel reviewed Feb 23, 2017

View reviewed changes

tuespetre force-pushed the requires-materialization branch from 27eda26 to a8ca21f Compare February 24, 2017 02:46

tuespetre commented Feb 24, 2017

View reviewed changes

maumar reviewed Feb 25, 2017

View reviewed changes

tuespetre force-pushed the requires-materialization branch from a8ca21f to b7a7e13 Compare February 25, 2017 20:35

maumar reviewed Feb 27, 2017

View reviewed changes

tuespetre force-pushed the requires-materialization branch from 80129f5 to 5586c0f Compare February 27, 2017 18:50

smitpatel reviewed Feb 28, 2017

View reviewed changes

tuespetre force-pushed the requires-materialization branch from 5586c0f to 276ebd3 Compare February 28, 2017 20:35

tuespetre commented Feb 28, 2017

View reviewed changes

maumar approved these changes Feb 28, 2017

View reviewed changes

tuespetre force-pushed the requires-materialization branch from 276ebd3 to c6ae0a8 Compare February 28, 2017 22:30

maumar closed this Feb 28, 2017

tuespetre deleted the requires-materialization branch March 1, 2017 03:11


		return node;
		private void AddResultOperators(QueryModel queryModel)


		var finalResultOperator = queryModel.ResultOperators.LastOrDefault();

		var unreachableFromParentSelector =


		var leftSubQueryExpression = node.Left as SubQueryExpression;
		return node;


		var rightSubQueryExpression = node.Right as SubQueryExpression;
		if (operand is SubQueryExpression subQueryExpression && isEntityTypeExpression)


		_queryModel.TransformExpressions(Visit);
		AdjustForResultOperators(expression.QueryModel);

Refactor RequiresMaterializationExpressionVisitor #7695

Refactor RequiresMaterializationExpressionVisitor #7695

Conversation

tuespetre commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar commented Feb 24, 2017 • edited Loading

tuespetre commented Feb 24, 2017

tuespetre commented Feb 24, 2017

tuespetre Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

tuespetre Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuespetre Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar Feb 25, 2017 • edited Loading

Choose a reason for hiding this comment

tuespetre Feb 25, 2017 • edited Loading

Choose a reason for hiding this comment

maumar Feb 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar commented Feb 25, 2017

maumar commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuespetre commented Feb 25, 2017

tuespetre commented Feb 25, 2017 • edited Loading

Choose a reason for hiding this comment

maumar Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuespetre commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksmithRenweb commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar Feb 23, 2017 •

edited

Loading

maumar commented Feb 24, 2017 •

edited

Loading

tuespetre Feb 24, 2017 •

edited

Loading

tuespetre Feb 24, 2017 •

edited

Loading

tuespetre Feb 24, 2017 •

edited

Loading

maumar Feb 25, 2017 •

edited

Loading

tuespetre Feb 25, 2017 •

edited

Loading

maumar Feb 25, 2017 •

edited

Loading

tuespetre commented Feb 25, 2017 •

edited

Loading

maumar Feb 27, 2017 •

edited

Loading