Query: Remove implicit convert nodes in query to avoid unnecessary ca…

…st in generated SQL Enable BuiltInDataTypeTests - Preserve convert nodes around parameters/constants in parameter extracting. - While translating to Sql, remove implicit convert nodes. Implicit convert nodes appear only for binary expressions since equality operators are not defined for all types - Apply explicit cast in SQL only when converted type is mapped. - Convert int value to enum value before printing literal (how old pipeline did it) Resolves #14159 Resolves #15330 Resolves #15948
dotnet · Jun 5, 2019 · 0a156d3 · 0a156d3
1 parent db9bca3
commit 0a156d3
Show file tree

Hide file tree

Showing 21 changed files with 212 additions and 89 deletions.
diff --git a/src/EFCore.Relational/Query/Pipeline/ISqlExpressionFactory.cs b/src/EFCore.Relational/Query/Pipeline/ISqlExpressionFactory.cs
@@ -16,6 +16,7 @@ public interface ISqlExpressionFactory
         SqlExpression ApplyTypeMapping(SqlExpression sqlExpression, RelationalTypeMapping typeMapping);
         SqlExpression ApplyDefaultTypeMapping(SqlExpression sqlExpression);
         RelationalTypeMapping GetTypeMappingForValue(object value);
+        RelationalTypeMapping FindMapping(Type type);
         #endregion
 
         #region Binary

diff --git a/src/EFCore.Relational/Query/Pipeline/RelationalSqlTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/Pipeline/RelationalSqlTranslatingExpressionVisitor.cs
@@ -2,10 +2,12 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
+using System.Runtime.InteropServices.ComTypes;
 using Microsoft.EntityFrameworkCore.Extensions.Internal;
 using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Metadata.Internal;
@@ -209,10 +211,43 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
             return _methodCallTranslatorProvider.Translate(_model, (SqlExpression)@object, methodCallExpression.Method, arguments);
         }
 
+        private static Expression TryRemoveImplicitConvert(Expression expression)
+        {
+            if (expression is UnaryExpression unaryExpression)
+            {
+                if (unaryExpression.NodeType == ExpressionType.Convert
+                    || unaryExpression.NodeType == ExpressionType.ConvertChecked)
+                {
+                    var innerType = unaryExpression.Operand.Type.UnwrapNullableType();
+                    if (innerType.IsEnum)
+                    {
+                        innerType = Enum.GetUnderlyingType(innerType);
+                    }
+                    var convertedType = unaryExpression.Type.UnwrapNullableType();
+
+                    if (innerType == convertedType
+                        || (convertedType == typeof(int)
+                            && (innerType == typeof(byte)
+                                || innerType == typeof(sbyte)
+                                || innerType == typeof(char)
+                                || innerType == typeof(short)
+                                || innerType == typeof(ushort))))
+                    {
+                        return TryRemoveImplicitConvert(unaryExpression.Operand);
+                    }
+                }
+            }
+
+            return expression;
+        }
+
         protected override Expression VisitBinary(BinaryExpression binaryExpression)
         {
-            var left = Visit(binaryExpression.Left);
-            var right = Visit(binaryExpression.Right);
+            var left = TryRemoveImplicitConvert(binaryExpression.Left);
+            var right = TryRemoveImplicitConvert(binaryExpression.Right);
+
+            left = Visit(left);
+            right = Visit(right);
 
             if (TranslationFailed(binaryExpression.Left, left)
                 || TranslationFailed(binaryExpression.Right, right))
@@ -331,9 +366,17 @@ protected override Expression VisitUnary(UnaryExpression unaryExpression)
                     {
                         return sqlOperand;
                     }
-                    sqlOperand = _sqlExpressionFactory.ApplyDefaultTypeMapping(sqlOperand);
 
-                    return _sqlExpressionFactory.Convert(sqlOperand, unaryExpression.Type);
+                    // Introduce explicit cast only if the target type is mapped else we need to client eval
+                    if (unaryExpression.Type == typeof(object)
+                        || _sqlExpressionFactory.FindMapping(unaryExpression.Type) != null)
+                    {
+                        sqlOperand = _sqlExpressionFactory.ApplyDefaultTypeMapping(sqlOperand);
+
+                        return _sqlExpressionFactory.Convert(sqlOperand, unaryExpression.Type);
+                    }
+
+                    break;
             }
 
             return null;

