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

Changing query from == false to ! results in differing queries #18338

Closed
patrickklaeren opened this issue Oct 11, 2019 · 3 comments · Fixed by #19168
Closed

Changing query from == false to ! results in differing queries #18338

patrickklaeren opened this issue Oct 11, 2019 · 3 comments · Fixed by #19168
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@patrickklaeren
Copy link

patrickklaeren commented Oct 11, 2019

Given the following model

public class Foo
{
    public int Id { get; set; }
    public string Address { get; set; }
    public string Postcode { get; set; }
    public bool Deleted { get; set; }
    public bool NeedsUpdating { get; set; }
}

Address and postcode are both optional/nullable.

When writing a query to pull all Foos where NeedsUpdating is true and the address or postcode are not null, changing == false to ! results in a different query.

I am unsure if this is a bug, a language quirk or something I am missing.

Queries:

 var foo = await _db.Foos
.Where(c => c.Deleted == false)
.Where(c => c.NeedsUpdating)
.Where(x => string.IsNullOrEmpty(x.Postcode) == false || string.IsNullOrEmpty(x.Address) == false)
 .AnyAsync();

results in the following SQL query:

SELECT CASE
    WHEN EXISTS (
        SELECT 1
        FROM [Foos] AS [c]
        WHERE (([c].[Deleted] = CAST(0 AS bit)) AND ([c].[NeedsUpdating] = CAST(1 AS bit))) AND ((([c].[Postcode] IS NULL OR (([c].[Postcode] = N'') AND [c].[Postcode] IS NOT NULL)) AND (CAST(0 AS bit) = CAST(1 AS bit))) OR (([c].[Address] IS NULL OR (([c].[Address] = N'') AND [c].[Address] IS NOT NULL)) AND (CAST(0 AS bit) = CAST(1 AS bit))))) THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END

this does not yield the correct SQL query.

However, when changing == false to !, it works:

 var foo = await _db.Foos
.Where(c => c.Deleted == false)
.Where(c => c.NeedsUpdating)
.Where(x => !string.IsNullOrEmpty(x.Postcode) || !string.IsNullOrEmpty(x.Address))
 .AnyAsync();

results in the following SQL query:

SELECT CASE
    WHEN EXISTS (
        SELECT 1
        FROM [Foos] AS [c]
        WHERE (([c].[Deleted] = CAST(0 AS bit)) AND ([c].[NeedsUpdating] = CAST(1 AS bit))) AND (([c].[Postcode] IS NOT NULL AND (([c].[Postcode] <> N'') OR [c].[Postcode] IS NULL)) OR ([c].[Address] IS NOT NULL AND (([c].[Address] <> N'') OR [c].[Address] IS NULL)))) THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END

Further technical details

EF Core version: 3.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: ASP.NET Core 3.0 (sdk 3.0.100)
Operating system: Windows 10 Pro 1903
IDE: Visual Studio 2019 16.3

@ajcvickers
Copy link
Member

Note from triage: Since string.IsNullOrEmpty(x.Postcode) cannot return null there is no need for null semantics compensation here.

@ajcvickers ajcvickers added this to the Backlog milestone Oct 14, 2019
@nathan-alden-sr
Copy link

I stepped through EF Core's code (definitely not an expert in EF Core code) a bit and it looks like this may be happening for any IMethodCallTranslator that results in a bool expression. It also happens for string.IsNullOrWhiteSpace. The translation seems to simply ignore == false's existence.

@maumar maumar modified the milestones: Backlog, 5.0.0 Dec 6, 2019
maumar added a commit that referenced this issue Dec 6, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls.

Resolves #11464
Resolves #15722
Resolved #18338
Resolves #18597
Resolves #18689
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Dec 6, 2019
@maumar
Copy link
Contributor

maumar commented Dec 6, 2019

with new refactorings in place we produce the same query in both cases:

SELECT CASE
    WHEN EXISTS (
        SELECT 1
        FROM [Gears] AS [g]
        LEFT JOIN [Cities] AS [c] ON [g].[AssignedCityName] = [c].[Name]
        WHERE ([g].[Discriminator] IN (N'Gear', N'Officer') AND ([g].[HasSoulPatch] <> CAST(1 AS bit))) AND ([c].[Name] IS NOT NULL AND (([c].[Name] <> N'') OR [c].[Name] IS NULL))) THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END

fact that we unnecessarily apply null check to non-nullable function is tracked by #18555

maumar added a commit that referenced this issue Dec 7, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls.

Resolves #11464
Resolves #15722
Resolved #18338
Resolves #18597
Resolves #18689
maumar added a commit that referenced this issue Dec 10, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
maumar added a commit that referenced this issue Dec 12, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
maumar added a commit that referenced this issue Dec 12, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
maumar added a commit that referenced this issue Dec 12, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
maumar added a commit that referenced this issue Dec 13, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Dec 13, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Dec 16, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Dec 16, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Dec 16, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Dec 18, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- moving optimizations that depend on knowing the nullability to NullSemantics visitor - optimizer now only contains optimizations that also work in 3-value logic, or when we know nulls can't happen,
- merging InExpressionValuesExpandingExpressionVisitor int NullSemantics visitor, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Dec 31, 2019
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 1, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 2, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 4, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 7, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 8, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 8, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 8, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 8, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 8, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
maumar added a commit that referenced this issue Jan 8, 2020
- moving NullSemantics visitor after 2nd level cache - we need to know the parameter values to properly handle IN expressions wrt null semantics,
- NullSemantics visitor needs to go before SqlExpressionOptimizer and SearchCondition, so those two are also moved after 2nd level cache,
- combining NullSemantics with SqlExpressionOptimizer (kept SqlExpressionOptimizer name) - SearchCondition now applies some relevant optimization itself, so that we don't need to run full optimizer afterwards,
- merging InExpressionValuesExpandingExpressionVisitor into SqlExpressionOptimizer as well, so that we don't apply the rewrite for UseRelationalNulls,
- preventing NulSemantics from performing double visitation when computing non-nullable columns.

Resolves #11464
Resolves #15722
Resolves #18338
Resolves #18597
Resolves #18689
Resolves #19019
@maumar maumar closed this as completed in a5f3e26 Jan 8, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview1 Mar 13, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview1, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants