Skip to content

Commit

Permalink
Better management of negated expressions
Browse files Browse the repository at this point in the history
Closes #31027
  • Loading branch information
roji committed Jun 4, 2023
1 parent dff1a3d commit af35cdc
Show file tree
Hide file tree
Showing 36 changed files with 253 additions and 313 deletions.
4 changes: 2 additions & 2 deletions src/EFCore.Cosmos/Query/Internal/CosmosContainsTranslator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@ public CosmosContainsTranslator(ISqlExpressionFactory sqlExpressionFactory)
&& method.GetGenericMethodDefinition().Equals(EnumerableMethods.Contains)
&& ValidateValues(arguments[0]))
{
return _sqlExpressionFactory.In(arguments[1], arguments[0], false);
return _sqlExpressionFactory.In(arguments[1], arguments[0]);
}

if (arguments.Count == 1
&& method.IsContainsMethod()
&& instance != null
&& ValidateValues(instance))
{
return _sqlExpressionFactory.In(arguments[0], instance, false);
return _sqlExpressionFactory.In(arguments[0], instance);
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ public override Expression Visit(Expression expression)
var updatedInExpression = inValues.Count > 0
? _sqlExpressionFactory.In(
(SqlExpression)Visit(inExpression.Item),
_sqlExpressionFactory.Constant(inValues, typeMapping),
inExpression.IsNegated)
_sqlExpressionFactory.Constant(inValues, typeMapping))
: null;

