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

!boolean in query should convert to boolean == false rather than boolean != true where possible #23472

Closed
davesmits opened this issue Nov 25, 2020 · 27 comments · Fixed by #23711
Assignees
Labels
area-perf 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

@davesmits
Copy link

Ask a question

We have in most of our tables a property bool Deleted (not nullable); and you guess in most of our queries we do Where(x => !x.Deleted).

No i notice that the query gets translated to Deleted <> 1 which is for sql different then Deleted == 0 making it for sql harder to use certain indexes. Are there any guidlines towards this?

@joakimriedel
Copy link
Contributor

Out of curiosity, what happens if you do Convert.ToBoolean(x.IsDeleted) == false ? It will translate to CONVERT(bit, [x].[IsDeleted]) = CAST(0 AS bit) but don't know if the redundant convert and cast will hurt performance or get optimized away.

@roji
Copy link
Member

roji commented Nov 25, 2020

No i notice that the query gets translated to Deleted <> 1 which is for sql different then Deleted == 0 making it for sql harder to use certain indexes. Are there any guidlines towards this?

Can you point to any resources on Deleted <> 1 being harder for indexes than Deleted == 0?

@davesmits
Copy link
Author

davesmits commented Nov 25, 2020

@roji i dont have documentation just personal investigation on a perf problem we had

Query generated by EF Core

SELECT [c].[Id], [c].[AssignedToId], [c].[CaseStateId], [c].[ContentModifiedAt], [c].[CreatedAt], [c].[CreatedByUserId], [c].[Deleted], [c].[Description], [c].[ModifiedAt], [c].[ModifiedByUserId], [c].[Tags], [c].[TenantId], [c].[Title]
      FROM [Cases] AS [c]
      WHERE ([c].[Deleted] <> CAST(1 AS bit)) AND ([c].[TenantId] = '40C53C5C-528E-4A60-BC04-0AA23616DA80')
      ORDER BY [c].[CreatedAt] DESC
      OFFSET 0 ROWS FETCH NEXT 100 ROWS ONLY

Resulted in this query plan
image

Change the Deleted<> 1 to Deleted = 0 ressulted in

image

and the last one is also using a index i was expecting

@davesmits
Copy link
Author

@joakimriedel Changing the query to x.Deleted == false is already resolving our issue. The issue is only there when using !x.Deleted

@joakimriedel
Copy link
Contributor

@davesmits that's interesting. For me running EF Core 5 x.IsDeleted == false will translate to [x].[IsDeleted] <> CAST(1 AS bit) which is why I suggested the convert solution.

@davesmits
Copy link
Author

@joakimriedel sorry; we didnt upgrade yet to 5.0. sorry for forgetting to mention we are in 3.1; working on the upgrade but some breaking changes in identity part making it hard

@joakimriedel
Copy link
Contributor

@davesmits OK one more thing to add for the upgrade to-do list then, if you choose to do x.IsDeleted == false as a workaround for !x.IsDeleted.

Not to hijack this issue but @roji do you know why x.IsDeleted == false translates differently in 3.1 and 5.0 ? It's not in breaking changes document.

@roji
Copy link
Member

roji commented Nov 25, 2020

@joakimriedel I'm not sure exactly why this change was done, but we occasionally change/improve the precise SQL constructs which get generated; this shouldn't be a breaking change in any case.

However, the different perf characteristics indeed seem important and we should look into it. Thanks for that info @davesmits!

@roji roji added the area-perf label Nov 25, 2020
@joakimriedel
Copy link
Contributor

joakimriedel commented Nov 26, 2020

@roji very minor impact, but still breaking. Indexing a bit column is normally useless unless the data is skewed to either many 0 or many 1. In one of my queries I had a filter with a predicate similar to

create index [IX_not_opened] on dbo.Messages (Id) where Opened=0

since there would be pretty few rows with Opened=0 this index helped SQL Server to quickly filter the rows when selecting non-opened messages.

After the upgrade to 5.0 this query didn't use the index anymore, but since it's a rarely used query I didn't notice the performance degradation until this issue posted by @davesmits . Tested my workaround above and the index is now properly used again!

Thanks for looking into this!

@smitpatel
Copy link
Contributor

Cannot change without #15586
The issue is bit is what is used for bool in SqlServer and conditions are bool. If you convert everything to int then bit values would be same and compare the same but that is not the knowledge query pipeline have yet.

@davesmits
Copy link
Author

@smitpatel I am not sure what you mean convert to int? Is that a internal thing of EF.Core?

@davesmits
Copy link
Author

thanks @joakimriedel ; we are now migrating to .net 5 and seeing indeed the problem beeing with with deleted == false . We applied your workaround as well

@ajcvickers
Copy link
Member

Ping @maumar

@maumar
Copy link
Contributor

maumar commented Dec 16, 2020

In a5f3e26 we unified the translation for !a, a != true, a == false where a is non nullable bool. They now all translate to !a. Later, if that expression is in the predicate (on sql server) we must rewrite it to a search condition: !a -> !(a == true) -> a != true

We could special case the final step on that translation (SimplifyNegatedBinary), when comparing to bool constant so that it always uses equality, rather than non-equality. This transformation should be safe to do, assuming c# null semantics are being used - in case of negated comparisons we weed out nullability completely in the null semantics

@maumar maumar changed the title !boolean in query !boolean in query should convert to boolean == false rather than boolean != true where possible Dec 17, 2020
maumar added a commit that referenced this issue Dec 17, 2020
…rather than boolean != true where possible

When comparing something to bool constant, which is later negated we now flip the constant true -> false and false -> true, rather than flipping the operation from == to !=
@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 17, 2020
maumar added a commit that referenced this issue Dec 17, 2020
…rather than boolean != true where possible

