Query: Refactor SelectExpression for referential integrity #7950

smitpatel · 2017-03-22T03:01:11Z

In current query pipeline, we create various column/alias expressions in ad-hoc manner and add them to SelectExpression, which create lack of detailed metadata info. e.g. In nested SelectExpression, we don't know which column from inner subqueries is being projected out in outer projection. That is a likely cause to create invalid SQL since it creates disconnect between the sql expressions and selectexpression. This also gives rise to various assumptions which goes viral across the stack like TryGetColumnExpression.
Above issue is huge blocker for immutable SelectExpression. This PR is first step in that direction.

Changes involved:
Overall design:
Every sql expression added to SelectExpression knows its source metadata. It is either a column expression backing property on an entity type or it is a .net expression built by combining column expressions. In nested queries, expressions on outer queries have reference to inner expression from where they are getting values. All parts of SelectExpression can be any kind of expression (which was previously restricted to AliasExpression).

Changes:

No ad-hoc creation of ColumnExpression. When binding a property to SelectExpression, SelectExpression will generate appropriate expression for it. (BindPropertyToSelectExpression method which binds property even if it is not added to projection)
Removal of non-property ColumnExpression - ColumnExpression represent a column which is directly backed by EntityType.Property. For any other case, different kind of expression should be used.
AliasExpression requires alias to be defined always. Hence unless column needs to aliased, it will not be made AliasExpression.
Implementation of Equals method of various expression which can appear in SelectExpression. And using ExpressionEqualityComparer to compare expressions in Projection/OrderBy to avoid duplicates. This removes the complex logic in AddToProjection.
Removal of TryGetColumnExpression - All the places, which relied on assumption of having a column backed expression are changed to work with any kind of expression.
Introduction of ColumnReferenceExpression which can store reference to inner projection expression so that each expression knows its root from subqueries.
~~- Introduction of ProjectStarExpression which stores all the projection which are actually being used by outer SelectExpression. (Though they are not printed in SQL. Just * will be printed)~~
SelectExpression._starProjection stores list of expressions being used by outer SelectExpression.
Simplification of AddToProjection method - Method adds expression straight to projection. Bind method will create the expression to be added from property. Method also unique-fy alias if it is inside subquery. This method also update OrderBy list if a complex expression is getting added to projection and present in order by by alias-ing it.
Removal of SourceMember/SourceExpression from AliasExpression, (which brought requirement of having every columnexpression as aliasExpression in projection). While visiting NewExpression in RelationalProjectionExpressionVisitor, we record SourceExpression to sqlExpression mapping and then we write MemberInfo to sqlExpression mapping in SelectExpression. Which is appropriately lifted during Subquery Pushdown. This makes it possible for us to bind operators after projecting into anonymous types.
Moved RelationalAnnotationsProvider to SelectExpression to find column name so that all the visitors trying to bind property with select expression does not need it to generate column name.

Pending tasks in this PR:

~~XML doc comments are pending. (I will write them tomorrow, created PR so that code review can start)~~
More tests? Suggestions?

Future tasks:

