LINQ "==" operator generates OR with IS NULL

[jeanrbastos]

When I do this query:

var query = from entity in session.Query<Entity>()
            where entity.Status != 2
            select entity;

NHibernate generate this SQL:

SELECT ...
FROM TABLE_NAME ALIAS
WHERE ALIAS.SITFIL <> 2 
       or ALIAS.SITFIL is null /*this 'or' is the problem*/

Why NHibernate do this?
The 'or' causes a performance issue on application I want to generate just:

SELECT ...
FROM TABLE_NAME ALIAS
WHERE ALIAS.SITFIL <> 2 

Without 'Or field is null'
The mapping not is nullable:

[Property(Column = "SITFIL", NotNull = true)]
public virtual TypeEnumOfProperty Status { get; set; }

The enum:

public enum TypeEnumOfProperty
{
    [Display(Name = "Ativa", Description ="Ativo")]
    Ativa = 0,
    [Display(Name = "Inativa", Description = "Inativo")]
    Inativa = 1,
    [Display(Name = "Inativa e oculta", Description = "Inativo e oculto")]
    InativaOculta = 2,
}

If I change the TypeEnumOfProperty to int the SQL is generated like I want, if change to TypeEnumOfProperty or long then was generated with the 'or'.

If I use 'equals' on comparision, NHibernate generates the where clause without 'or'.

[gliljas]

If you explicitly set it to be not nullable? In a nullable scenario the query is correct.

[jeanrbastos]

@gliljas I tried to set nullable, but keeps generating 'or'.
If I use type 'int' works right.

[hazzik]

The query is correct and this intentional. Unfortunately Linq lacks reflection capability to see what the actual mapped type (and its options) is in such a case, because it’s too early in the pipeline.

There are different goals for HQL and Linq providers. HQL wants to be closer as possible to the actual SQL generated. But Linq wants to be closer to Linq-to-object provider as a reference.

[jeanrbastos]

The query is correct and this intentional. Unfortunately Linq lacks reflection capability to see what the actual mapped type (and its options) is in such a case, because it’s too early in the pipeline.

There are different goals for HQL and Linq providers. HQL wants to be closer as possible to the actual SQL generated. But Linq wants to be closer to Linq-to-object provider as a reference.

@hazzik when I put NotNull = true in the mapping, should generate without the 'or is null'?

[bahusoid]

The query is correct and this intentional.

But it's really unfortunate - I've also experienced issues when null check made MS SQL Server to ignore index and made query super slow. Query generator should generate null checks only for nullable fields.

@jeanrbastos As a workaround you can generate this query via QueryOver:

var entities = session.QueryOver<Entity>().Where(e => e.Status != 2).List();

[fredericDelaporte]

When the property .Net type is not nullable, I think the Linq provider should not issue the comparison to null. From a linq-to-object, it makes no sense, since the property cannot be null.

If the column is actually nullable in the database and have null values, this is a mapping mismatch which responsibility lies on the user (since .Net Framework 2), and which should be fixed either by ceasing having a nullable column or by changing the property .Net type to something nullable.

Of course this would not account notably for the string case, always nullable in .Net while it could be not nullable in database. But I think it would be still better.

[gliljas]

I agree.

[fredericDelaporte]

There is a case where dropping the null test could be considered bad: when the non-nullable property is navigated to through a nullable association. But for such a case, in linq-to-object, the query would require an explicit null guard on the association, so I think this case is not really troublesome.

[patearl]

Based on https://nhibernate.jira.com/browse/NH-2402, I would have said that the new behavior should be reverted, but I see that EF Core has different null behavior than before as well. While I haven't analyzed the behavior, I think it would be reasonable to make the same trade-offs that EF Core is making. This will lead to maximum future compatibility with code that generates Linq queries automatically.

[fredericDelaporte]

There is some history about this handling:

NH-2402, NH v3:

