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

LINQ/SQLITE: Unnecessary IS NULL term generated for linq clause !string.isnullorempty() #19410

Closed
fmaeseele opened this issue Dec 27, 2019 · 4 comments · Fixed by #25951
Closed
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

@fmaeseele
Copy link

fmaeseele commented Dec 27, 2019

Hi,
Using AspnetCore 3.1 with Linq/SQLite, I have a SQL strangely generated for a simple clause.
I have a simple table containing 2 text fields : COUNTRY and ISOCOUNTRY.
I want to get all rows where COUNTRY is not null and not empty:

VisitorsByCountry = _dataDbContext.Visitors
    .AsNoTracking()
    // Fix for NULL values on ordinal 11
    .Select(v => new { v.Country, v.IsoCountry })
    .Where(v => !string.IsNullOrEmpty(v.Country))
    .AsEnumerable(); // Fix for EF 3.0 not supporting ClientSide GroupBy

When I look into the SQL generated for the Where(v => !string.IsNullOrEmpty(v.Country)) :

SELECT "v"."Country", "v"."IsoCountry"
FROM "Visitors" AS "v"
WHERE "v"."Country" IS NOT NULL AND (("v"."Country" <> '') OR "v"."Country" IS NULL)

The Where clause should be like: WHERE "v"."Country" IS NOT NULL AND "v"."Country" <> ''

Regards

@fmaeseele fmaeseele changed the title LINQ/SQLITE: Invalid query with clause field string.isnullorempty() LINQ/SQLITE: Invalid generated SQL with linq clause string.isnullorempty() Dec 27, 2019
@roji
Copy link
Member

roji commented Dec 27, 2019

/cc @maumar

@maumar
Copy link
Contributor

maumar commented Dec 31, 2019

IsNullOrEmpty can never be null, so we shouldn't ally "full" expansion even for the negated case. Related to #18555. Irrespective, OR "v"."Country" IS NULL should be optimized out because we already know that v.Country is not nullable from the earlier IS NOT NULL check

@maumar maumar self-assigned this Dec 31, 2019
@maumar
Copy link
Contributor

maumar commented Jan 1, 2020

Problem here is that at the time we compute null semantics we have the expression in the following form:

NOT ( v.Country IS NULL OR (  v.Country = N'' ))

however we only recognize pattern is form: column IS NOT NULL && (...)

We should recognize the pattern in the negated form as well, i.e.

column IS NULL OR (...)

@maumar maumar changed the title LINQ/SQLITE: Invalid generated SQL with linq clause string.isnullorempty() LINQ/SQLITE: Unnecessary terms generated for linq clause !string.isnullorempty() Jan 1, 2020
@maumar maumar changed the title LINQ/SQLITE: Unnecessary terms generated for linq clause !string.isnullorempty() LINQ/SQLITE: Unnecessary IS NULL term generated for linq clause !string.isnullorempty() Jan 1, 2020
@ajcvickers ajcvickers added this to the Backlog milestone Jan 3, 2020
@ajcvickers ajcvickers modified the milestones: Backlog, 6.0.0 Nov 2, 2020
@maumar
Copy link
Contributor

maumar commented Sep 8, 2021

note: on sqlserver we translate IsNullOrEmpty to like and don't produce the redundant null check:

NOT ([c].[Country] IS NULL OR ([c].[Country] LIKE N''))

maumar added a commit that referenced this issue Sep 9, 2021
…at are guaranteed to be null and remove redundant IS NULL/ IS NOT NULL checks

Adding a list of columns guaranteed to be null in the given subtree. When processing right side of the || operator, we can convert those to non-nullable columns and therefore improve the generated sql.
Also added optimization for conditional - if the test has some columns guaranteed to be nullable, we can flip those also when processing the Else subtree.

Also fixes #19410
@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 Sep 9, 2021
maumar added a commit that referenced this issue Sep 9, 2021
…at are guaranteed to be null and remove redundant IS NULL/ IS NOT NULL checks

Adding a list of columns guaranteed to be null in the given subtree. When processing right side of the || operator, we can convert those to non-nullable columns and therefore improve the generated sql.
Also added optimization for conditional - if the test has some columns guaranteed to be nullable, we can flip those also when processing the Else subtree.

Fixes #19883
Fixes #19410
maumar added a commit that referenced this issue Sep 11, 2021
…at are guaranteed to be null and remove redundant IS NULL/ IS NOT NULL checks

Adding a list of columns guaranteed to be null in the given subtree. When processing right side of the || operator, we can convert those to non-nullable columns and therefore improve the generated sql.
Also added optimization for conditional - if the test has some columns guaranteed to be nullable, we can flip those also when processing the Else subtree.

Fixes #19883
Fixes #19410
maumar added a commit that referenced this issue Sep 11, 2021
…at are guaranteed to be null and remove redundant IS NULL/ IS NOT NULL checks

Adding a list of columns guaranteed to be null in the given subtree. When processing right side of the || operator, we can convert those to non-nullable columns and therefore improve the generated sql.
Also added optimization for conditional - if the test has some columns guaranteed to be nullable, we can flip those also when processing the Else subtree.

Fixes #19883
Fixes #19410
maumar added a commit that referenced this issue Sep 11, 2021
…at are guaranteed to be null and remove redundant IS NULL/ IS NOT NULL checks

Adding a list of columns guaranteed to be null in the given subtree. When processing right side of the || operator, we can convert those to non-nullable columns and therefore improve the generated sql.

Fixes #19883
Fixes #19410
maumar added a commit that referenced this issue Sep 14, 2021
…at are guaranteed to be null and remove redundant IS NULL/ IS NOT NULL checks

Adding a list of columns guaranteed to be null in the given subtree. When processing right side of the || operator, we can convert those to non-nullable columns and therefore improve the generated sql.

Fixes #19883
Fixes #19410
@maumar maumar closed this as completed in fc00579 Sep 14, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc2 Sep 14, 2021
@ajcvickers ajcvickers removed this from the 6.0.0-rc2 milestone Nov 8, 2021
@ajcvickers ajcvickers added this to the 6.0.0 milestone 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
4 participants