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

Fixing some small bugs in sql generation #16112

Merged
merged 1 commit into from
Jun 19, 2019
Merged

Fixing some small bugs in sql generation #16112

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
10 changes: 7 additions & 3 deletions src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,21 @@ public override Expression Visit(Expression expression)
: null;

var nullCheckExpression = hasNullValue
? _sqlExpressionFactory.IsNull(inExpression.Item)
? inExpression.Negated
? _sqlExpressionFactory.IsNotNull(inExpression.Item)
: _sqlExpressionFactory.IsNull(inExpression.Item)
: null;

if (updatedInExpression != null && nullCheckExpression != null)
{
return _sqlExpressionFactory.OrElse(updatedInExpression, nullCheckExpression);
return inExpression.Negated
? _sqlExpressionFactory.AndAlso(updatedInExpression, nullCheckExpression)
: _sqlExpressionFactory.OrElse(updatedInExpression, nullCheckExpression);
}

if (updatedInExpression == null && nullCheckExpression == null)
{
return _sqlExpressionFactory.Equal(_sqlExpressionFactory.Constant(true), _sqlExpressionFactory.Constant(false));
return _sqlExpressionFactory.Equal(_sqlExpressionFactory.Constant(true), _sqlExpressionFactory.Constant(inExpression.Negated));
}