As per analysis posted on the development mailing list, it seems that it is expected that LINQ equality will map directly to SQL equality and will not have special handling for NULL built-in. This would mean that in cases where there's no null parameter, an == expression would map directly to = in SQL. Here's a summary of the results from the mailing list with option A being direct mapping and option B using special handling for nulls.
Code Votes:
A = Linq to SQL v4
A = Entity Framework v4
A = llblgen
A = Criteria
A = Hql
A = QueryOver
A = Old NHibernate.Linq
B = New NHibernate.Linq

Individual Votes:
A
A
A

So those who have expressed their opinion at that time were all for not trying to emulate linq-to-object about null semantic. This discussion is here.

Nonetheless, changes have later occurred in v5, seemingly overlooking the discussion and decision in NH-2402.

They have been triggered by NH-3100, which reported a valid bug, but this has caused reconsidering null handling. Apparently, at that time EF had changed too and was also no more following option A as defined in NH-2402, which adds an additional justification for the change.

In EF they do very-deep null propagation analysis on expression and extend it to conditional/more complex expression. This is not a matter of just adding random case statements to the query. I'll nominate this to 5.0

This has been done with #516.

It has triggered some concerns after release, like #1495, but these concerns were discarded by further explaining why this change has been done:

The Linq semantic should match across providers (Linq-to-objects, NH, EF, etc), and this case was a clear bug. So it needs to be a little sophisticated about how to translate the expression tree to the SQL. The QueryOver, on the other hand, is dumber and requires more explicit expressions. But these query providers are not directly comparable.

I think as a probable solution we could make IsNullable check in TranslateInequalityComparison more clever to detect nullable types.

Still, the subject of unneeded null check as raised by @jeanrbastos was written about, but not translated to an issue for eventually tackling it some day. #1860 here now does that, which is fine.

I do not think we should go back to NH-2402 behavior. The situation has changed since then, NH is no more the only one bridging the gap between .Net null semantic and SQL one. The issue here should only target removing useless null checks, the ones done when the operands are not supposed to be potentially null.

#516 has also fixed #1042 by the way. If #516 is to be reverted, this case and maybe some others not reported or that I have not seen, would have to be fixed again.

[patearl]

This issue was investigated previously with a different conclusion... as I see you referenced:

https://nhibernate.jira.com/browse/NH-2402

However, now EF Core has different null handling behavior than the previous frameworks. I support following whatever design decisions were made in EF Core to maintain maximum future compatibility of the LINQ provider. Beyond making ourselves compatible with new EF Core behavior, I don't stand behind the conceptual goal of being the same as LINQ to Objects. There will always be differences and trade-offs that affect the simplicity of SQL generated. We need to avoid LINQ being useless for database queries because it has a bunch of auxiliary code generated just for the purpose of making it more similar to the in-memory version of the same.

[gliljas]

This bit us in production today. I will create a local workaround that reverts to the old behavior, but this topic needs to be discussed and addressed.

[hazzik]

How did it impact you, @gliljas?

[gliljas]

Two ways. A suboptimal query plan + the query with " x != "" " started returning rows with x = null.

I understand the rationale behind the change, and I can live with the latter issue (returning with null), but the query plan issue is much worse.

[gliljas]

On closer inspection, the query plan issue may not be as bad as it first appeared.

[larini]

How did it impact you, @gliljas?

Hi @hazzik, the impact for my case is that oracle looses the index and a query that takes less than a second now takes 2 minutes to run.

in v4:

guia5_.PEGID = peg1_.ID

in v5:

guia5_.PEGID = peg1_.ID   
   or (guia5_.PEGID is null)
  and (peg1_.ID is null)     

[larini]

Any news on this?

[senhorlopes]

I've got the same problem! Any news?

[maca88]

As #2036 was recently approved, I am planning to update #1996 once it is merged, as it contains a more robust method to retrieve mapping information from expressions. After that, #1996 will be ready for review and hopefully added in the next release.