Skip to content

Commit

Permalink
Query: Avoid parethesis around binary operations when used with AND/OR
Browse files Browse the repository at this point in the history 
Sqlite/Postgre => AND - OR 2 most lowest priority operation in that order
SqlServer => Same as Sqlite except IN/LIKE which are below AND and same as OR
LIKE always puts parentheses around it. And IN evaluated left to right if first works. If 2nd then first one has to be same type as item (to cause `a OR b IN ...`) to evaluate OR first. That implies bool type and SqlServer cannot have bool value directly due to search condition so it always converts to `a = (1 as bit) OR b IN ...` which doesn't cause priority issue.
  • Loading branch information
@smitpatel
smitpatel committed Nov 19, 2021
1 parent 911d876 commit ea046af
Show file tree
Hide file tree
Showing 61 changed files with 2,542 additions and 2,387 deletions.
27 changes: 14 additions & 13 deletions src/EFCore.Relational/Query/QuerySqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -804,19 +804,20 @@ protected virtual bool RequiresParentheses(SqlExpression outerExpression, SqlExp

case SqlBinaryExpression sqlBinaryExpression:
{
//if (outerExpression is SqlBinaryExpression outerBinary)
//{
// if (outerBinary.OperatorType == ExpressionType.AndAlso)
// {
// return sqlBinaryExpression.OperatorType == ExpressionType.OrElse;
// }

// if (outerBinary.OperatorType == ExpressionType.OrElse)
// {
// // Precedence-wise AND is above OR but we still add parenthesis for ease of understanding
// return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso;
// }
//}
if (outerExpression is SqlBinaryExpression outerBinary)
{
// Math, bitwise, comparison and equality operators have higher precedence
if (outerBinary.OperatorType == ExpressionType.AndAlso)
{
return sqlBinaryExpression.OperatorType == ExpressionType.OrElse;
}

if (outerBinary.OperatorType == ExpressionType.OrElse)
{
// Precedence-wise AND is above OR but we still add parenthesis for ease of understanding
return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso;
}
}

return true;
}
Expand Down
30 changes: 30 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/Query/NorthwindWhereQueryCosmosTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2300,6 +2300,36 @@ public override Task Last_over_custom_projection_compared_to_not_null(bool async
return base.Last_over_custom_projection_compared_to_not_null(async);
}

public override async Task Where_Contains_and_comparison(bool async)
{
await base.Where_Contains_and_comparison(async);

AssertSql(
@"SELECT c
FROM root c
WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""CustomerID""] IN (""ALFKI"", ""FISSA"") AND (c[""City""] = ""Seattle"")))");
}

public override async Task Where_Contains_or_comparison(bool async)
{
await base.Where_Contains_or_comparison(async);

AssertSql(
@"SELECT c
FROM root c
WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""CustomerID""] IN (""ALFKI"", ""FISSA"") OR (c[""City""] = ""Seattle"")))");
}

public override Task Where_Like_and_comparison(bool async)
{
return AssertTranslationFailed(() => base.Where_Like_and_comparison(async));
}

public override Task Where_Like_or_comparison(bool async)
{
return AssertTranslationFailed(() => base.Where_Like_or_comparison(async));
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down
20 changes: 20 additions & 0 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9282,6 +9282,26 @@ public virtual Task Where_equals_method_on_nullable_with_object_overload(bool as
ss => ss.Set<Mission>().Where(m => m.Rating.Equals(null)));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Where_bool_column_and_Contains(bool async)
{
var values = new[] { false, true };
return AssertQuery(
async,
ss => ss.Set<Gear>().Where(g => g.HasSoulPatch && values.Contains(g.HasSoulPatch)));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Where_bool_column_or_Contains(bool async)
{
var values = new[] { false, true };
return AssertQuery(
async,
ss => ss.Set<Gear>().Where(g => g.HasSoulPatch && values.Contains(g.HasSoulPatch)));
}

protected GearsOfWarContext CreateContext()
=> Fixture.CreateContext();

Expand Down
43 changes: 43 additions & 0 deletions test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2672,5 +2672,48 @@ public virtual Task Last_over_custom_projection_compared_to_not_null(bool async)

private string StringMethod(string arg)
=> arg;

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Where_Contains_and_comparison(bool async)
{
var customerIds = new[] { "ALFKI", "FISSA" };
return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => customerIds.Contains(c.CustomerID) && c.City == "Seattle"));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Where_Contains_or_comparison(bool async)
{
var customerIds = new[] { "ALFKI", "FISSA" };
return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => customerIds.Contains(c.CustomerID) || c.City == "Seattle"),
entryCount: 3);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Where_Like_and_comparison(bool async)
{
return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => EF.Functions.Like(c.CustomerID, "F%") && c.City == "Seattle"),
ss => ss.Set<Customer>().Where(c => c.CustomerID.StartsWith("F") && c.City == "Seattle"),
entryCount: 0);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Where_Like_or_comparison(bool async)
{
return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => EF.Functions.Like(c.CustomerID, "F%") || c.City == "Seattle"),
ss => ss.Set<Customer>().Where(c => c.CustomerID.StartsWith("F") || c.City == "Seattle"),
entryCount: 9);
}
}
}
4 changes: 2 additions & 2 deletions test/EFCore.SqlServer.FunctionalTests/CustomConvertersSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ public override void Value_conversion_is_appropriately_used_for_join_condition()
SELECT [b].[Url]
FROM [Blog] AS [b]
INNER JOIN [Post] AS [p] ON (([b].[BlogId] = [p].[BlogId]) AND ([b].[IsVisible] = N'Y')) AND ([b].[BlogId] = @__blogId_0)
INNER JOIN [Post] AS [p] ON [b].[BlogId] = [p].[BlogId] AND [b].[IsVisible] = N'Y' AND [b].[BlogId] = @__blogId_0
WHERE [b].[IsVisible] = N'Y'");
}