When comparing something to bool constant, which is later negated we now flip the constant true -> false and false -> true, rather than flipping the operation from == to !=
maumar added a commit that referenced this issue Dec 17, 2020
…rather than boolean != true where possible

When comparing something to bool constant, which is later negated we now flip the constant true -> false and false -> true, rather than flipping the operation from == to !=
maumar added a commit that referenced this issue Dec 17, 2020
…rather than boolean != true where possible

When comparing something to bool constant, which is later negated we now flip the constant true -> false and false -> true, rather than flipping the operation from == to !=
maumar added a commit that referenced this issue Dec 17, 2020
…rather than boolean != true where possible

When comparing something to bool constant, which is later negated we now flip the constant true -> false and false -> true, rather than flipping the operation from == to !=
maumar added a commit that referenced this issue Dec 17, 2020
…rather than boolean != true where possible

When comparing something to bool constant, which is later negated we now flip the constant true -> false and false -> true, rather than flipping the operation from == to !=
maumar added a commit that referenced this issue Dec 18, 2020
…rather than boolean != true where possible

When comparing something to bool constant, which is later negated we now flip the constant true -> false and false -> true, rather than flipping the operation from == to !=
@ajcvickers ajcvickers added this to the 6.0.0 milestone Jan 27, 2021
@Chrille79
Copy link

Is there any workaround for EF 5?


SELECT TOP(1000) [t].[Id]
FROM [Transaction] AS [t]
WHERE [t].[Computed] <> CAST(1 AS bit)
ORDER BY [t].[Id]

--SQL Server Execution Times: CPU time = 17797 ms,  elapsed time = 23389 ms.
 
 
SELECT TOP(1000) [t].[Id]
FROM [Transaction] AS [t]
WHERE [t].[Computed] = CAST(0 AS bit)
ORDER BY [t].[Id]

--SQL Server Execution Times: CPU time = 0 ms,  elapsed time = 1 ms.

The only workaround we know for the moment is going to FromSQL or not to upgrade and wait for EF 6

@joakimriedel
Copy link
Contributor

Is there any workaround for EF 5?

Did you try the workaround I posted in the beginning of this thread? It helped for me.

@Chrille79
Copy link

@joakimriedel tnx, seams to work with CONVERT(bit, [t].[Computed]) = CAST(0 AS bit)
was trying all the other options.

@bbycroft
Copy link

bbycroft commented Sep 2, 2021

Note that the workaround (Convert.ToBoolean(x.IsDeleted) == false) doesn't work in the case where the non-nullable bit column is used in a multi-column index (as opposed to an index filter clause).

e.g. if the index is defined as CREATE INDEX MyIdx ON MyTable (IsDeleted, CreateTime), then
WHERE CONVERT(bit, [x].[IsDeleted]) = CAST(0 AS bit) doesn't match against that index.

I'm using latest stable EF Core 5.0.9 & Microsoft SQL Server 2019 (RTM) - 15.0.2000.5 (X64), and I've run out of workarounds using Linq against this db structure.

@gparissis
Copy link

Note that the workaround (Convert.ToBoolean(x.IsDeleted) == false) doesn't work in the case where the non-nullable bit column is used in a multi-column index (as opposed to an index filter clause).

e.g. if the index is defined as CREATE INDEX MyIdx ON MyTable (IsDeleted, CreateTime), then WHERE CONVERT(bit, [x].[IsDeleted]) = CAST(0 AS bit) doesn't match against that index.

I'm using latest stable EF Core 5.0.9 & Microsoft SQL Server 2019 (RTM) - 15.0.2000.5 (X64), and I've run out of workarounds using Linq against this db structure.

Is this going to be corrected with EF Core 6? The workaround is not working for me because of the reason explained above.
I am using a filtered index on the bool column and the conversion is not using the index.

@ajcvickers
Copy link
Member

@gparissis This is fixed in 6.0. Please try with EF Core 6.0 RC2.

@gparissis
Copy link

gparissis commented Oct 19, 2021

@gparissis This is fixed in 6.0. Please try with EF Core 6.0 RC2.

Thanks, can't upgrade to a non production version yet, because of compliance restrictions, but I just found a workaround that worked perfect. I used a value conversion and the SQL on the column is generated as [i].[voided] = 0.

What I did:
modelBuilder.Entity<ItemFact>().Property(x => x.Voided).HasConversion<int>();

The Voided is a bool property that before was translating to
[i].[voided] <> CAST(1 AS bit) but after the conversion it translated to [i].[voided] = 0 when I execute the below query
FetchCurrentFact<ItemFact, DataContext>( x => x.Key == key && x.Voided == false, ctx)

Hope it helps.

@granit1986
Copy link

@gparissis This is fixed in 6.0. Please try with EF Core 6.0 RC2.

I tried but doesn't work

@roji
Copy link
Member

roji commented Oct 26, 2021

@gparissis can you share a code repro with 6.0 rc2 which shows the issue still happening?

@ajcvickers ajcvickers modified the milestones: 6.0.0-preview1, 6.0.0 Nov 8, 2021
@diogonborges

This comment has been minimized.

@diogonborges

This comment has been minimized.

@roji

This comment has been minimized.

@EhsanMrf
Copy link

EhsanMrf commented Mar 19, 2022

Hi, I have this problem, but I tried very hard but I did not succeed
I am using this version 6.0.2

See the pictures below

Return of two records
But the duration is 14 milliseconds

But the second image
12 records returned but duration 2 milliseconds

Two points:
Tip 1: Because the IsInStoock column is not indexed
It may act slowly

Tip 2:
CAST (1 bit AS)

This is an image
This is an image

Question?
How problematic is this break in the above records?
@roji Rojansky

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-perf 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