Fix comparison of nullable result of functions

[dmitry-lipetsk]

EF Core - master source

First example:

  [Table("TEST_MODIFY_ROW")]
  public sealed class TEST_RECORD
  {
   [Key]
   [Column("TEST_ID")]
   public Int64? TEST_ID { get; set; }

   [Column("COL_TIMESTAMP", TypeName="TIMESTAMP")]
   public DateTime? DATA { get; set; }
  };//class TEST_RECORD

// ....
     var recs=db.testTable.Where(r => r.DATA==r.DATA && r.TEST_ID==testID);

EFCore generates SQL:

WHERE (("r"."COL_TIMESTAMP" = "r"."COL_TIMESTAMP") OR ("r"."COL_TIMESTAMP" IS NULL AND "r"."COL_TIMESTAMP" IS NULL)) AND ("r"."TEST_ID" = :__testID_0)

It is OK,

Second example:

     var recs=db.testTable.Where(r => r.DATA.AddYears(1)==r.DATA.AddYears(1) && r.TEST_ID==testID);

Where used extension method:

public static System.Nullable<System.DateTime> AddYears
                                           (this System.Nullable<System.DateTime> obj,
                                            System.Nullable<System.Int32>         value)

EFCore generates SQL:

WHERE (DATEADD(YEAR,1,"r"."COL_TIMESTAMP") = DATEADD(YEAR,1,"r"."COL_TIMESTAMP")) AND ("r"."TEST_ID" = :__testID_0)

From my point of view - it is problem. In second case EFCore must generate SQL like SQL from first example:

WHERE ((DATEADD(YEAR,1,"r"."COL_TIMESTAMP") = DATEADD(YEAR,1,"r"."COL_TIMESTAMP")) OR (DATEADD(YEAR,1,"r"."COL_TIMESTAMP") IS NULL AND DATEADD(YEAR,1,"r"."COL_TIMESTAMP") IS NULL)) AND ("r"."TEST_ID" = :__testID_0)

[dmitry-lipetsk]

I found the point, where the "IS NULL"statement is builded:

https://github.com/aspnet/EntityFrameworkCore/blob/d59be61006d78d507dea07a9779c3c4103821ca3/src/EFCore.Relational/Query/ExpressionVisitors/Internal/IsNullExpressionBuildingVisitor.cs#L80-L95

I added to this method:

            if(extensionExpression.Type.IsNullableType())
            {
                AddToResult(new IsNullExpression(extensionExpression));

                return extensionExpression;
            }

And now my copy of EFCore generates expected condition for second example:

WHERE ((DATEADD(YEAR,1,"r"."COL_TIMESTAMP") = DATEADD(YEAR,1,"r"."COL_TIMESTAMP")) OR (DATEADD(YEAR,1,"r"."COL_TIMESTAMP") IS NULL AND DATEADD(YEAR,1,"r"."COL_TIMESTAMP") IS NULL)) AND ("r"."TEST_ID" = :__testID_0)

Could anybody verify this change with own tests of EFCore?

[dmitry-lipetsk]

Additional changes for IsNullExpressionBuildingVisitor::VisitBinary

            if (binaryExpression.Type.IsNullableType())
            {
                AddToResult(new IsNullExpression(binaryExpression));

                return binaryExpression;
            }//if

Need for correct translation of comparisons like

r.DATA == r.DATA.Substring(0,8)+"3"

Expected condition:

(("r"."COL_VARCHAR_10" = (SUBSTRING("r"."COL_VARCHAR_10" FROM 0 + 1 FOR 8)||_utf8 '3')) OR ("r"."COL_VARCHAR_10" IS NULL AND SUBSTRING("r"."COL_VARCHAR_10" FROM 0 + 1 FOR 8)||_utf8 '3' IS NULL))

[maumar]

The way I was thinking about this is that function is null if any of its arguments is null (doesn't apply to all functions like COALESCE but it does to most). So we don't need to test the entire function for NULL, just its arguments:

So it would look something like this:

WHERE ((DATEADD(YEAR,1,"r"."COL_TIMESTAMP") = DATEADD(YEAR,1,"r"."COL_TIMESTAMP")) OR ("r"."COL_TIMESTAMP" IS NULL AND "r"."COL_TIMESTAMP" IS NULL)) AND ("r"."TEST_ID" = :__testID_0)

further optimized to:

WHERE ((DATEADD(YEAR,1,"r"."COL_TIMESTAMP") = DATEADD(YEAR,1,"r"."COL_TIMESTAMP")) OR "r"."COL_TIMESTAMP" IS NULL ) AND ("r"."TEST_ID" = :__testID_0)

This would also work for functions that return non-nullable values, e.g. CHARINDEX('foo', myNullColumn) - sql will return null if myNullColumn is actually NULL, but the function itself returns int/bigint so we can't rely on return type alone here.

[dmitry-lipetsk]

Hello @maumar,

I think, in common case (at first stage) need generate expression:

WHERE ((DATEADD(YEAR,1,"r"."COL_TIMESTAMP") = DATEADD(YEAR,1,"r"."COL_TIMESTAMP")) OR (DATEADD(YEAR,1,"r"."COL_TIMESTAMP") IS NULL AND DATEADD(YEAR,1,"r"."COL_TIMESTAMP") IS NULL)) AND ("r"."TEST_ID" = :__testID_0)

Optimization of this expression - it is optional / additional / second stage.

Personally I not want to mix this stages - it is way for bad designe.

---

This issue is critical for me, I hope you will find the decision.

For example, you may offer the way for replacing of IsNullExpressionBuildingVisitor at data provider level.

[dmitry-lipetsk]

I not sure, but seem optimization of "DATEADD(YEAR,1,"r"."COL_TIMESTAMP") IS NULL))" may be implemented at PredicateReductionExpressionOptimizer level.

[maumar]

fixed in fd842ed