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

Consider removing the CAST to BIT for boolean literal values #27150

Open
roji opened this issue Jan 10, 2022 · 13 comments
Open

Consider removing the CAST to BIT for boolean literal values #27150

roji opened this issue Jan 10, 2022 · 13 comments

Comments

@roji
Copy link
Member

roji commented Jan 10, 2022

Our SQL representation of boolean literal values is CAST(1 AS BIT). However, in at least some cases, the CAST produces an inferior query plan. The following example shows this when calling hierarchyid's IsDescendantOf:

Hierarchyid plan without CAST

SELECT COUNT(*) FROM Data WHERE Hid.IsDescendantOf('/') = 1;

Plan:

SELECT COUNT(*) FROM Data WHERE Hid.IsDescendantOf('/') = 1,1,1,0,,,1,,1,,,,4.456616,,,SELECT,false,
"  |--Compute Scalar(DEFINE:([Expr1002]=CONVERT_IMPLICIT(int,[Expr1003],0)))",1,2,1,Compute Scalar,Compute Scalar,"DEFINE:([Expr1002]=CONVERT_IMPLICIT(int,[Expr1003],0))","[Expr1002]=CONVERT_IMPLICIT(int,[Expr1003],0)",1,0,0,11,4.456616,[Expr1002],,PLAN_ROW,false,1
       |--Stream Aggregate(DEFINE:([Expr1003]=Count(*))),1,3,2,Stream Aggregate,Aggregate,,[Expr1003]=Count(*),1,0,0.6000005,11,4.456616,[Expr1003],,PLAN_ROW,false,1
"            |--Index Seek(OBJECT:([master].[dbo].[Data].[IX_name]), SEEK:([master].[dbo].[Data].[Hid] >= / AND [master].[dbo].[Data].[Hid] <= Showplan: failed to convert to string from hierarchyid) ORDERED FORWARD)",1,4,3,Index Seek,Index Seek,"OBJECT:([master].[dbo].[Data].[IX_name]), SEEK:([master].[dbo].[Data].[Hid] >= / AND [master].[dbo].[Data].[Hid] <= Showplan: failed to convert to string from hierarchyid) ORDERED FORWARD",,1000000,2.7564583,1.100157,9,3.8566153,,,PLAN_ROW,false,1

Hierarchyid plan with CAST

SELECT COUNT(*) FROM Data WHERE Hid.IsDescendantOf('/') = CAST(1 AS bit);

Plan:

SELECT COUNT(*) FROM Data WHERE Hid.IsDescendantOf('/') = CAST(1 AS bit),1,1,0,,,1,,1,,,,4.6366158,,,SELECT,false,
"  |--Compute Scalar(DEFINE:([Expr1002]=CONVERT_IMPLICIT(int,[Expr1004],0)))",1,2,1,Compute Scalar,Compute Scalar,"DEFINE:([Expr1002]=CONVERT_IMPLICIT(int,[Expr1004],0))","[Expr1002]=CONVERT_IMPLICIT(int,[Expr1004],0)",1,0,0,11,4.6366158,[Expr1002],,PLAN_ROW,false,1
       |--Stream Aggregate(DEFINE:([Expr1004]=Count(*))),1,3,2,Stream Aggregate,Aggregate,,[Expr1004]=Count(*),1,0,0.6000005,11,4.6366158,[Expr1004],,PLAN_ROW,false,1
            |--Filter(WHERE:([master].[dbo].[Data].[Hid].IsDescendantOf(/)=(1))),1,4,3,Filter,Filter,WHERE:([master].[dbo].[Data].[Hid].IsDescendantOf(/)=(1)),,1000000,0,0.18,9,4.0366154,,,PLAN_ROW,false,1
                 |--Index Scan(OBJECT:([master].[dbo].[Data].[IX_name])),1,5,4,Index Scan,Index Scan,OBJECT:([master].[dbo].[Data].[IX_name]),[master].[dbo].[Data].[Hid],1000000,2.7564583,1.100157,13,3.8566153,[master].[dbo].[Data].[Hid],,PLAN_ROW,false,1

Test SQL

hierarchyid test
CREATE TABLE Data (Id INT IDENTITY(1,1) PRIMARY KEY, Hid HIERARCHYID);
CREATE INDEX IX_name ON Data(Hid);

BEGIN TRANSACTION;

DECLARE @i INT = 0;
WHILE @i < 50000
BEGIN
    INSERT INTO Data (Hid) VALUES ('/1/3/');
    SET @i = @i + 1;
END;
COMMIT;

SET SHOWPLAN_ALL ON;
SELECT COUNT(*) FROM Data WHERE Hid.IsDescendantOf('/') = 1;
SELECT COUNT(*) FROM Data WHERE Hid.IsDescendantOf('/') = CAST(1 AS bit);
SET SHOWPLAN_ALL OFF;

Note that in other scenarios, e.g. simply comparing a regular BIT column to literal true/false, the degradation does not occur (this could be specific to hierarchyid, though who knows):

Regular column test
CREATE TABLE Data (Id INT IDENTITY(1,1) PRIMARY KEY, IsSomething BIT);
CREATE INDEX IX_name ON Data(IsSomething);

BEGIN TRANSACTION;

DECLARE @i INT = 0;
WHILE @i < 500000
BEGIN
    INSERT INTO Data (IsSomething) VALUES (1);
    SET @i = @i + 1;
END;

WHILE @i < 1000000
BEGIN
    INSERT INTO Data (IsSomething) VALUES (0);
    SET @i = @i + 1;
END;

COMMIT;

