Query: Refactor SearchConditionTranslatingVisitor #7554

smitpatel · 2017-02-04T03:06:06Z

Resolves issue #7271
part of #7520

maumar · 2017-02-04T03:26:40Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

            }

            protected override Expression VisitBinary(BinaryExpression expression)
            {
-                Expression newLeft;
-                Expression newRight;
+                // Visit the children


remove this comment - seems inaccurate (visit children comes later)

maumar · 2017-02-04T03:27:30Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs


-                newLeft = AdjustExpressionType(Visit(expression.Left), expression.Left.Type);
-                newRight = AdjustExpressionType(Visit(expression.Right), expression.Right.Type);
+                // Update the node


remove this comment as seems redundant - fairly obvious what the code is doing here

maumar · 2017-02-04T03:29:12Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs


-                return expression.Update(newLeft, expression.Conversion, newRight);
+                // Do conversion
+                return _isSearchCondition


This pattern in repeated several times, consider DRYing it up.

maumar · 2017-02-04T03:29:28Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

            protected override Expression VisitConditional(ConditionalExpression expression)
            {
+                // Visit the children


remove comment

maumar · 2017-02-04T03:29:38Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

                var ifTrue = Visit(expression.IfTrue);
                var ifFalse = Visit(expression.IfFalse);

                _isSearchCondition = parentIsSearchCondition;

-                var newExpression = Expression.Condition(test, ifTrue, ifFalse);
+                // Update the node


remove comment

maumar · 2017-02-04T03:29:52Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

                }

-                if (expression.NodeType == ExpressionType.Not)
+                // Visit the children


remove comment

maumar · 2017-02-04T03:31:07Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

-                            expression.Operand,
-                            Expression.Constant(false, expression.Operand.Type));
-                    }
+                // Update the node


remove comment

maumar · 2017-02-04T03:31:30Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

-                if (_isSearchCondition)
-                {
-                    var parentIsSearchCondition = _isSearchCondition;
+                // Visit the children


remove comment

maumar · 2017-02-04T03:37:09Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

            }

-            protected override Expression VisitParameter(ParameterExpression expression)
+            private static Expression MakeCompareToExpression(Expression expression, bool compareTo)


nit: maybe 'Build' or 'Create' instead of 'Make'

anpete · 2017-02-04T03:50:24Z

Does it make sense to move from "SearchCondition" to "Predicate" for consistency?

anpete · 2017-02-04T03:48:48Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

-
-                return Expression.Equal(newExpression, Expression.Constant(true, typeof(bool)));
-            }
+            newExpression = new SearchConditionTranslatingVisitor(searchCondition).Convert(newExpression);


Can we call Visit instead on Convert?

I find the naming confusing. Is it that this visitor is a predicate translator that performs a different translation when the target expression is a search condition?

I also need suggestions for name. Visit would be starting method for every expression. But the code block should be applied only at top level or starting expression. I moved the extra handling at top level inside the visitor to make it more contained in itself.

How about Translate as method name?

This is visitor which converts value to search condition or vice-versa as needed. It is applied to predicates/join conditions & projections. After thinking more about name of the visitor,
I believe it shouldn't be named with Predicate.

Translate is fine. I was looking for symmetry with the 3 lines above, but I see why it needs a custom entry point method.

How about something like:

new BooleanExpressionTranslatingVisitor().Translate(expression, searchCondition: false);

anpete · 2017-02-04T03:51:06Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

@@ -1623,245 +1607,219 @@ var constantExpression

        private class SearchConditionTranslatingVisitor : RelinqExpressionVisitor
        {
+            /*


Put this in /// Summary doc comment

smitpatel · 2017-02-04T05:42:43Z

It was named "SearchCondition" because T-Sql uses that term. I am fine with using "Predicate" or anything else also.

anpete · 2017-02-07T19:28:25Z

src/Microsoft.EntityFrameworkCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

-
-                return Expression.Equal(newExpression, Expression.Constant(true, typeof(bool)));
-            }
+            newExpression = new SearchConditionTranslatingVisitor(searchCondition).Convert(newExpression);


Translate is fine. I was looking for symmetry with the 3 lines above, but I see why it needs a custom entry point method.

How about something like:

new BooleanExpressionTranslatingVisitor().Translate(expression, searchCondition: false);

Resolves #7271

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

smitpatel added the needs-review label Feb 4, 2017

smitpatel force-pushed the searchConditionRefactor branch from 7d78c88 to f89471d Compare February 4, 2017 03:06

maumar reviewed Feb 4, 2017

View reviewed changes

anpete reviewed Feb 4, 2017

View reviewed changes

smitpatel force-pushed the searchConditionRefactor branch from f89471d to f8a2918 Compare February 4, 2017 05:55

anpete approved these changes Feb 7, 2017

View reviewed changes

anpete removed the needs-review label Feb 7, 2017

smitpatel force-pushed the searchConditionRefactor branch 2 times, most recently from b2ab380 to 98a8049 Compare February 7, 2017 20:39

Query: Refactor SearchConditionTranslatingVisitor

1649ad0

Resolves #7271

smitpatel force-pushed the searchConditionRefactor branch from 98a8049 to 1649ad0 Compare February 7, 2017 21:05

smitpatel merged commit 1649ad0 into dev Feb 7, 2017

smitpatel deleted the searchConditionRefactor branch February 7, 2017 21:29

anpete mentioned this pull request Feb 7, 2017

Query: List of refactoring ideas #7520

Closed

12 tasks

smitpatel mentioned this pull request Jun 14, 2017

OrderBy NULL value throws "Incorrect syntax near the keyword IS NULL" error #8591

Closed

smitpatel mentioned this pull request Jul 13, 2017

Query: Error using a compare with a nullable boolean (again) #9169

Closed

smitpatel mentioned this pull request Jul 24, 2017

Ternary expression and let with external boolean variable with false value produces invalid SQL. #9247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query: Refactor SearchConditionTranslatingVisitor #7554

Query: Refactor SearchConditionTranslatingVisitor #7554

smitpatel commented Feb 4, 2017 •

edited

Loading

maumar Feb 4, 2017

maumar Feb 4, 2017

maumar Feb 4, 2017

maumar Feb 4, 2017 •

edited

Loading

maumar Feb 4, 2017 •

edited

Loading

maumar Feb 4, 2017

maumar Feb 4, 2017

maumar Feb 4, 2017

maumar Feb 4, 2017

anpete commented Feb 4, 2017 •

edited

Loading

anpete Feb 4, 2017

anpete Feb 4, 2017

smitpatel Feb 4, 2017

smitpatel Feb 4, 2017

anpete Feb 7, 2017

anpete Feb 4, 2017

smitpatel commented Feb 4, 2017

anpete Feb 7, 2017

Query: Refactor SearchConditionTranslatingVisitor #7554

Query: Refactor SearchConditionTranslatingVisitor #7554

Conversation

smitpatel commented Feb 4, 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 Feb 4, 2017 • edited Loading

Choose a reason for hiding this comment

maumar Feb 4, 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

anpete commented Feb 4, 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

smitpatel commented Feb 4, 2017

Choose a reason for hiding this comment

smitpatel commented Feb 4, 2017 •

edited

Loading

maumar Feb 4, 2017 •

edited

Loading

maumar Feb 4, 2017 •

edited

Loading

anpete commented Feb 4, 2017 •

edited

Loading