Expand All @@ -234,7 +234,7 @@ public override void Value_conversion_is_appropriately_used_for_left_join_condit
SELECT [b].[Url]
FROM [Blog] AS [b]
LEFT JOIN [Post] AS [p] ON (([b].[BlogId] = [p].[BlogId]) AND ([b].[IsVisible] = N'Y')) AND ([b].[BlogId] = @__blogId_0)
LEFT JOIN [Post] AS [p] ON [b].[BlogId] = [p].[BlogId] AND [b].[IsVisible] = N'Y' AND [b].[BlogId] = @__blogId_0
WHERE [b].[IsVisible] = N'Y'");
}

Expand Down
12 changes: 6 additions & 6 deletions test/EFCore.SqlServer.FunctionalTests/FindSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ public override void Find_composite_key_from_store()
SELECT TOP(1) [c].[Id1], [c].[Id2], [c].[Foo]
FROM [CompositeKey] AS [c]
WHERE ([c].[Id1] = @__p_0) AND ([c].[Id2] = @__p_1)");
WHERE [c].[Id1] = @__p_0 AND [c].[Id2] = @__p_1");
}

public override void Returns_null_for_composite_key_not_in_store()
Expand All @@ -180,7 +180,7 @@ public override void Returns_null_for_composite_key_not_in_store()
SELECT TOP(1) [c].[Id1], [c].[Id2], [c].[Foo]
FROM [CompositeKey] AS [c]
WHERE ([c].[Id1] = @__p_0) AND ([c].[Id2] = @__p_1)");
WHERE [c].[Id1] = @__p_0 AND [c].[Id2] = @__p_1");
}

public override void Find_base_type_tracked()
Expand Down Expand Up @@ -230,7 +230,7 @@ public override void Find_derived_type_from_store()
SELECT TOP(1) [b].[Id], [b].[Discriminator], [b].[Foo], [b].[Boo]
FROM [BaseType] AS [b]
WHERE ([b].[Discriminator] = N'DerivedType') AND ([b].[Id] = @__p_0)");
WHERE [b].[Discriminator] = N'DerivedType' AND [b].[Id] = @__p_0");
}

public override void Returns_null_for_derived_type_not_in_store()
Expand All @@ -242,7 +242,7 @@ public override void Returns_null_for_derived_type_not_in_store()
SELECT TOP(1) [b].[Id], [b].[Discriminator], [b].[Foo], [b].[Boo]
FROM [BaseType] AS [b]
WHERE ([b].[Discriminator] = N'DerivedType') AND ([b].[Id] = @__p_0)");
WHERE [b].[Discriminator] = N'DerivedType' AND [b].[Id] = @__p_0");
}

public override void Find_base_type_using_derived_set_tracked()
Expand All @@ -254,7 +254,7 @@ public override void Find_base_type_using_derived_set_tracked()
SELECT TOP(1) [b].[Id], [b].[Discriminator], [b].[Foo], [b].[Boo]
FROM [BaseType] AS [b]
WHERE ([b].[Discriminator] = N'DerivedType') AND ([b].[Id] = @__p_0)");
WHERE [b].[Discriminator] = N'DerivedType' AND [b].[Id] = @__p_0");
}

public override void Find_base_type_using_derived_set_from_store()
Expand All @@ -266,7 +266,7 @@ public override void Find_base_type_using_derived_set_from_store()
SELECT TOP(1) [b].[Id], [b].[Discriminator], [b].[Foo], [b].[Boo]
FROM [BaseType] AS [b]
WHERE ([b].[Discriminator] = N'DerivedType') AND ([b].[Id] = @__p_0)");
WHERE [b].[Discriminator] = N'DerivedType' AND [b].[Id] = @__p_0");
}

