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

Arithmetic potential null values #8014

Closed
joacar opened this issue Mar 28, 2017 · 10 comments
Closed

Arithmetic potential null values #8014

joacar opened this issue Mar 28, 2017 · 10 comments
Labels
closed-no-further-action The issue is closed and no further action is planned.

Comments

@joacar
Copy link

joacar commented Mar 28, 2017

Arithmetic values are not treated the same if null coalesce operator is used instead of HasValue on nullable integer types

Steps to reproduce

Project available here and source code

Two expressions that, imo, should yield the same result doesn't
1.

a => (a.A1.Cost ?? 0 + a.A2.Cost ?? 0 + a.A3.Cost) > limit;
SELECT *
FROM [SetA] AS [a]
LEFT JOIN [A3] AS [a.A3] ON [a].[A3Id] = [a.A3].[Id]
LEFT JOIN [A2] AS [a.A2] ON [a].[A2Id] = [a.A2].[Id]
LEFT JOIN [A1] AS [a.A1] ON [a].[A1Id] = [a.A1].[Id]
WHERE COALESCE([a.A1].[Cost], COALESCE(0 + [a.A2].[Cost], 0 + [a.A3].[Cost])) > 10
ORDER BY [a].[A3Id], [a].[A2Id], [a].[A1Id]
a => ((a.A1.Cost.HasValue ? a.A1.Cost.Value : 0) +
          (a.A2.Cost.HasValue ? a.A2.Cost.Value : 0)
          + a.A3.Cost) > limit;
SELECT*
FROM [SetA] AS [a]
LEFT JOIN [A3] AS [a.A3] ON [a].[A3Id] = [a.A3].[Id]
LEFT JOIN [A2] AS [a.A2] ON [a].[A2Id] = [a.A2].[Id]
LEFT JOIN [A1] AS [a.A1] ON [a].[A1Id] = [a.A1].[Id]
WHERE ((CASE
    WHEN [a.A1].[Cost] IS NOT NULL
    THEN [a.A1].[Cost] ELSE 0
END + CASE
    WHEN [a.A2].[Cost] IS NOT NULL
    THEN [a.A2].[Cost] ELSE 0
END) + [a.A3].[Cost]) > 10
ORDER BY [a].[A3Id], [a].[A2Id], [a].[A1Id]

Perhaps because if SQL server backwards compatability reasons, but couldn't above just be rewritten with ISNULL(..., 0)? Probably no performance gains, but the SQL would be more readable :)

Further technical details

EF Core version: 1.1.1
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10
IDE: Visual Studio 2017 Community

@smitpatel
Copy link
Contributor

Addition (+) has higher operator precedence in C# than coalesce (??). So effectively first expression becomes a => (a.A1.Cost ?? (0 + a.A2.Cost) ?? (0 + a.A3.Cost)) > limit; Which makes both the expressions different and yields different translation. (both of them are correct from what has been provided in user query).

Can you check what is the query generated when you write the first expression like this
a => ((a.A1.Cost ?? 0) + (a.A2.Cost ?? 0) + a.A3.Cost) > limit;

@joacar
Copy link
Author

joacar commented Mar 28, 2017

@smitpatel Thanks for quick answer.

a => (a.A1.Cost ?? 0) + (a.A2.Cost ?? 0) + a.A3.Cost > limit;
SELECT *
FROM [SetA] AS [a]
LEFT JOIN [A3] AS [a.A3] ON [a].[A3Id] = [a.A3].[Id]
LEFT JOIN [A2] AS [a.A2] ON [a].[A2Id] = [a.A2].[Id]
LEFT JOIN [A1] AS [a.A1] ON [a].[A1Id] = [a.A1].[Id]
WHERE ((COALESCE([a.A1].[Cost], 0) + COALESCE([a.A2].[Cost], 0)) + [a.A3].[Cost]) > 100
ORDER BY [a].[A3Id], [a].[A2Id], [a].[A1Id]

Looks good 👍

Is there any way that is "better" than the other? The generated SQL looks more attractive, I'll use it soley for that ;)

@smitpatel
Copy link
Contributor

Is there any way that is "better" than the other? The generated SQL looks more attractive, I'll use it soley for that ;)

According to https://docs.microsoft.com/en-us/sql/t-sql/language-elements/coalesce-transact-sql,
The COALESCE expression is a syntactic shortcut for the CASE expression and rewritten by query optimizer into CASE statements.

So, both are same performance-wise. COALESCE generates a shorter (and prettier may be) query. 😄

@joacar joacar closed this as completed Mar 28, 2017
@smitpatel smitpatel added the closed-no-further-action The issue is closed and no further action is planned. label Mar 28, 2017
@joacar
Copy link
Author

joacar commented Mar 28, 2017

@smitpatel

