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.

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

Better management of negated expressions #31028

Merged
merged 1 commit into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
maumar marked this conversation as resolved.
Show resolved Hide resolved
{
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
maumar marked this conversation as resolved.
Show resolved Hide resolved
{
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