Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query: Remove implicit convert nodes in query to avoid unnecessary cast in generated SQL #15954

Merged
merged 1 commit into from
Jun 5, 2019
Merged

Query: Remove implicit convert nodes in query to avoid unnecessary cast in generated SQL #15954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/EFCore.Relational/Query/Pipeline/ISqlExpressionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
51 changes: 47 additions & 4 deletions src/EFCore.Relational/Query/Pipeline/RelationalSqlTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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;
Expand Down
5 changes: 5 additions & 0 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions src/EFCore.Relational/Storage/RelationalTypeMapping.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
14 changes: 9 additions & 5 deletions src/EFCore/Query/Pipeline/ParameterExtractingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 2 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/BuiltInDataTypesCosmosTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/CustomConvertersCosmosTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 9 additions & 0 deletions test/EFCore.InMemory.FunctionalTests/BuiltInDataTypesInMemoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Microsoft.EntityFrameworkCore.TestUtilities.Xunit;

// ReSharper disable InconsistentNaming
namespace Microsoft.EntityFrameworkCore
Expand All @@ -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;
Expand All @@ -28,6 +35,8 @@ public class BuiltInDataTypesInMemoryFixture : BuiltInDataTypesFixtureBase

public override bool SupportsBinaryKeys => false;

public override bool SupportsDecimalComparisons => true;

public override DateTime DefaultDateTime => new DateTime();
}
}
Expand Down
9 changes: 9 additions & 0 deletions test/EFCore.InMemory.FunctionalTests/ConvertToProviderTypesInMemoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Microsoft.EntityFrameworkCore.TestUtilities.Xunit;

namespace Microsoft.EntityFrameworkCore
{
Expand All @@ -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;
Expand All @@ -28,6 +35,8 @@ public class ConvertToProviderTypesInMemoryFixture : ConvertToProviderTypesFixtu

public override bool SupportsBinaryKeys => false;

public override bool SupportsDecimalComparisons => true;

public override DateTime DefaultDateTime => new DateTime();
}
}
Expand Down
9 changes: 9 additions & 0 deletions test/EFCore.InMemory.FunctionalTests/CustomConvertersInMemoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Microsoft.EntityFrameworkCore.TestUtilities.Xunit;

namespace Microsoft.EntityFrameworkCore
{
Expand All @@ -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;
Expand All @@ -32,6 +39,8 @@ public class CustomConvertersInMemoryFixture : CustomConvertersFixtureBase

public override bool SupportsBinaryKeys => false;

public override bool SupportsDecimalComparisons => true;

public override DateTime DefaultDateTime => new DateTime();
}
}
Expand Down
Loading