SQL Server: consider switching from COALESCE to ISNULL

[roji]

It seems that COALESCE may be expanded to a CASE/WHEN, which causes the expression to get evaluated twice (link); ISNULL apparently doesn't have this behavior. This has a performance impact (i.e. when the expression is a heavy subquery), but can also lead to incorrect results when the expression is non-deterministic and can return different results each time it's evaluated:

CREATE TABLE Foo (A int, B int);
INSERT INTO Foo (A, B) VALUES (1, NULL), (2, 2), (3, NULL);

SELECT COALESCE((SELECT TOP 1 B FROM Foo ORDER BY NEWID() ASC), 0); -- sometimes returns NULL!

Also, check if the same thing happens on other databases, i.e. whether this should be a SQL Server-only change or something with a wider scope.

SQL Server documentation detailing all the above, plus the differences between COALESCE and ISNULL. See also (this post) on that.

Thanks for @John0King for reporting this (see #22243 (comment)).

[roji]

A quick check on PostgreSQL, SQLite and MariaDB doesn't show the same issue occuring. That is, with the above code, one always gets either 0 or 2, never NULL.

[Matthew-Ricks-USBE]

There are differences between COALESCE and ISNULL, but +1 for adding at least limited use of ISNULL. I have a use case where I'm summing a correlated subquery and comparing that against a known value. It's a dynamic piece added via Where, but it's something like this:

var values = new [] { "A", "B" }; // Dynamic.
return (TargetEntity te) => te.EnumerableProperty
    .Distinct()
    .Select(v => values.Contains(v) ? 1 : values.Length + 1)
    .Sum() != values.Length;

The result of SUM is treated as nullable by the generated SQL, so a second clause checking for null is added which hurts performance. I can add a coalesce sum ?? 0, but the generated SQL uses COALESCE and takes several minutes, often hitting my timeout. Swapping for ISNULL runs in less than a minute.

Fortunately I don't expect this piece of functionality to have much if any demand/use, so I'm tolerating the timeouts for now, but I'd love it if this kept up with our other functions.

[roji]

@Matthew-Ricks-USBE that very specific case should be covered by #32574 (draft PR #32575 is out); it's more a case of problematic translation of the Contains (in this very specific case) than a problem with COALESCE itself.

[Matthew-Ricks-USBE]

I don't think they're related. The Select and Sum from the sample code translate to:

SELECT SUM(
    CASE
        WHEN [t].[ENUMERATED_THING] IN ( N'A', N'B' ) THEN 1
        ELSE 3
    END
)

That piece of the translation makes sense to me, but maybe I'm missing something.

[roji]

@Matthew-Ricks-USBE I'm assuming you're using SQL Server - you may want to give 8.0 a try, it generates very different SQL. When using that, PR #32575 is about actually detected if there are nulls inside the parameterized list, and generating better SQL based on that; think about it as if you were doing the same query but with a collection of ints (non-nullable), rather than strings (nullable). In other words, ideally EF wouldn't even add the second clause for checking null.

But I take your general point... Switching from COALESCE to ISNULL is definitely something we should look into.

[ranma42]

I would like to try and do this transformation.
IIUC ISNULL has a different behavior regarding the type of the result (it always uses the type of the first expression).
Besides this, is there any other thing I should be careful about?

[ranma42]

@raymens mentions that another important difference is that ISNULL can be used in indexed views; see #34171 (comment)

[raymens]

Thanks @ranma42 , meant to put that comment here but I guess I confused the open tabs :)

For anyone facing the same issue related to using an aggregated indexed view, I've created a workaround by registering a custom function:

public static class Functions
{
    public static decimal IsNull(decimal? first, decimal def)
    {
        return first ?? def;
    }
}

// db context:
        modelBuilder
            .HasDbFunction(
                typeof(Functions).GetMethod(nameof(Functions.IsNull), [typeof(decimal?), typeof(decimal)]))
            .HasTranslation(
                args =>
                    new SqlFunctionExpression(
                        "ISNULL",
                        args,
                        nullable: false,
                        argumentsPropagateNullability: [false, false],
                        type: args[0].Type,
                        typeMapping: args[0].TypeMapping));

// usage
db.SomeTable.Sum(a => Functions.IsNull(a.Column1, 0));