public override void Find_derived_type_using_base_set_tracked()
Expand Down
8 changes: 4 additions & 4 deletions test/EFCore.SqlServer.FunctionalTests/LazyLoadProxySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ public override void Lazy_load_collection_composite_key(EntityState state)
SELECT [c].[Id], [c].[ParentAlternateId], [c].[ParentId]
FROM [ChildCompositeKey] AS [c]
WHERE ([c].[ParentAlternateId] = @__p_0) AND ([c].[ParentId] = @__p_1)");
WHERE [c].[ParentAlternateId] = @__p_0 AND [c].[ParentId] = @__p_1");
}

public override void Lazy_load_many_to_one_reference_to_principal_composite_key(EntityState state)
Expand All @@ -333,7 +333,7 @@ public override void Lazy_load_many_to_one_reference_to_principal_composite_key(
SELECT [p].[Id], [p].[AlternateId], [p].[Discriminator]
FROM [Parent] AS [p]
WHERE ([p].[AlternateId] = @__p_0) AND ([p].[Id] = @__p_1)");
WHERE [p].[AlternateId] = @__p_0 AND [p].[Id] = @__p_1");
}

public override void Lazy_load_one_to_one_reference_to_principal_composite_key(EntityState state)
Expand All @@ -346,7 +346,7 @@ public override void Lazy_load_one_to_one_reference_to_principal_composite_key(E
SELECT [p].[Id], [p].[AlternateId], [p].[Discriminator]
FROM [Parent] AS [p]
WHERE ([p].[AlternateId] = @__p_0) AND ([p].[Id] = @__p_1)");
WHERE [p].[AlternateId] = @__p_0 AND [p].[Id] = @__p_1");
}

public override void Lazy_load_one_to_one_reference_to_dependent_composite_key(EntityState state)
Expand All @@ -359,7 +359,7 @@ public override void Lazy_load_one_to_one_reference_to_dependent_composite_key(E
SELECT [s].[Id], [s].[ParentAlternateId], [s].[ParentId]
FROM [SingleCompositeKey] AS [s]
WHERE ([s].[ParentAlternateId] = @__p_0) AND ([s].[ParentId] = @__p_1)");
WHERE [s].[ParentAlternateId] = @__p_0 AND [s].[ParentId] = @__p_1");
}

public override void Lazy_load_many_to_one_reference_to_principal_null_FK_composite_key(EntityState state)
Expand Down
24 changes: 12 additions & 12 deletions test/EFCore.SqlServer.FunctionalTests/LoadSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ public override void Lazy_load_collection_composite_key(EntityState state)
SELECT [c].[Id], [c].[ParentAlternateId], [c].[ParentId]
FROM [ChildCompositeKey] AS [c]
WHERE ([c].[ParentAlternateId] = @__p_0) AND ([c].[ParentId] = @__p_1)");
WHERE [c].[ParentAlternateId] = @__p_0 AND [c].[ParentId] = @__p_1");
}

public override void Lazy_load_many_to_one_reference_to_principal_composite_key(EntityState state)
Expand All @@ -331,7 +331,7 @@ public override void Lazy_load_many_to_one_reference_to_principal_composite_key(
SELECT [p].[Id], [p].[AlternateId]
FROM [Parent] AS [p]
WHERE ([p].[AlternateId] = @__p_0) AND ([p].[Id] = @__p_1)");
WHERE [p].[AlternateId] = @__p_0 AND [p].[Id] = @__p_1");
}

public override void Lazy_load_one_to_one_reference_to_principal_composite_key(EntityState state)
Expand All @@ -344,7 +344,7 @@ public override void Lazy_load_one_to_one_reference_to_principal_composite_key(E
SELECT [p].[Id], [p].[AlternateId]
FROM [Parent] AS [p]
WHERE ([p].[AlternateId] = @__p_0) AND ([p].[Id] = @__p_1)");
WHERE [p].[AlternateId] = @__p_0 AND [p].[Id] = @__p_1");
}

public override void Lazy_load_one_to_one_reference_to_dependent_composite_key(EntityState state)
Expand All @@ -357,7 +357,7 @@ public override void Lazy_load_one_to_one_reference_to_dependent_composite_key(E
SELECT [s].[Id], [s].[ParentAlternateId], [s].[ParentId]
FROM [SingleCompositeKey] AS [s]
WHERE ([s].[ParentAlternateId] = @__p_0) AND ([s].[ParentId] = @__p_1)");
WHERE [s].[ParentAlternateId] = @__p_0 AND [s].[ParentId] = @__p_1");
}