diff --git a/src/EFCore.Relational/Query/Pipeline/SqlExpressionFactory.cs b/src/EFCore.Relational/Query/Pipeline/SqlExpressionFactory.cs
@@ -205,6 +205,11 @@ public virtual RelationalTypeMapping GetTypeMappingForValue(object value)
         {
             return _typeMappingSource.GetMappingForValue(value);
         }
+
+        public virtual RelationalTypeMapping FindMapping(Type type)
+        {
+            return _typeMappingSource.FindMapping(type);
+        }
         #endregion
 
         #region Binary

diff --git a/src/EFCore.Relational/Storage/RelationalTypeMapping.cs b/src/EFCore.Relational/Storage/RelationalTypeMapping.cs
@@ -500,6 +500,12 @@ public virtual string GenerateSqlLiteral([CanBeNull] object value)
         {
             if (Converter != null)
             {
+                if (value?.GetType().IsInteger() == true
+                    && ClrType.UnwrapNullableType().IsEnum)
+                {
+                    value = Enum.ToObject(ClrType.UnwrapNullableType(), value);
+                }
+
                 value = Converter.ConvertToProvider(value);
             }
 

diff --git a/src/EFCore/Query/Pipeline/ParameterExtractingExpressionVisitor.cs b/src/EFCore/Query/Pipeline/ParameterExtractingExpressionVisitor.cs
@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections;
 using System.Collections.Generic;
 using System.Linq;
 using System.Linq.Expressions;
@@ -31,7 +32,6 @@ public class ParameterExtractingExpressionVisitor : ExpressionVisitor
         private readonly bool _generateContextAccessors;
         private readonly EvaluatableExpressionFindingExpressionVisitor _evaluatableExpressionFindingExpressionVisitor;
         private readonly ContextParameterReplacingExpressionVisitor _contextParameterReplacingExpressionVisitor;
-
         private readonly Dictionary<Expression, Expression> _evaluatedValues
             = new Dictionary<Expression, Expression>(ExpressionEqualityComparer.Instance);
 
@@ -122,15 +122,19 @@ protected virtual bool PreserveConvertNode(Expression expression)
                     || unaryExpression.NodeType == ExpressionType.ConvertChecked))
             {
                 if (unaryExpression.Type == typeof(object)
-                    || unaryExpression.Type == typeof(Enum))
+                    || unaryExpression.Type == typeof(Enum)
+                    || unaryExpression.Operand.Type.UnwrapNullableType().IsEnum)
                 {
                     return true;
                 }
 
+                var innerType = unaryExpression.Operand.Type.UnwrapNullableType();
                 if (unaryExpression.Type.UnwrapNullableType() == typeof(int)
-                    && (unaryExpression.Operand.Type.UnwrapNullableType().IsEnum
-                        || unaryExpression.Operand.Type.UnwrapNullableType() == typeof(char)
-                        || unaryExpression.Operand.Type.UnwrapNullableType() == typeof(ushort)))
+                    && (innerType == typeof(byte)
+                        || innerType == typeof(sbyte)
+                        || innerType == typeof(char)
+                        || innerType == typeof(short)
+                        || innerType == typeof(ushort)))
                 {
                     return true;
                 }

diff --git a/test/EFCore.Cosmos.FunctionalTests/BuiltInDataTypesCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/BuiltInDataTypesCosmosTest.cs
@@ -45,6 +45,8 @@ public class BuiltInDataTypesCosmosFixture : BuiltInDataTypesFixtureBase
 
             public override bool SupportsBinaryKeys => true;
 
+            public override bool SupportsDecimalComparisons => true;
+
             public override DateTime DefaultDateTime => new DateTime();
 
             protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)

diff --git a/test/EFCore.Cosmos.FunctionalTests/CustomConvertersCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/CustomConvertersCosmosTest.cs
@@ -49,6 +49,8 @@ public class CustomConvertersCosmosFixture : CustomConvertersFixtureBase
 
             public override bool SupportsBinaryKeys => true;
 