SET SHOWPLAN_ALL ON;
SELECT COUNT(*) FROM Data WHERE IsSomething = 1;
SELECT COUNT(*) FROM Data WHERE IsSomething = CAST(1 AS bit);
SET SHOWPLAN_ALL OFF;

Flagged by @diogonborges in #23472 (comment)

@ErikEJ
Copy link
Contributor

ErikEJ commented Jan 10, 2022

Is hierarchyid officially supported in the SQL Server provider?

@roji
Copy link
Member Author

roji commented Jan 10, 2022

There's an unofficial extension we maintain.

But my fear is that if there's a different plan in this case, there could be other cases we don't know about. Yet another example that generating minimal/lighter SQL is always a good idea.

@diogonborges
Copy link

I'm curious as if, in the meanwhile, there anything that our code can tap into to hotfix, before this issue gets to see daylight?
Does EFCore allow customization of default representation for literal values? I'm looking to avoid using interceptors, so I'm hoping that that isn't the last resource.

@roji
Copy link
Member Author

roji commented Jan 10, 2022

A command interceptor is probably the reasonable thing to do here: you would identify commands that do hierarchy operations (either by tagging them with EF Core, or simply by searching for hierarchy ID function names), and replace CAST(1 AS BIT) with 1. You could also write an expression visitor to remove the casting inside the query pipeline, but I'm honestly not sure that's better in this case.

@smitpatel
Copy link
Contributor

Blocked on #15586

@roji
Copy link
Member Author

roji commented Jan 11, 2022

The general objective here is to end up with 1 as the bool literal on SQL Server instead of CAST(1 AS BIT).

If I understand correctly, the approach blocked on #15586 would involve comparing to an int (requiring knowledge on what's comparable to what in SQL).

An alternative would be to simply change the literal representation in SqlServerBoolTypeMapping to simply not have the CAST. This would cause issues when a bool constant is being projected (#24075), since a bare 1 or 0 can't be read into a .NET bool.

I think that should be considered as part of the more global problem of projecting constant representations with evaluate to a different type - continuing this thought in #24075.

@iduras3
Copy link

iduras3 commented May 4, 2022

I am currently running in this issue, is there a work around for this ?

@roji
Copy link
Member Author

roji commented May 4, 2022

@iduras3 see the command interceptor approach discussed above.

@scp-mb
Copy link

scp-mb commented Oct 5, 2022

We're seeing a very similar issue however we're using external tables in Azure SQL. It's supposed to push the WHERE clause to the remote database server but having a CAST(0 AS BIT) in the query is causing it to instead pull the whole table in, then do the WHERE filter locally instead.

In the example below, we're using EF global filters to not pull back any soft deleted records

Expression<Func<object, bool>> IsSoftDeletedFilter => e => ((ISoftDeletable)e) == null || !((ISoftDeletable)e).IsDeleted;

SELECT [d1].[Id], [d1].[CompanyId], [d1].[IsDeleted], [d1].[Name], [d1].[OrderBy]
    FROM [Directory_Nationality] AS [d1]
    WHERE ([d1].[IsDeleted] = CAST(0 AS bit))

image

SELECT [d1].[Id], [d1].[CompanyId], [d1].[IsDeleted], [d1].[Name], [d1].[OrderBy]
    FROM [Directory_Nationality] AS [d1]
    WHERE ([d1].[IsDeleted] = 0)

image

@hgedik
Copy link

hgedik commented May 18, 2023

For a detailed look at the bool/bit cast problem on the global QueryFilter.

#29930

@ajcvickers ajcvickers removed their assignment Aug 31, 2024
@jwyza-pi
Copy link

jwyza-pi commented Oct 7, 2024

For folks who come to this issue and are interested, this is what I came up with for the Interceptor suggestion. It handles the projection problem by checking to see if the CAST is happening right before AS [someColumn]. If it is, then don't change it. Not sure if this will cover every scenario, so use at your own risk/discretion.

/// <summary>
/// Intercepter to replace the <c>CAST(0 AS bit)</c> with <c>0</c> and <c>CAST(1 AS bit)</c> with <c>1</c>.
/// This works around https://github.com/dotnet/efcore/issues/27150 which can cause performance issues.
/// </summary>
/// <remarks>Does not replace <c>CAST(0 AS bit)</c> when followed by <c>AS [somename]</c> so as not to break projection.  Also doesn't do the replacement in the scenario of CASE THEN ELSE results.</remarks>
internal partial class BooleanCommandInterceptor : DbCommandInterceptor
{
    [GeneratedRegex(@"(?<!\bTHEN\s+|\bELSE\s+)CAST\((0|1) AS bit\)(?!\s+AS\s+\[\w+\])")]
    private static partial Regex BitCastRegex();
    
    public override InterceptionResult<DbDataReader> ReaderExecuting(
        DbCommand command,
        CommandEventData eventData,
        InterceptionResult<DbDataReader> result)
    {
        ManipulateCommand(command);

        return result;
    }

    public override ValueTask<InterceptionResult<DbDataReader>> ReaderExecutingAsync(
        DbCommand command,
        CommandEventData eventData,
        InterceptionResult<DbDataReader> result,
        CancellationToken cancellationToken = default)
    {
        ManipulateCommand(command);

        return new ValueTask<InterceptionResult<DbDataReader>>(result);
    }

    private static void ManipulateCommand(DbCommand command)
    {
        var matches = BitCastRegex().Matches(command.CommandText);
        if (matches.Count != 0)
        {
            command.CommandText = BitCastRegex().Replace(command.CommandText, "$1");
        }
    }
}

Edit: 10/15, fixed a problem where CASE THEN ELSE wouldn't work because it was removing the cast from the results. Fixed that.

@uittorio

This comment has been minimized.

@roji

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment