Improve binding to parent query models #7696

tuespetre · 2017-02-23T19:10:21Z

This is a much smaller, less radical version of #7663 that brings only enhancements (no regressions) to the plate.

dnfclas · 2017-02-23T19:10:25Z

@tuespetre,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

smitpatel

Add a test where subquery tries to bind with outer qmv which has client projection.

smitpatel · 2017-02-24T19:52:26Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

        /// <returns>
        ///     An Expression.
        /// </returns>
-        protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
+        protected override Expression VisitMethodCall(MethodCallExpression node)


revert the name change

smitpatel · 2017-02-24T19:52:45Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

        /// <returns>
        ///     An Expression.
        /// </returns>
-        protected override Expression VisitMember(MemberExpression expression)
+        protected override Expression VisitMember(MemberExpression node)


change to memberExpression

change to memberExpression or revert to expression?

smitpatel · 2017-02-24T19:53:10Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs


-            if (aliasExpression == null)
+        private Expression TryBindQuerySourcePropertyExpression(MemberExpression expression)


memberExpression

Is this method being used elsewhere? If not then we don't need to create this method.

There are several other methods (even in this class) that are only used in one place. I feel that the readability was greatly improved by making this into its own method:

return TryBindAliasExpression(node, (visitor, binder) => visitor.BindMemberExpression(node, binder)) ?? TryBindQuerySourcePropertyExpression(node) ?? _queryModelVisitor.BindMemberToOuterQueryParameter(node);

Additionally, this method can be expanded on (for instance, to add support for translating access of a grouping key.)

Sounds good then.

smitpatel · 2017-02-24T20:02:17Z

src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs

            {
-                var parentSelectExpression = ParentQueryModelVisitor?.TryGetQuery(querySource);
-                if (parentSelectExpression != null)
+                var ancestor = ParentQueryModelVisitor;


variable name -> outerQueryModelVisitor

smitpatel · 2017-02-24T20:02:31Z

src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs

@@ -1594,10 +1605,19 @@ public override Expression BindMemberToValueBuffer(MemberExpression memberExpres

        private ParameterExpression BindPropertyToOuterParameter(IQuerySource querySource, IProperty property, bool isMemberExpression)
        {
-            if (querySource != null && _canBindPropertyToOuterParameter)
+            if (querySource != null && _canBindPropertyToOuterParameter && ParentQueryModelVisitor != null)


remove null check on qmv

smitpatel · 2017-02-24T20:02:44Z

src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs

-                var parentSelectExpression = ParentQueryModelVisitor?.TryGetQuery(querySource);
-                if (parentSelectExpression != null)
+                var ancestor = ParentQueryModelVisitor;
+                SelectExpression outerQuery = null;


var name -> outerSelectExpression

smitpatel · 2017-02-24T20:03:12Z

src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs

+                var ancestor = ParentQueryModelVisitor;
+                SelectExpression outerQuery = null;
+
+                do


Remove do part of loop by assigning values when declaring variables.

smitpatel · 2017-02-24T20:10:20Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

+                var column = _relationalAnnotationProvider.For(property).ColumnName;
+                var table = selectExpression.GetTableForQuerySource(querySource);
+
+                return new AliasExpression(new ColumnExpression(column, property, table));


make inline.

Why? It's used twice. (See: https://github.com/aspnet/EntityFramework/pull/7696/files/e7e61afd4d65c3c1d158f9bd7911440f10f32148#diff-c0990b2bb9d2ed7c8071a0c4323c01a4R708)

Inline in the sense, you don't need to declare variables for column & table just put them directly in constructor since they are not being used.

smitpatel · 2017-02-24T20:17:06Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

+            }
+
+            var ancestor = _queryModelVisitor.ParentQueryModelVisitor;
+            var canBindToAncestor = _queryModelVisitor.CanBindToParentQueryModel;


You may not be able to bind to parent query model visitor if it requires client projection.

CanBindToParentQueryModel should never be true if the parent qmv requires client projection.

Can you point me to the place where we are setting that flag appropriately?

The flag is set by RelationalQueryModelVisitor.VisitSubQueryModel, which is called in RelationalQueryModelVisitor.LiftSubQuery and SqlTranslatingExpressionVisitor.VisitSubQuery.

FWIW, When a new query model visitor is created, the flag is set to false. So in the case of client projections we create the qmv in ProjectExpressionVisitor which would be called for client projections only.

smitpatel · 2017-02-24T21:20:02Z

src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs

+        /// <value>
+        ///     true if the query model visitor can bind to its parent's properties, false if not.
+        /// </value>
+        public virtual bool CanBindToParentQueryModel { get; protected set; } = false;


no need to set to false. Its default

smitpatel · 2017-02-24T21:26:37Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

-                        qmv => qmv.BindMemberExpression(expression, CreateAliasedColumnExpressionCore));
-            }
+            return TryBindAliasExpression(node, (visitor, binder)
+                    => visitor.BindMemberExpression(node, binder))