var nullCheckExpression = hasNullValue
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -704,8 +704,7 @@ protected override Expression VisitTypeBinary(TypeBinaryExpression typeBinaryExp
_sqlExpressionFactory.Constant(concreteEntityTypes[0].GetDiscriminatorValue()))
: _sqlExpressionFactory.In(
discriminatorColumn,
_sqlExpressionFactory.Constant(concreteEntityTypes.Select(et => et.GetDiscriminatorValue()).ToList()),
negated: false);
_sqlExpressionFactory.Constant(concreteEntityTypes.Select(et => et.GetDiscriminatorValue()).ToList()));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ SqlConditionalExpression Condition(
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
InExpression In(SqlExpression item, SqlExpression values, bool negated);
InExpression In(SqlExpression item, SqlExpression values);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down
26 changes: 3 additions & 23 deletions src/EFCore.Cosmos/Query/Internal/InExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,11 @@ public class InExpression : SqlExpression
/// </summary>
public InExpression(
SqlExpression item,
bool negated,
SqlExpression values,
CoreTypeMapping typeMapping)
: base(typeof(bool), typeMapping)
{
Item = item;
IsNegated = negated;
Values = values;
}

Expand All @@ -37,14 +35,6 @@ public InExpression(
/// </summary>
public virtual SqlExpression Item { get; }

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual bool IsNegated { get; }

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
Expand All @@ -67,15 +57,6 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
return Update(newItem, values);
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual InExpression Negate()
=> new(Item, !IsNegated, Values, TypeMapping!);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
Expand All @@ -84,7 +65,7 @@ public virtual InExpression Negate()
/// </summary>
public virtual InExpression Update(SqlExpression item, SqlExpression values)
=> item != Item || values != Values
? new InExpression(item, IsNegated, values, TypeMapping!)
? new InExpression(item, values, TypeMapping!)
: this;

/// <summary>
Expand All @@ -96,7 +77,7 @@ public virtual InExpression Update(SqlExpression item, SqlExpression values)
protected override void Print(ExpressionPrinter expressionPrinter)
{
expressionPrinter.Visit(Item);
expressionPrinter.Append(IsNegated ? " NOT IN " : " IN ");
expressionPrinter.Append(" IN ");
expressionPrinter.Append("(");
expressionPrinter.Visit(Values);
expressionPrinter.Append(")");
Expand All @@ -117,7 +98,6 @@ public override bool Equals(object? obj)
private bool Equals(InExpression inExpression)
=> base.Equals(inExpression)
&& Item.Equals(inExpression.Item)
&& IsNegated.Equals(inExpression.IsNegated)
&& Values.Equals(inExpression.Values);

/// <summary>
Expand All @@ -127,5 +107,5 @@ private bool Equals(InExpression inExpression)
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public override int GetHashCode()
=> HashCode.Combine(base.GetHashCode(), Item, IsNegated, Values);
=> HashCode.Combine(base.GetHashCode(), Item, Values);
}
26 changes: 22 additions & 4 deletions src/EFCore.Cosmos/Query/Internal/QuerySqlGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,13 @@ protected override Expression VisitSqlUnary(SqlUnaryExpression sqlUnaryExpressio
if (sqlUnaryExpression.OperatorType == ExpressionType.Not
&& sqlUnaryExpression.Operand.Type == typeof(bool))
{
if (sqlUnaryExpression.Operand is InExpression inExpression)
{
GenerateIn(inExpression, negated: true);

return sqlUnaryExpression;
}

op = "NOT";
}

Expand Down Expand Up @@ -506,18 +513,29 @@ protected override Expression VisitSqlParameter(SqlParameterExpression sqlParame
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
protected override Expression VisitIn(InExpression inExpression)
protected sealed override Expression VisitIn(InExpression inExpression)
{
GenerateIn(inExpression, negated: false);

return inExpression;
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
protected virtual void GenerateIn(InExpression inExpression, bool negated)
{
Visit(inExpression.Item);
_sqlBuilder.Append(inExpression.IsNegated ? " NOT IN " : " IN ");
_sqlBuilder.Append(negated ? " NOT IN " : " IN ");
_sqlBuilder.Append('(');
var valuesConstant = (SqlConstantExpression)inExpression.Values;
var valuesList = ((IEnumerable<object>)valuesConstant.Value)
.Select(v => new SqlConstantExpression(Expression.Constant(v), valuesConstant.TypeMapping)).ToList();
GenerateList(valuesList, e => Visit(e));
_sqlBuilder.Append(')');

return inExpression;
}

/// <summary>
Expand Down
7 changes: 3 additions & 4 deletions src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -473,14 +473,14 @@ public virtual SqlConditionalExpression Condition(SqlExpression test, SqlExpress
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual InExpression In(SqlExpression item, SqlExpression values, bool negated)
public virtual InExpression In(SqlExpression item, SqlExpression values)
{
var typeMapping = item.TypeMapping ?? _typeMappingSource.FindMapping(item.Type, _model);

item = ApplyTypeMapping(item, typeMapping);
values = ApplyTypeMapping(values, typeMapping);

return new InExpression(item, negated, values, _boolTypeMapping);
return new InExpression(item, values, _boolTypeMapping);
}

/// <summary>
Expand Down Expand Up @@ -539,8 +539,7 @@ private void AddDiscriminator(SelectExpression selectExpression, IEntityType ent

selectExpression.ApplyPredicate(
In(
(SqlExpression)discriminatorColumn, Constant(concreteEntityTypes.Select(et => et.GetDiscriminatorValue()).ToList()),
negated: false));
(SqlExpression)discriminatorColumn, Constant(concreteEntityTypes.Select(et => et.GetDiscriminatorValue()).ToList())));
}
}
}
9 changes: 3 additions & 6 deletions src/EFCore.Relational/Query/ISqlExpressionFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -399,27 +399,24 @@ SqlFunctionExpression NiladicFunction(
/// Creates a new <see cref="ExistsExpression" /> which represents an EXISTS operation in a SQL tree.
/// </summary>
/// <param name="subquery">A subquery to check existence of.</param>
/// <param name="negated">A value indicating if the existence check is negated.</param>
/// <returns>An expression representing an EXISTS operation in a SQL tree.</returns>
ExistsExpression Exists(SelectExpression subquery, bool negated);
ExistsExpression Exists(SelectExpression subquery);

/// <summary>
/// Creates a new <see cref="InExpression" /> which represents an IN operation in a SQL tree.
/// </summary>
/// <param name="item">An item to look into values.</param>
/// <param name="values">A list of values in which item is searched.</param>
/// <param name="negated">A value indicating if the item should be present in the values or absent.</param>
/// <returns>An expression representing an IN operation in a SQL tree.</returns>
InExpression In(SqlExpression item, SqlExpression values, bool negated);
InExpression In(SqlExpression item, SqlExpression values);

/// <summary>
/// Creates a new <see cref="InExpression" /> which represents an IN operation in a SQL tree.
/// </summary>
/// <param name="item">An item to look into values.</param>
/// <param name="subquery">A subquery in which item is searched.</param>
/// <param name="negated">A value indicating if the item should be present in the values or absent.</param>
/// <returns>An expression representing an IN operation in a SQL tree.</returns>
InExpression In(SqlExpression item, SelectExpression subquery, bool negated);
InExpression In(SqlExpression item, SelectExpression subquery);

/// <summary>
/// Creates a new <see cref="InExpression" /> which represents a LIKE in a SQL tree.
Expand Down
4 changes: 2 additions & 2 deletions src/EFCore.Relational/Query/Internal/ContainsTranslator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,15 @@ public ContainsTranslator(ISqlExpressionFactory sqlExpressionFactory)
&& method.GetGenericMethodDefinition().Equals(EnumerableMethods.Contains)
&& ValidateValues(arguments[0]))
{
return _sqlExpressionFactory.In(RemoveObjectConvert(arguments[1]), arguments[0], negated: false);
return _sqlExpressionFactory.In(RemoveObjectConvert(arguments[1]), arguments[0]);
}

if (arguments.Count == 1
&& method.IsContainsMethod()
&& instance != null
&& ValidateValues(instance))
{
return _sqlExpressionFactory.In(RemoveObjectConvert(arguments[0]), instance, negated: false);
return _sqlExpressionFactory.In(RemoveObjectConvert(arguments[0]), instance);
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,10 +307,16 @@ or ExpressionType.LessThan
break;
}

return _sqlExpressionFactory.In(
var inExpression = _sqlExpressionFactory.In(
leftCandidateInfo.ColumnExpression,
_sqlExpressionFactory.Constant(resultArray, leftCandidateInfo.TypeMapping),
leftCandidateInfo.OperationType == ExpressionType.NotEqual);
_sqlExpressionFactory.Constant(resultArray, leftCandidateInfo.TypeMapping));

return leftCandidateInfo.OperationType switch
{
ExpressionType.Equal => inExpression,
ExpressionType.NotEqual => _sqlExpressionFactory.Not(inExpression),
_ => throw new InvalidOperationException("IMPOSSIBLE")
};
}

if (leftConstantIsEnumerable && rightConstantIsEnumerable)
Expand All @@ -321,10 +327,16 @@ or ExpressionType.LessThan
(IEnumerable)leftCandidateInfo.ConstantValue,
(IEnumerable)rightCandidateInfo.ConstantValue);

