Skip to content

Commit

Permalink
Fixing some small bugs in sql generation
Browse files Browse the repository at this point in the history 
- removing redundant term in Contains translation with constant argument,
- fixing logic around IN sql gen,
- clean up logic around de morgan optimization,
- adding new optimization op(a, b) == true -> op(a, b),
- removing a number of tests that are no longer relevant due to query pipeline changes.
  • Loading branch information
@maumar
maumar committed Jun 15, 2019
1 parent 48480d7 commit 37643f7
Show file tree
Hide file tree
Showing 15 changed files with 441 additions and 427 deletions.
12 changes: 9 additions & 3 deletions src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,23 @@ 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 inExpression.Negated
? _sqlExpressionFactory.Equal(_sqlExpressionFactory.Constant(true), _sqlExpressionFactory.Constant(true))
: _sqlExpressionFactory.Equal(_sqlExpressionFactory.Constant(true), _sqlExpressionFactory.Constant(false));
}

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

public RelationalShapedQueryOptimizer(
QueryCompilationContext queryCompilationContext,
ISqlExpressionFactory sqlExpressionFactory)
{
_queryCompilationContext = queryCompilationContext;
QueryCompilationContext = queryCompilationContext;
SqlExpressionFactory = sqlExpressionFactory;
}

protected QueryCompilationContext QueryCompilationContext { get; private set; }
protected ISqlExpressionFactory SqlExpressionFactory { get; private set; }

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

if (!RelationalOptionsExtension.Extract(_queryCompilationContext.ContextOptions).UseRelationalNulls)
var useRelationalNulls = 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
108 changes: 75 additions & 33 deletions src/EFCore.Relational/Query/Pipeline/SqlExpressionOptimizingVisitor.cs
View file
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,49 @@ 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 value
? _sqlExpressionFactory.Constant(false, sqlUnaryExpression.TypeMapping)
: _sqlExpressionFactory.Constant(true, 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 +148,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 +176,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 +211,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;
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
4 changes: 4 additions & 0 deletions src/EFCore.SqlServer/Query/Pipeline/SqlServerShapedQueryOptimizingExpressionVisitors.cs
View file
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.Linq.Expressions;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Query.Pipeline;
using Microsoft.EntityFrameworkCore.Relational.Query.Pipeline;

Expand All @@ -21,6 +22,9 @@ public override Expression Visit(Expression query)
query = base.Visit(query);
query = new SearchConditionConvertingExpressionVisitor(SqlExpressionFactory).Visit(query);

var useRelationalNulls = RelationalOptionsExtension.Extract(QueryCompilationContext.ContextOptions).UseRelationalNulls;
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
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
Loading

0 comments on commit 37643f7

Please sign in to comment.