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

UseRelationalNulls causes subquery to include NOT IN (NULL, x) #23761

Closed
forenpm opened this issue Dec 24, 2020 · 2 comments · Fixed by #23856
Closed

UseRelationalNulls causes subquery to include NOT IN (NULL, x) #23761

forenpm opened this issue Dec 24, 2020 · 2 comments · Fixed by #23856
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

@forenpm
Copy link

forenpm commented Dec 24, 2020

This query produces an invalid WHERE when UseRelationalNulls = true for nullable int.
Incorrect SQL producted: AND [a].[StatusCode] NOT IN (NULL, 3)

await db.ProjectModels
  .Where(p => p.ProjectId == projectid)
  .Select(p => new ScoreRemainingActivitiesInput
  {
      RemainingActivities = p.Activities
          .Where(a => a.TaskType != TaskTypeEnum.TT_WBS && a.TaskType != TaskTypeEnum.TT_LOE)
          .Where(a => a.StatusCode != null && a.StatusCode != StatusCodeEnum.TK_Complete)
          .Count()
  }).FirstOrDefaultAsync();

SQL when UseRelationalNulls = false.

SELECT TOP (1) (
    SELECT COUNT(*)
    FROM [ActivityModels] AS [a]
    WHERE (
        ([p].[ProjectId] = [a].[ProjectId])
        AND (
          [a].[TaskType] NOT IN (6, 3)
          OR [a].[TaskType] IS NULL
          )
        )
      AND (
        ([a].[StatusCode] <> 3)
        AND [a].[StatusCode] IS NOT NULL
        )
    ) AS [RemainingActivities]
FROM [ProjectModels] AS [p]
WHERE [p].[ProjectId] = '1A66B859-F096-4595-8B30-D127D8CA6E0E'

SQL when UseRelationalNulls = true.

SELECT TOP (1) (
    SELECT COUNT(*)
    FROM [ActivityModels] AS [a]
    WHERE (
        ([p].[ProjectId] = [a].[ProjectId])
        AND [a].[TaskType] NOT IN (6, 3)
        )
      AND [a].[StatusCode] NOT IN (NULL, 3)
    ) AS [RemainingActivities]
FROM [ProjectModels] AS [p]
WHERE [p].[ProjectId] = '1A66B859-F096-4595-8B30-D127D8CA6E0E'

EF Core version: 5.0.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 5.0
Operating system: Windows 10
IDE: Visual Studio 2019 16.8.3

@maumar maumar self-assigned this Dec 24, 2020
@ajcvickers ajcvickers added this to the 6.0.0 milestone Jan 4, 2021
@maumar
Copy link
Contributor

maumar commented Jan 6, 2021

This is by design, when relational null semantics is switched on, EF Core naively translates null comparisons. There is an optimization which compacts multiple comparisons into IN/NOT IN, but the query yields the same results if the optimization is not applied, i.e. [a].[StatusCode] <> 3 AND [a].[StatusCode] <> NULL rather than [a].[StatusCode] NOT IN (NULL, 3)

The purpose for relational null semantics is to get a bit of performance by skipping all the additional null checks/optimizations when you know that these checks are not necessary, e.g. when the database columns are nullable but they don't contain any nulls. Otherwise it is recommended to use clr null semantics to avoid data corruption errors.

@maumar maumar removed this from the 6.0.0 milestone Jan 6, 2021
@maumar maumar closed this as completed Jan 6, 2021
@smitpatel smitpatel reopened this Jan 6, 2021
@smitpatel
Copy link
Contributor

a => a.StatusCode != null && a.StatusCode != StatusCodeEnum.TK_Complete

We should not combine above in InExpression if relational null semantics are on. Currently there is no way for user to write the comparison. If user writes a list.Contains where list has a null value then it is fine to generate above invalid SQL. User can explicitly check for null, if list contains it and remove the null values from list before calling Contains. (Funcletizer will take care of generate correct parameters and translating it)

@maumar maumar modified the milestone: 6.0.0 Jan 7, 2021
maumar added a commit that referenced this issue Jan 12, 2021
…(NULL, x)

We do optimization which combines comparisons based on the same property into IN and NOT IN. Problem is that if the comparisons contain nulls, we add those into the list of IN values, generating queries like  a IN (1, 2, NULL). In normal circumstances, during null semantics processing we extract these nulls and produce correct IS NULL / IS NOT NULL calls, however when useRelationalNulls is enabled, we don't run null semantics visitor and therefore don't "fix" the IN expressions.

Fix is to only apply the initial optimization for c# null semantics.

Fixes #23761
@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 Jan 12, 2021
maumar added a commit that referenced this issue Jan 14, 2021
…(NULL, x)

We do optimization which combines comparisons based on the same property into IN and NOT IN. Problem is that if the comparisons contain nulls, we add those into the list of IN values, generating queries like  a IN (1, 2, NULL). In normal circumstances, during null semantics processing we extract these nulls and produce correct IS NULL / IS NOT NULL calls, however when useRelationalNulls is enabled, we don't run null semantics visitor and therefore don't "fix" the IN expressions.

Fix is to only apply the initial optimization for c# null semantics.

Fixes #23761
maumar added a commit that referenced this issue Jan 14, 2021
…(NULL, x)

We do optimization which combines comparisons based on the same property into IN and NOT IN. Problem is that if the comparisons contain nulls, we add those into the list of IN values, generating queries like  a IN (1, 2, NULL). In normal circumstances, during null semantics processing we extract these nulls and produce correct IS NULL / IS NOT NULL calls, however when useRelationalNulls is enabled, we don't run null semantics visitor and therefore don't "fix" the IN expressions.

Fix is to only apply the initial optimization for c# null semantics.

Fixes #23761
maumar added a commit that referenced this issue Jan 15, 2021
…(NULL, x)

We do optimization which combines comparisons based on the same property into IN and NOT IN. Problem is that if the comparisons contain nulls, we add those into the list of IN values, generating queries like  a IN (1, 2, NULL). In normal circumstances, during null semantics processing we extract these nulls and produce correct IS NULL / IS NOT NULL calls, however when useRelationalNulls is enabled, we don't run null semantics visitor and therefore don't "fix" the IN expressions.

Fix is to only apply the initial optimization for c# null semantics.

Fixes #23761
@maumar maumar closed this as completed in f70d12d Jan 15, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview1 Jan 27, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview1, 6.0.0 Nov 8, 2021
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.

4 participants