+            public override bool SupportsDecimalComparisons => true;
+
             public override DateTime DefaultDateTime => new DateTime();
 
             protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)

diff --git a/test/EFCore.InMemory.FunctionalTests/BuiltInDataTypesInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/BuiltInDataTypesInMemoryTest.cs
@@ -3,6 +3,7 @@
 
 using System;
 using Microsoft.EntityFrameworkCore.TestUtilities;
+using Microsoft.EntityFrameworkCore.TestUtilities.Xunit;
 
 // ReSharper disable InconsistentNaming
 namespace Microsoft.EntityFrameworkCore
@@ -14,6 +15,12 @@ public BuiltInDataTypesInMemoryTest(BuiltInDataTypesInMemoryFixture fixture)
         {
         }
 
+        [ConditionalFact(Skip = "Issue#15711")]
+        public override void Can_insert_and_read_back_with_string_key()
+        {
+            base.Can_insert_and_read_back_with_string_key();
+        }
+
         public class BuiltInDataTypesInMemoryFixture : BuiltInDataTypesFixtureBase
         {
             protected override ITestStoreFactory TestStoreFactory => InMemoryTestStoreFactory.Instance;
@@ -28,6 +35,8 @@ public class BuiltInDataTypesInMemoryFixture : BuiltInDataTypesFixtureBase
 
             public override bool SupportsBinaryKeys => false;
 
+            public override bool SupportsDecimalComparisons => true;
+
             public override DateTime DefaultDateTime => new DateTime();
         }
     }

diff --git a/test/EFCore.InMemory.FunctionalTests/ConvertToProviderTypesInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/ConvertToProviderTypesInMemoryTest.cs
@@ -3,6 +3,7 @@
 
 using System;
 using Microsoft.EntityFrameworkCore.TestUtilities;
+using Microsoft.EntityFrameworkCore.TestUtilities.Xunit;
 
 namespace Microsoft.EntityFrameworkCore
 {
@@ -14,6 +15,12 @@ public ConvertToProviderTypesInMemoryTest(ConvertToProviderTypesInMemoryFixture
         {
         }
 
+        [ConditionalFact(Skip = "Issue#15711")]
+        public override void Can_insert_and_read_back_with_string_key()
+        {
+            base.Can_insert_and_read_back_with_string_key();
+        }
+
         public class ConvertToProviderTypesInMemoryFixture : ConvertToProviderTypesFixtureBase
         {
             protected override ITestStoreFactory TestStoreFactory => InMemoryTestStoreFactory.Instance;
@@ -28,6 +35,8 @@ public class ConvertToProviderTypesInMemoryFixture : ConvertToProviderTypesFixtu
 
             public override bool SupportsBinaryKeys => false;
 
+            public override bool SupportsDecimalComparisons => true;
+
             public override DateTime DefaultDateTime => new DateTime();
         }
     }

diff --git a/test/EFCore.InMemory.FunctionalTests/CustomConvertersInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/CustomConvertersInMemoryTest.cs
@@ -3,6 +3,7 @@
 
 using System;
 using Microsoft.EntityFrameworkCore.TestUtilities;
+using Microsoft.EntityFrameworkCore.TestUtilities.Xunit;
 
 namespace Microsoft.EntityFrameworkCore
 {
@@ -18,6 +19,12 @@ public override void Can_insert_and_read_back_with_case_insensitive_string_key()
         {
         }
 
+        [ConditionalFact(Skip = "Issue#15711")]
+        public override void Can_insert_and_read_back_with_string_key()
+        {
+            base.Can_insert_and_read_back_with_string_key();
+        }
+
         public class CustomConvertersInMemoryFixture : CustomConvertersFixtureBase
         {
             public override bool StrictEquality => true;
@@ -32,6 +39,8 @@ public class CustomConvertersInMemoryFixture : CustomConvertersFixtureBase
 
             public override bool SupportsBinaryKeys => false;
 
+            public override bool SupportsDecimalComparisons => true;
+
             public override DateTime DefaultDateTime => new DateTime();
         }
     }