return (SqlExpression)updatedInExpression ?? nullCheckExpression;
Expand Down
11 changes: 5 additions & 6 deletions src/EFCore.Relational/Query/Pipeline/RelationalShapedQueryOptimizingExpressionVisitors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,16 @@ namespace Microsoft.EntityFrameworkCore.Relational.Query.Pipeline
{
public class RelationalShapedQueryOptimizer : ShapedQueryOptimizer
{
private readonly QueryCompilationContext _queryCompilationContext;

public RelationalShapedQueryOptimizer(
QueryCompilationContext queryCompilationContext,
ISqlExpressionFactory sqlExpressionFactory)
{
_queryCompilationContext = queryCompilationContext;
UseRelationalNulls = RelationalOptionsExtension.Extract(queryCompilationContext.ContextOptions).UseRelationalNulls;
SqlExpressionFactory = sqlExpressionFactory;
}

protected ISqlExpressionFactory SqlExpressionFactory { get; private set; }
protected ISqlExpressionFactory SqlExpressionFactory { get; }
protected bool UseRelationalNulls { get; }

public override Expression Visit(Expression query)
{
Expand All @@ -29,12 +28,12 @@ public override Expression Visit(Expression query)
query = new CollectionJoinApplyingExpressionVisitor().Visit(query);
query = new SelectExpressionTableAliasUniquifyingExpressionVisitor().Visit(query);

if (!RelationalOptionsExtension.Extract(_queryCompilationContext.ContextOptions).UseRelationalNulls)
if (!UseRelationalNulls)
{
query = new NullSemanticsRewritingVisitor(SqlExpressionFactory).Visit(query);
}

query = new SqlExpressionOptimizingVisitor(SqlExpressionFactory).Visit(query);
query = new SqlExpressionOptimizingVisitor(SqlExpressionFactory, UseRelationalNulls).Visit(query);
query = new NullComparisonTransformingExpressionVisitor().Visit(query);

if (query is ShapedQueryExpression shapedQueryExpression)
Expand Down
106 changes: 73 additions & 33 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressionOptimizingVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace Microsoft.EntityFrameworkCore.Query.Pipeline
public class SqlExpressionOptimizingVisitor : ExpressionVisitor
{
private readonly ISqlExpressionFactory _sqlExpressionFactory;
private readonly bool _useRelationalNulls;

private static bool TryNegate(ExpressionType expressionType, out ExpressionType result)
{
Expand All @@ -29,9 +30,10 @@ private static bool TryNegate(ExpressionType expressionType, out ExpressionType
return negated.HasValue;
}

public SqlExpressionOptimizingVisitor(ISqlExpressionFactory sqlExpressionFactory)
public SqlExpressionOptimizingVisitor(ISqlExpressionFactory sqlExpressionFactory, bool useRelationalNulls)
{
_sqlExpressionFactory = sqlExpressionFactory;
_useRelationalNulls = useRelationalNulls;
}

protected override Expression VisitExtension(Expression extensionExpression)
Expand All @@ -51,15 +53,9 @@ protected override Expression VisitExtension(Expression extensionExpression)

private Expression VisitSqlUnaryExpression(SqlUnaryExpression sqlUnaryExpression)
{
// !(true) -> false
// !(false) -> true
if (sqlUnaryExpression.OperatorType == ExpressionType.Not
&& sqlUnaryExpression.Operand is SqlConstantExpression innerConstantBool
&& innerConstantBool.Value is bool value)
if (sqlUnaryExpression.OperatorType == ExpressionType.Not)
{
return value
? _sqlExpressionFactory.Constant(false, sqlUnaryExpression.TypeMapping)
: _sqlExpressionFactory.Constant(true, sqlUnaryExpression.TypeMapping);
return VisitNot(sqlUnaryExpression);
}

// NULL IS NULL -> true
Expand All @@ -80,27 +76,6 @@ private Expression VisitSqlUnaryExpression(SqlUnaryExpression sqlUnaryExpression

if (sqlUnaryExpression.Operand is SqlUnaryExpression innerUnary)
{
if (sqlUnaryExpression.OperatorType == ExpressionType.Not)
{
// !(!a) -> a
if (innerUnary.OperatorType == ExpressionType.Not)
{
return Visit(innerUnary.Operand);
}

if (innerUnary.OperatorType == ExpressionType.Equal)
{
//!(a IS NULL) -> a IS NOT NULL
return Visit(_sqlExpressionFactory.IsNotNull(innerUnary.Operand));
}

//!(a IS NOT NULL) -> a IS NULL
if (innerUnary.OperatorType == ExpressionType.NotEqual)
{
return Visit(_sqlExpressionFactory.IsNull(innerUnary.Operand));
}
}

// (!a) IS NULL <==> a IS NULL
if (sqlUnaryExpression.OperatorType == ExpressionType.Equal
&& innerUnary.OperatorType == ExpressionType.Not)
Expand All @@ -116,6 +91,47 @@ private Expression VisitSqlUnaryExpression(SqlUnaryExpression sqlUnaryExpression
}
}

var newOperand = (SqlExpression)Visit(sqlUnaryExpression.Operand);

return sqlUnaryExpression.Update(newOperand);
}

private Expression VisitNot(SqlUnaryExpression sqlUnaryExpression)
{
// !(true) -> false
// !(false) -> true
if (sqlUnaryExpression.Operand is SqlConstantExpression innerConstantBool
&& innerConstantBool.Value is bool value)
{
return _sqlExpressionFactory.Constant(!value, sqlUnaryExpression.TypeMapping);
}

if (sqlUnaryExpression.Operand is InExpression inExpression)
{
return Visit(inExpression.Negate());
}

if (sqlUnaryExpression.Operand is SqlUnaryExpression innerUnary)
{
// !(!a) -> a
if (innerUnary.OperatorType == ExpressionType.Not)
{
return Visit(innerUnary.Operand);
}

if (innerUnary.OperatorType == ExpressionType.Equal)
{
//!(a IS NULL) -> a IS NOT NULL
return Visit(_sqlExpressionFactory.IsNotNull(innerUnary.Operand));
}

//!(a IS NOT NULL) -> a IS NULL
if (innerUnary.OperatorType == ExpressionType.NotEqual)
{
return Visit(_sqlExpressionFactory.IsNull(innerUnary.Operand));
}
}

if (sqlUnaryExpression.Operand is SqlBinaryExpression innerBinary)
{
// De Morgan's
Expand All @@ -130,11 +146,11 @@ private Expression VisitSqlUnaryExpression(SqlUnaryExpression sqlUnaryExpression
: _sqlExpressionFactory.AndAlso(newLeft, newRight);
}

// note that those optimizations are only valid in 2-value logic
// those optimizations are only valid in 2-value logic
// they are safe to do here because null semantics removes possibility of nulls in the tree
// however if we decide to do "partial" null semantics (that doesn't distinguish between NULL and FALSE, e.g. for predicates)
// we need to be extra careful here
if (TryNegate(innerBinary.OperatorType, out var negated))
if (!_useRelationalNulls && TryNegate(innerBinary.OperatorType, out var negated))
{
return Visit(
_sqlExpressionFactory.MakeBinary(
Expand All @@ -158,7 +174,6 @@ private Expression VisitSqlBinaryExpression(SqlBinaryExpression sqlBinaryExpress
if (sqlBinaryExpression.OperatorType == ExpressionType.AndAlso
|| sqlBinaryExpression.OperatorType == ExpressionType.OrElse)
{

var newLeftConstant = newLeft as SqlConstantExpression;
var newRightConstant = newRight as SqlConstantExpression;

Expand Down Expand Up @@ -194,6 +209,31 @@ private Expression VisitSqlBinaryExpression(SqlBinaryExpression sqlBinaryExpress
return sqlBinaryExpression.Update(newLeft, newRight);
}

// those optimizations are only valid in 2-value logic
// they are safe to do here because null semantics removes possibility of nulls in the tree
// however if we decide to do "partial" null semantics (that doesn't distinguish between NULL and FALSE, e.g. for predicates)
// we need to be extra careful here
if (!_useRelationalNulls
&& (sqlBinaryExpression.OperatorType == ExpressionType.Equal || sqlBinaryExpression.OperatorType == ExpressionType.NotEqual))
{
// op(a, b) == true -> op(a, b)
// op(a, b) != false -> op(a, b)
// op(a, b) == false -> !op(a, b)
// op(a, b) != true -> !op(a, b)
var constant = sqlBinaryExpression.Left as SqlConstantExpression ?? sqlBinaryExpression.Right as SqlConstantExpression;
var binary = sqlBinaryExpression.Left as SqlBinaryExpression ?? sqlBinaryExpression.Right as SqlBinaryExpression;
smitpatel marked this conversation as resolved.
Show resolved Hide resolved
if (constant != null && binary != null && TryNegate(binary.OperatorType, out var negated))
{
return (bool)constant.Value == (sqlBinaryExpression.OperatorType == ExpressionType.Equal)
? binary
: _sqlExpressionFactory.MakeBinary(
negated,
sqlBinaryExpression.Left,
sqlBinaryExpression.Right,
sqlBinaryExpression.TypeMapping);
}
}

return sqlBinaryExpression.Update(newLeft, newRight);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/EFCore.SqlServer/Query/Pipeline/SqlServerShapedQueryOptimizingExpressionVisitors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public override Expression Visit(Expression query)
{
query = base.Visit(query);
query = new SearchConditionConvertingExpressionVisitor(SqlExpressionFactory).Visit(query);
query = new SqlExpressionOptimizingVisitor(SqlExpressionFactory, UseRelationalNulls).Visit(query);

return query;
}
Expand Down
20 changes: 19 additions & 1 deletion src/EFCore.SqlServer/Query/Pipeline/SqlServerStringMethodTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,28 @@ public SqlExpression Translate(SqlExpression instance, MethodInfo method, IList<
{
var pattern = arguments[0];
var stringTypeMapping = ExpressionExtensions.InferTypeMapping(instance, pattern);

instance = _sqlExpressionFactory.ApplyTypeMapping(instance, stringTypeMapping);
pattern = _sqlExpressionFactory.ApplyTypeMapping(pattern, stringTypeMapping);

if (pattern is SqlConstantExpression constantPattern)
{
if ((string)constantPattern.Value == string.Empty)
{
return _sqlExpressionFactory.Constant(true);
}

return _sqlExpressionFactory.GreaterThan(
_sqlExpressionFactory.Function(
"CHARINDEX",
new[]
{
pattern,
instance
},
typeof(int)),
_sqlExpressionFactory.Constant(0));
}

return _sqlExpressionFactory.OrElse(
_sqlExpressionFactory.Equal(
pattern,
Expand Down
14 changes: 7 additions & 7 deletions src/EFCore/Query/Pipeline/EntityEqualityRewritingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ protected virtual Expression VisitOrderingMethodCall(MethodCallExpression method
}

var genericMethodDefinition = methodCallExpression.Method.GetGenericMethodDefinition();
var isFirstOrdering =
var firstOrdering =
genericMethodDefinition == LinqMethodHelpers.QueryableOrderByMethodInfo
|| genericMethodDefinition == LinqMethodHelpers.QueryableOrderByDescendingMethodInfo;
var isAscending =
Expand All @@ -336,28 +336,28 @@ protected virtual Expression VisitOrderingMethodCall(MethodCallExpression method
body.CreateEFPropertyExpression(keyProperty, makeNullable: false)),
param);

var orderingMethodInfo = GetOrderingMethodInfo(isFirstOrdering, isAscending);
var orderingMethodInfo = GetOrderingMethodInfo(firstOrdering, isAscending);

expression = Expression.Call(
orderingMethodInfo.MakeGenericMethod(entityType.ClrType, keyProperty.ClrType),
expression,
rewrittenKeySelector
);

isFirstOrdering = false;
firstOrdering = false;
}

return expression;

static MethodInfo GetOrderingMethodInfo(bool isFirstOrdering, bool isAscending)
static MethodInfo GetOrderingMethodInfo(bool firstOrdering, bool ascending)
{
if (isFirstOrdering)
if (firstOrdering)
{
return isAscending
return ascending
? LinqMethodHelpers.QueryableOrderByMethodInfo
: LinqMethodHelpers.QueryableOrderByDescendingMethodInfo;
}
return isAscending
return ascending
? LinqMethodHelpers.QueryableThenByMethodInfo
: LinqMethodHelpers.QueryableThenByDescendingMethodInfo;
}
Expand Down
Loading