This is actually a closure rather than a Func since you are passing value of node into the lambda expression. If you look at the function TryBindAliasExpression then you will see the first parameter is never used in its method body. We don't want to create a closure here. Instead change this lambda to take the node as argument and pass the node to it in TryBindAliasExpression

smitpatel · 2017-02-24T21:26:55Z

Looks good.

tuespetre · 2017-02-24T21:51:59Z

🆙📅

smitpatel · 2017-02-24T22:14:25Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

-
-            return expression
-                   ?? _queryModelVisitor.BindMethodToOuterQueryParameter(methodCallExpression);
+            return TryBindAliasExpression(methodCallExpression, (e, visitor, binder)


e -> expression

smitpatel · 2017-02-24T22:14:40Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

@@ -646,7 +630,7 @@ protected override Expression VisitMember(MemberExpression expression)
            {
                var newExpression = Visit(expression.Expression);

-                if (newExpression != null
+                if (newExpression != null 


Format the doc.

huh, wonder why Visual Studio diff didn't show that

smitpatel · 2017-02-24T22:16:05Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

-                        _queryModelVisitor.ParentQueryModelVisitor,
-                        qmv => qmv.BindMemberExpression(expression, CreateAliasedColumnExpressionCore));
-            }
+            return TryBindAliasExpression(expression, (e, visitor, binder)


e -> expression

smitpatel · 2017-02-24T22:17:11Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs


-            if (aliasExpression == null)
+        private Expression TryBindQuerySourcePropertyExpression(MemberExpression expression)


memberExpression

tuespetre · 2017-02-24T22:20:20Z

🆙📅

smitpatel · 2017-02-24T22:20:24Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

-            return binder(queryModelVisitor)
-                   ?? TryBindParentExpression(queryModelVisitor.ParentQueryModelVisitor, binder);
-        }
+            var ancestor = _queryModelVisitor.ParentQueryModelVisitor;


can we move away from this variable name. ancestor does not specify ancestor of what particularly.
Better name would be outerQueryModelVisitor (since its not direct parent always)

Done (also changed canBindtoAncestor to canBindToOuterQueryModelVisitor)

smitpatel · 2017-02-24T22:21:34Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

-        private static AliasExpression TryBindParentExpression(
-            RelationalQueryModelVisitor queryModelVisitor,
-            Func<RelationalQueryModelVisitor, AliasExpression> binder)
+        private Expression TryBindAliasExpression<TExpression>(


~~Do we need generics here? Does a simple Expression work?~~
Probably won't work as binder methods are type specific.

Generic due to MemberExpression vs. MethodCallExpression

function name TryBindMemberOrMethodToAliasExpression?

smitpatel · 2017-02-24T22:27:24Z

Can you add some test around improved binding? Multi-level queries which can be bound with outer select expression through alias expression and through parameter.

smitpatel · 2017-02-24T22:28:17Z

...t.EntityFrameworkCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

+                        property,
+                        selectExpression.GetTableForQuerySource(querySource)));
+
+            var bound = binder(sourceExpression, _queryModelVisitor, (property, querySource, selectExpression) =>


boundExpression

tuespetre · 2017-02-27T20:08:01Z

@smitpatel working on tests.

tuespetre · 2017-02-27T21:39:25Z

🆙📅 Added some tests and also a fix for an error that was revealed by the tests. Did in a separate commit so you can see the isolated change.

smitpatel · 2017-02-28T23:10:38Z

src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs

@@ -1605,6 +1605,8 @@ public override Expression BindMemberToValueBuffer(MemberExpression memberExpres

        private const string OuterQueryParameterNamePrefix = @"_outer_";

+        private readonly Dictionary<string, Expression> _boundParameters = new Dictionary<string, Expression>();


Can we name it injectedParameters?

smitpatel

Squash-rebase on latest dev 👍

tuespetre · 2017-02-28T23:47:05Z

🏁 done

smitpatel · 2017-03-01T01:15:28Z

@tuespetre - QueryInMemoryTests are failing.

smitpatel · 2017-03-01T01:19:47Z

src/Microsoft.EntityFrameworkCore.Relational/Query/RelationalQueryModelVisitor.cs

+        ///     the subquery can be lifted.
+        /// </summary>
+        /// <param name="subQueryModelVisitor"> The query model visitor for the subquery being lifted. </param>
+        public void LiftInjectedParameters(RelationalQueryModelVisitor subQueryModelVisitor)


add virtual, NotNull annotation and null check on parameter

tuespetre · 2017-03-01T01:32:44Z

Oops, hadn't thought to run those before pushing. (What happened to the Travis logs BTW?)

tuespetre · 2017-03-01T03:03:05Z

🆙📅 fixed the tests (needed to change from Region to City)

smitpatel · 2017-03-01T20:36:37Z

Merged via 08bdfc9

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

tuespetre mentioned this pull request Feb 23, 2017

Implement GROUP BY #7697

Closed

smitpatel self-assigned this Feb 23, 2017

smitpatel added the needs-review label Feb 23, 2017

smitpatel suggested changes Feb 24, 2017

View reviewed changes

smitpatel reviewed Feb 24, 2017

View reviewed changes

tuespetre force-pushed the bind-parent-queries branch from e7e61af to 520ac6b Compare February 24, 2017 21:42

smitpatel reviewed Feb 24, 2017

View reviewed changes

tuespetre force-pushed the bind-parent-queries branch from 520ac6b to d25427b Compare February 24, 2017 22:20

smitpatel reviewed Feb 24, 2017

View reviewed changes

tuespetre force-pushed the bind-parent-queries branch from d25427b to 5d33173 Compare February 24, 2017 22:23

smitpatel reviewed Feb 24, 2017

View reviewed changes

tuespetre force-pushed the bind-parent-queries branch from 5d33173 to 7bea268 Compare February 25, 2017 03:03

smitpatel reviewed Feb 28, 2017

View reviewed changes

smitpatel approved these changes Feb 28, 2017

View reviewed changes

tuespetre force-pushed the bind-parent-queries branch from b03bae8 to 3d16914 Compare February 28, 2017 23:46

smitpatel reviewed Mar 1, 2017

View reviewed changes

tuespetre force-pushed the bind-parent-queries branch from 3d16914 to 40b1b43 Compare March 1, 2017 03:02

tuespetre force-pushed the bind-parent-queries branch 2 times, most recently from fff887a to 18b4d4c Compare March 1, 2017 03:05

Improve binding to parent query models

18b4d4c

smitpatel closed this Mar 1, 2017

tuespetre deleted the bind-parent-queries branch March 19, 2017 18:04


		if (aliasExpression == null)
		private Expression TryBindQuerySourcePropertyExpression(MemberExpression expression)

		@@ -1605,6 +1605,8 @@ public override Expression BindMemberToValueBuffer(MemberExpression memberExpres

		private const string OuterQueryParameterNamePrefix = @"_outer_";

		private readonly Dictionary<string, Expression> _boundParameters = new Dictionary<string, Expression>();

Improve binding to parent query models #7696

Improve binding to parent query models #7696

Conversation

tuespetre commented Feb 23, 2017

dnfclas commented Feb 23, 2017

smitpatel left a 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

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

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

smitpatel commented Feb 24, 2017

tuespetre commented Feb 24, 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

tuespetre commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel 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

smitpatel commented Feb 24, 2017

Choose a reason for hiding this comment

tuespetre commented Feb 27, 2017

tuespetre commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel left a comment

Choose a reason for hiding this comment

tuespetre commented Feb 28, 2017

smitpatel commented Mar 1, 2017

Choose a reason for hiding this comment

tuespetre commented Mar 1, 2017

tuespetre commented Mar 1, 2017

smitpatel commented Mar 1, 2017

tuespetre Feb 24, 2017 •

edited

Loading

smitpatel Feb 24, 2017 •

edited

Loading