Investigate visiting NewExpression in RelationalProjectionExpressionVisitor. At present we visit it twice. The only info we get out of first visit is about client projection. We throw away translated expression. (this would fix Query: Query containing subquery with annonymous type fails to translate to server #7844 )
Removal of IncludeExpressionVisitor and associated helper methods from SelectExpression which are just to support current Include pipeline.
More robust way to deal with Anonymous expressions in projection which helps us identify an anonymous projection from an actual client eval method.
Implement Equals method on all custom expression. Uniform or need based?
Investigate and consolidate the handling of star projection. Currently we have IsProjectStar, ProjectStarExpression, ProjectStarTable.

This PR is part of #7520
Partially fix #6703

tuespetre · 2017-03-22T15:23:16Z

Moved RelationalAnnotationsProvider to SelectExpression to find column name so that all the visitors trying to bind property with select expression does not need it to generate column name.

omgyes

tuespetre

Excited to work with the improvements you made!

tuespetre · 2017-03-22T15:24:27Z

src/EFCore.Relational/Query/Internal/RelationalResultOperatorHandler.cs

            if (relationalQueryModelVisitor.RequiresClientEval
                || relationalQueryModelVisitor.RequiresClientSelectMany
                || relationalQueryModelVisitor.RequiresClientJoin
                || relationalQueryModelVisitor.RequiresClientFilter
                || relationalQueryModelVisitor.RequiresClientOrderBy
                || relationalQueryModelVisitor.RequiresClientResultOperator
-                || !_resultHandlers.TryGetValue(resultOperator.GetType(), out resultHandler)
+                || !_resultHandlers.TryGetValue(resultOperator.GetType(), out Func<HandlerContext, Expression> resultHandler)


out var resultHandler

tuespetre · 2017-03-22T15:28:41Z

src/EFCore.Relational/Query/ExpressionVisitors/Internal/IncludeExpressionVisitor.cs

                                /*querySource:*/ null,
-                                out _);
+                                out Dictionary<Type, int[]> _);


tuespetre · 2017-03-22T15:29:01Z

src/EFCore.Relational/Query/ExpressionVisitors/Internal/IncludeExpressionVisitor.cs

                                    p,
                                    querySource),
                                /*querySource:*/ null,
-                                out _);
+                                out Dictionary<Type, int[]> _);


tuespetre · 2017-03-22T15:31:03Z

src/EFCore.Relational/Query/ExpressionVisitors/RelationalEntityQueryableExpressionVisitor.cs

                                    p,
                                    _querySource),
                            _querySource,
-                            out typeIndexMap).Compile();
+                            out Dictionary<Type, int[]> typeIndexMap).Compile();


out var typeIndexMap

tuespetre · 2017-03-22T15:34:56Z

src/EFCore.Relational/Query/ExpressionVisitors/SqlTranslatingExpressionVisitor.cs

-                    return selectExpression.Projection
-                        .OfType<AliasExpression>()
-                        .SingleOrDefault(ae => Equals(ae.SourceMember, memberExpression.Member));
+                    return selectExpression.MemberInfoProjectionMapping[memberExpression.Member];


Can these lines be encapsulated into something like selectExpression?.GetMappedProjectionExpression(member)? Remove burden of ContainsKey from the caller, etc.

tuespetre · 2017-03-22T15:37:00Z

src/EFCore.Relational/Query/Expressions/ColumnExpression.cs

            Check.NotNull(tableExpression, nameof(tableExpression));

            Name = name;
-            Type = type;
+            _property = property;


Thank you 😭

tuespetre · 2017-03-22T15:44:24Z

src/EFCore.Relational/Query/Expressions/SelectExpression.cs

@@ -85,6 +90,8 @@ public class SelectExpression : TableExpressionBase
        /// </value>
        public virtual Expression Predicate { get; [param: CanBeNull] set; }

+        public virtual Dictionary<MemberInfo, Expression> MemberInfoProjectionMapping { get; private set; } = new Dictionary<MemberInfo, Expression>();


Make private readonly field and use methods to manipulate?

anpete · 2017-03-23T21:44:13Z

src/EFCore.Relational/Extensions/RelationalExpressionExtensions.cs

+            Check.NotNull(expression, nameof(expression));
+            Check.NotNull(table, nameof(table));
+
+            return expression is ColumnExpression columnExpression


Consider switch..case for clarity here.

anpete · 2017-03-23T21:46:48Z

src/EFCore.Relational/Query/ExpressionVisitors/Internal/IncludeExpressionVisitor.cs