return _sqlExpressionFactory.In(
var inExpression = _sqlExpressionFactory.In(
leftCandidateInfo.ColumnExpression,
_sqlExpressionFactory.Constant(resultArray, leftCandidateInfo.TypeMapping),
leftCandidateInfo.OperationType == ExpressionType.NotEqual);
_sqlExpressionFactory.Constant(resultArray, leftCandidateInfo.TypeMapping));

return leftCandidateInfo.OperationType switch
{
ExpressionType.Equal => inExpression,
ExpressionType.NotEqual => _sqlExpressionFactory.Not(inExpression),
_ => throw new InvalidOperationException("IMPOSSIBLE")
};
}
}
}
Expand Down Expand Up @@ -424,10 +436,9 @@ private static bool TryGetInExpressionCandidateInfo(
else if (sqlExpression is InExpression
{
Item: ColumnExpression column, Subquery: null, Values: SqlConstantExpression valuesConstant
} inExpression)
})
{
candidateInfo = (column, valuesConstant.Value!, valuesConstant.TypeMapping!,
inExpression.IsNegated ? ExpressionType.NotEqual : ExpressionType.Equal);
candidateInfo = (column, valuesConstant.Value!, valuesConstant.TypeMapping!, ExpressionType.Equal);

return true;
}
Expand Down
Loading

0 comments on commit af35cdc

Please sign in to comment.