public override void Lazy_load_many_to_one_reference_to_principal_null_FK_composite_key(EntityState state)
Expand Down Expand Up @@ -1321,7 +1321,7 @@ public override async Task Load_collection_composite_key(EntityState state, bool
SELECT [c].[Id], [c].[ParentAlternateId], [c].[ParentId]
FROM [ChildCompositeKey] AS [c]
WHERE ([c].[ParentAlternateId] = @__p_0) AND ([c].[ParentId] = @__p_1)");
WHERE [c].[ParentAlternateId] = @__p_0 AND [c].[ParentId] = @__p_1");
}

public override async Task Load_many_to_one_reference_to_principal_composite_key(EntityState state, bool async)
Expand All @@ -1334,7 +1334,7 @@ public override async Task Load_many_to_one_reference_to_principal_composite_key
SELECT [p].[Id], [p].[AlternateId]
FROM [Parent] AS [p]
WHERE ([p].[AlternateId] = @__p_0) AND ([p].[Id] = @__p_1)");
WHERE [p].[AlternateId] = @__p_0 AND [p].[Id] = @__p_1");
}

public override async Task Load_one_to_one_reference_to_principal_composite_key(EntityState state, bool async)
Expand All @@ -1347,7 +1347,7 @@ public override async Task Load_one_to_one_reference_to_principal_composite_key(
SELECT [p].[Id], [p].[AlternateId]
FROM [Parent] AS [p]
WHERE ([p].[AlternateId] = @__p_0) AND ([p].[Id] = @__p_1)");
WHERE [p].[AlternateId] = @__p_0 AND [p].[Id] = @__p_1");
}

public override async Task Load_one_to_one_reference_to_dependent_composite_key(EntityState state, bool async)
Expand All @@ -1360,7 +1360,7 @@ public override async Task Load_one_to_one_reference_to_dependent_composite_key(
SELECT [s].[Id], [s].[ParentAlternateId], [s].[ParentId]
FROM [SingleCompositeKey] AS [s]
WHERE ([s].[ParentAlternateId] = @__p_0) AND ([s].[ParentId] = @__p_1)");
WHERE [s].[ParentAlternateId] = @__p_0 AND [s].[ParentId] = @__p_1");
}

public override async Task Load_collection_using_Query_composite_key(EntityState state, bool async)
Expand All @@ -1373,7 +1373,7 @@ public override async Task Load_collection_using_Query_composite_key(EntityState
SELECT [c].[Id], [c].[ParentAlternateId], [c].[ParentId]
FROM [ChildCompositeKey] AS [c]
WHERE ([c].[ParentAlternateId] = @__p_0) AND ([c].[ParentId] = @__p_1)");
WHERE [c].[ParentAlternateId] = @__p_0 AND [c].[ParentId] = @__p_1");
}

public override async Task Load_many_to_one_reference_to_principal_using_Query_composite_key(EntityState state, bool async)
Expand All @@ -1386,7 +1386,7 @@ public override async Task Load_many_to_one_reference_to_principal_using_Query_c
SELECT TOP(2) [p].[Id], [p].[AlternateId]
FROM [Parent] AS [p]
WHERE ([p].[AlternateId] = @__p_0) AND ([p].[Id] = @__p_1)");
WHERE [p].[AlternateId] = @__p_0 AND [p].[Id] = @__p_1");
}

public override async Task Load_one_to_one_reference_to_principal_using_Query_composite_key(EntityState state, bool async)
Expand All @@ -1399,7 +1399,7 @@ public override async Task Load_one_to_one_reference_to_principal_using_Query_co
SELECT TOP(2) [p].[Id], [p].[AlternateId]
FROM [Parent] AS [p]
WHERE ([p].[AlternateId] = @__p_0) AND ([p].[Id] = @__p_1)");
WHERE [p].[AlternateId] = @__p_0 AND [p].[Id] = @__p_1");
}

public override async Task Load_one_to_one_reference_to_dependent_using_Query_composite_key(EntityState state, bool async)
Expand All @@ -1412,7 +1412,7 @@ public override async Task Load_one_to_one_reference_to_dependent_using_Query_co
SELECT TOP(2) [s].[Id], [s].[ParentAlternateId], [s].[ParentId]
FROM [SingleCompositeKey] AS [s]
WHERE ([s].[ParentAlternateId] = @__p_0) AND ([s].[ParentId] = @__p_1)");
WHERE [s].[ParentAlternateId] = @__p_0 AND [s].[ParentId] = @__p_1");
}

public override async Task Load_many_to_one_reference_to_principal_null_FK_composite_key(EntityState state, bool async)
Expand Down
Loading

0 comments on commit ea046af

Please sign in to comment.