+
+        private static IProperty TryGetProperty(Expression expression)
+        {
+            return expression is ColumnExpression columnExpression


Same here for switch..case

anpete · 2017-03-23T21:50:57Z

src/EFCore.Relational/Query/Expressions/ColumnReferenceExpression.cs

+        /// </returns>
+        public override bool Equals(object obj)
+        {
+            if (ReferenceEquals(null, obj))


Added everywhere needed. Resharper 👍

anpete · 2017-03-23T21:51:49Z

src/EFCore.Relational/Query/Expressions/ProjectStarExpression.cs

+        /// <param name="projections"> The list of existing expressions being used from this star projection. </param>
+        /// <param name="tableExpression"> The target table expression being represented by this star projection. </param>
+        public ProjectStarExpression(
+            [NotNull]List<Expression> projections,


anpete · 2017-03-23T21:53:23Z

src/EFCore.Relational/Query/Expressions/ProjectStarExpression.cs

+            return obj.GetType() == GetType() && Equals((ProjectStarExpression)obj);
+        }
+
+        private bool Equals([NotNull] ProjectStarExpression other) => _projections.Equals(other._projections) && _tableExpression.Equals(other._tableExpression);


Are you sure you want to use reference equality on _projections here? We typically do SequenceEquals here - same for GetHashCode below - aggregate the hash codes of the elements.

Might want to review any other possible instances of this.

smitpatel · 2017-03-25T00:10:09Z

Rebased.

smitpatel · 2017-03-27T22:17:19Z

Ping!

maumar · 2017-03-27T22:26:53Z

...FCore.Relational/Query/ExpressionVisitors/Internal/EqualityPredicateInExpressionOptimizer.cs

                {
-                    return binaryExpression.Right.TryGetColumnExpression()
-                           ?? binaryExpression.Left.TryGetColumnExpression();
+                    nonColumnExpression = isLeftConstantOrParameter ? left : right;


nit: add newline before return #Resolved

anpete · 2017-03-27T22:44:38Z

src/EFCore.SqlServer/Query/Expressions/Internal/RowNumberExpression.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>
+        public override int GetHashCode() => _orderings.GetHashCode();


Equals and GHC over collection

anpete · 2017-03-27T22:49:21Z

src/EFCore/Query/Internal/ExpressionEqualityComparer.cs

@@ -564,6 +563,9 @@ private static bool CompareMemberList(IReadOnlyList<MemberInfo> a, IReadOnlyList
            private bool CompareNewArray(NewArrayExpression a, NewArrayExpression b)
                => CompareExpressionList(a.Expressions, b.Expressions);

+            private bool CompareExtension(Expression a, Expression b)
+                => a.Equals(b);


I don't see Equals implemented on NullConditionalExpression

There are few expressions where Equals in not implemented. We can chat more about it in person.

Implemented Equals for all expressions of extension type except abstract classes & SelectExpression.

anpete · 2017-03-27T22:49:48Z

src/EFCore/Query/Internal/ExpressionEqualityComparer.cs

                        {
-                            goto default;
+                            hashCode += (hashCode * 397) ^ GetHashCode(nullConditionalExpression.AccessOperation);


Implement this on NullConditionalExpression

anpete · 2017-03-27T22:51:14Z

src/EFCore.Relational/Extensions/RelationalExpressionExtensions.cs

+                   || unwrappedExpression is AliasExpression;
+        }
+
+        public static ColumnReferenceExpression LiftExpressionFromSubquery([NotNull] this Expression expression, [NotNull] TableExpressionBase table)


Polymorphic method on SQL extension nodes? What happens in null case?

Lets talk in person.

For now only this types would need it. By design it should cover all cases and should not require anymore types to be evaluated.

maumar · 2017-03-28T17:58:30Z

src/EFCore/Query/Expressions/Internal/NullConditionalExpression.cs

+                    + string.Join(",", methodCallExpression.Arguments.Skip(1)) + ")";
+            }
+
+            return "?" + AccessOperation + "?";


use string interpolation

maumar · 2017-03-28T17:59:16Z

src/EFCore/Query/Expressions/Internal/NullConditionalExpression.cs

+                        + "?." + methodCallExpression.Method.Name
+                        + "(" + string.Join(",", methodCallExpression.Arguments) + ")";
+                }
+                var method = methodCallExpression.Method;


nit: newline after {, and before return

maumar · 2017-03-28T22:26:48Z

src/EFCore.Relational/Query/Expressions/ColumnExpression.cs

@@ -106,7 +86,7 @@ protected override Expression Accept(ExpressionVisitor visitor)
        }

        /// <summary>
-        ///     Reduces the node and then calls the <see cref="ExpressionVisitor.Visit(Expression)" /> method passing the
+        ///     Reduces the node and then calls the <see cref="ExpressionVisitor.Visit(System.Linq.Expressions.Expression)" /> method passing the


why fully qualified name here?

Some auto-formatting may have loved it.

smitpatel · 2017-03-28T23:39:51Z

Updated.

maumar · 2017-03-29T00:16:22Z

src/EFCore.Relational/Query/Expressions/SelectExpression.cs


            foreach (var expression in _projection)
            {
-                var aliasExpression = expression as AliasExpression;
+                var expressionToAdd = expression;
+                var memberInfo = _memberInfoProjectionMapping.FirstOrDefault(kvp => ExpressionEqualityComparer.Equals(kvp.Value, expression)).Key;


nit: this can be moved down

- No ad-hoc creation of ColumnExpression. When binding a property to SelectExpression, SelectExpression will generate appropriate expression for it. (BindPropertyToSelectExpression method which binds property even if it is not added to projection) - Removal of non-property ColumnExpression - ColumnExpression represent a column which is directly backed by EntityType.Property. For any other case, a different kind of expression should be used. - AliasExpression requires alias to be defined always. Hence unless column needs to aliased, it will not be made AliasExpression. - Implementation of Equals method of all extension expressions and using ExpressionEqualityComparer to compare expressions in Projection/OrderBy to avoid duplicates. This removes the complex logic in AddToProjection. - Removal of TryGetColumnExpression - All the places, which relied on assumption of having a column backed expression are changed to work with any kind of expression. - Introduction of ColumnReferenceExpression which can store reference to inner projected expression so that each expression knows its root inside the subqueries. - SelectExpression._starProjection stores list of expressions being used by outer SelectExpression. - Simplification of AddToProjection method - Method adds expression straight to projection. Bind method will create the expression to be added from property. Method also unique-fy alias if it is inside subquery. This method also update OrderBy list if a complex expression is getting added to projection and present in orderby by alias-ing it. - Removal of SourceMember/SourceExpression from AliasExpression, (which brought requirement of having every ColumnExpression as AliasExpression in projection). While visiting NewExpression in RelationalProjectionExpressionVisitor, we record SourceExpression to projected expression mapping and then we write MemberInfo to projected expression mapping in SelectExpression. Which is appropriately lifted during PushDownSubquery. This makes it possible for us to bind members after projecting into anonymous types. - Moved RelationalAnnotationsProvider to SelectExpression to find column name so that all the visitors trying to bind property with select expression does not need it to generate column name.

dnfclas added the cla-already-signed label Mar 22, 2017

smitpatel requested review from anpete and maumar March 22, 2017 03:01

smitpatel added breaking-change providers-beware labels Mar 22, 2017

tuespetre reviewed Mar 22, 2017

View reviewed changes

smitpatel force-pushed the thisnotgonnaendwell7520 branch 3 times, most recently from be8bf91 to 2d04e6b Compare March 22, 2017 18:31

anpete reviewed Mar 23, 2017

View reviewed changes

smitpatel force-pushed the thisnotgonnaendwell7520 branch 2 times, most recently from 04606e8 to 1467451 Compare March 23, 2017 23:56

anpete mentioned this pull request Mar 23, 2017

Query: List of refactoring ideas #7520

Closed

12 tasks

smitpatel force-pushed the thisnotgonnaendwell7520 branch 3 times, most recently from 8ba26c9 to bfb93f0 Compare March 25, 2017 00:10

maumar reviewed Mar 27, 2017

View reviewed changes

anpete reviewed Mar 27, 2017

View reviewed changes

smitpatel force-pushed the thisnotgonnaendwell7520 branch from bfb93f0 to bb66f16 Compare March 28, 2017 00:26

maumar reviewed Mar 28, 2017

View reviewed changes

smitpatel force-pushed the thisnotgonnaendwell7520 branch from bb66f16 to 2d66ab3 Compare March 28, 2017 19:35

maumar reviewed Mar 28, 2017

View reviewed changes

maumar reviewed Mar 29, 2017

View reviewed changes

smitpatel force-pushed the thisnotgonnaendwell7520 branch 2 times, most recently from 43cee32 to 3390b87 Compare March 29, 2017 17:18

anpete approved these changes Mar 29, 2017

View reviewed changes

smitpatel force-pushed the thisnotgonnaendwell7520 branch 3 times, most recently from b403876 to b16e1b2 Compare March 29, 2017 20:59

smitpatel force-pushed the thisnotgonnaendwell7520 branch from b16e1b2 to 111b095 Compare March 29, 2017 21:06

smitpatel merged commit 111b095 into dev Mar 29, 2017

smitpatel deleted the thisnotgonnaendwell7520 branch March 29, 2017 22:01

This was referenced Apr 4, 2017

Refactor RelationalProjectionExpressionVisitor #7767

Closed

Query: Relational: Fix issues with problematic Take/Skip queries #7823

Closed

Query: Avoid revisiting NewExpression in RelationalProjectionExpressi… #8072

Merged

This was referenced Apr 18, 2017

Query: Subquery comparison to null is translated incorrectly #6353

Closed

Add test for subquery null comparison translated correctly #8211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query: Refactor SelectExpression for referential integrity #7950

Query: Refactor SelectExpression for referential integrity #7950

smitpatel commented Mar 22, 2017 •

edited

Loading

tuespetre commented Mar 22, 2017

tuespetre left a comment

tuespetre Mar 22, 2017

tuespetre Mar 22, 2017

tuespetre Mar 22, 2017

tuespetre Mar 22, 2017

tuespetre Mar 22, 2017

tuespetre Mar 22, 2017

tuespetre Mar 22, 2017

anpete Mar 23, 2017

anpete Mar 23, 2017

anpete Mar 23, 2017

smitpatel Mar 23, 2017

anpete Mar 23, 2017

anpete Mar 23, 2017

anpete Mar 23, 2017

smitpatel commented Mar 25, 2017

smitpatel commented Mar 27, 2017

maumar Mar 27, 2017 •

edited by smitpatel

Loading

anpete Mar 27, 2017

anpete Mar 27, 2017

smitpatel Mar 28, 2017

smitpatel Mar 28, 2017

anpete Mar 27, 2017

anpete Mar 27, 2017

smitpatel Mar 27, 2017

smitpatel Mar 28, 2017

maumar Mar 28, 2017

maumar Mar 28, 2017

maumar Mar 28, 2017

smitpatel Mar 28, 2017

smitpatel commented Mar 28, 2017

maumar Mar 29, 2017

Query: Refactor SelectExpression for referential integrity #7950

Query: Refactor SelectExpression for referential integrity #7950

Conversation

smitpatel commented Mar 22, 2017 • edited Loading

tuespetre commented Mar 22, 2017

tuespetre 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Mar 25, 2017

smitpatel commented Mar 27, 2017

maumar Mar 27, 2017 • edited by smitpatel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Mar 28, 2017

Choose a reason for hiding this comment

smitpatel commented Mar 22, 2017 •

edited

Loading

maumar Mar 27, 2017 •

edited by smitpatel

Loading