Added three statements with ??-operator and everything works, of course, but there seem the be exessive parenthesis

a => (a.A1.Cost ?? 0) + (a.A2.Cost ?? 0) + (a.A3.Cost ?? 0) > limit;

produces

((COALESCE([a.A1].[Cost], 0) + COALESCE([a.A2].[Cost], 0)) + COALESCE([a.A3].[Cost], 0))

Tried adding parenthesis around the three statements but the generated sql is the same

@smitpatel
Copy link
Contributor

During our SQL generation, we don't try to reason about operator precedence in SQL. It would require us to look into all nodes together & apply complex logic. Also precedence can vary between different providers. Therefore we just generate SQL with explicit operator precedence.
I would expect a + b + c which is interpreted by compiler (and thus query parser) as (a + b) + c would be translated to SQL with parenthesis only.

@joacar
Copy link
Author

joacar commented Mar 28, 2017

Fair point :)

@joacar
Copy link
Author

joacar commented Apr 21, 2017

Edit: Didn't meen to re-open...

@smitpatel "All of a sudden" my custom sorting expressions stopped working. The generated SQL is

Microsoft.EntityFrameworkCore.Query.Internal.SqlServerQueryCompilationContextFactory:Warning: The LINQ expression 'orderby ((((?[p.Instagram].Cost?) ?? 0) + ((?[p.Blog].Cost?) ?? 0)) + ([p].Youtube.Cost ?? 0)) desc' could not be translated and will be evaluated locally

instead of correctly applying COALESCE operator. However this issue is not present in the linked test project allthough they reference the same version of EF, namely Microsoft.EntityFrameworkCore.SqlServer (1.1.1).

The project that doesn't work is a ASP.NET Core MVC project so perhaps some other library causing this missmatch.

Function:

public static Expression<Func<Profile, int>> OrderByCost()
            {
                return p => (p.Instagram.Cost ?? 0) +
                    (p.Blog.Cost ?? 0) +
                   (p.Youtube.Cost ?? 0);
            }

@joacar joacar reopened this Apr 21, 2017
@ajcvickers ajcvickers added this to the 2.0.0 milestone Apr 21, 2017
@smitpatel
Copy link
Contributor

This works in EF.
Generated SQL:
version 1.1.1

SELECT [p].[Id], [p].[BlogId], [p].[InstagramId], [p].[YoutubeId], [p.Youtube].[Id], [p.Youtube].[Cost], [p.Blog].[Id], [p.Blog].[Cost], [p.Instagram].[Id], [p.Instagram].[Cost]
FROM [Roots] AS [p]
LEFT JOIN [Youtube] AS [p.Youtube] ON [p].[YoutubeId] = [p.Youtube].[Id]
LEFT JOIN [Blog] AS [p.Blog] ON [p].[BlogId] = [p.Blog].[Id]
LEFT JOIN [Instagram] AS [p.Instagram] ON [p].[InstagramId] = [p.Instagram].[Id]
ORDER BY (COALESCE([p.Instagram].[Cost], 0) + COALESCE([p.Blog].[Cost], 0)) + COALESCE([p.Youtube].[Cost], 0), [p].[YoutubeId], [p].[BlogId], [p].[InstagramId]

Extra order by and all columns in projection is due to #6647

In nightly builds

SELECT [p].[Id], [p].[BlogId], [p].[InstagramId], [p].[YoutubeId]
FROM [Roots] AS [p]
LEFT JOIN [Youtube] AS [p.Youtube] ON [p].[YoutubeId] = [p.Youtube].[Id]
LEFT JOIN [Blog] AS [p.Blog] ON [p].[BlogId] = [p.Blog].[Id]
LEFT JOIN [Instagram] AS [p.Instagram] ON [p].[InstagramId] = [p.Instagram].[Id]
ORDER BY (COALESCE([p.Instagram].[Cost], 0) + COALESCE([p.Blog].[Cost], 0)) + COALESCE([p.Youtube].[Cost], 0)

Probably there is some error with your project setup or exact packages they are referencing.

@joacar
Copy link
Author

joacar commented Apr 24, 2017

So this will be incredibly hard to debug... I'll add a WebApplication project and see if I can reproduce it within that context. It runs fine in console mode and only referencing EF-packages

@joacar
Copy link
Author

joacar commented Apr 26, 2017

Not a big surprise that there was a application error - a change in the datamodel was the cause. Had a property on my domain model that no longer mapped against a column in the database hence the issue.

Sorry for taking your time!

@joacar joacar closed this as completed Apr 26, 2017
@ajcvickers ajcvickers removed this from the 2.0.0 milestone Apr 26, 2017
@smitpatel smitpatel removed their assignment Jan 12, 2022
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned.
Projects
None yet
Development

No branches or pull requests

3 participants