AddMinutes with Date cannot be translated when upgrading from 5.0.10

[petergummer]

The following program works with Npgsql.EntityFrameworkCore.PostgreSQL 5.0.10, but with 6.0.22 and 8.0 it cannot be translated.

using Microsoft.EntityFrameworkCore;

using System;
using System.Linq;

var ctx = new Ctx();
var utc = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified);
var query = ctx.Items.Where(i => i.Dt < utc.AddMinutes(i.OffsetMinutes).Date);
Console.WriteLine(query.ToQueryString());

public record Item(int Id, DateTime Dt, int OffsetMinutes);

public class Ctx : DbContext
{
    public DbSet<Item> Items { get; set; } = null!;

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) => optionsBuilder.UseNpgsql("Host=localhost");
}

This is its output with 5.0.10:

-- @__utc_0='2023-12-13T00:58:49.3731327' (DbType = DateTime)
SELECT i."Id", i."Dt", i."OffsetMinutes"
FROM "Items" AS i
WHERE i."Dt" < date_trunc('day', @__utc_0 + CAST((CAST(CAST(i."OffsetMinutes" AS double precision) AS text) || ' mins') AS interval))

And this is the error with 6.0.22 and 8.0:

Unhandled exception. System.InvalidOperationException: The LINQ expression 'DbSet<Item>()
    .Where(i => i.Dt < __utc_0.AddMinutes((double)i.OffsetMinutes).Date)' could not be translated. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to 'AsEnumerable', 'AsAsyncEnumerable', 'ToList', or 'ToListAsync'. See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.

Note that the expression can be translated if I remove either the AddMinutes or Date, but of course I can't do that because either of those changes would return incorrect results.
I think that this is a duplicate of PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1715, which was reported last year but is still open.

[roji]

I took a quick look, and it seems to me that the failure is expected.

var utc = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified);
var query = ctx.Items.Where(i => i.Dt < utc.AddMinutes(i.OffsetMinutes).Date);

When EF translates this LINQ query, it does not know the kind of the DateTime you're passing as a parameter here (utc); this is because the same LINQ query can be executed multiple times, with different DateTime instances (and therefore with different Kinds). However, in order to properly translate AddMinutes, the PG provider must know whether the instance it's called on is a timestamp with time zone (UTC DateTime) or timestamp without time zone (non-UTC DateTime); the SQL translations differ in these two cases (see this code to understand this better). Since the type mapping isn't known when translating (because AddMinutes is invoked over the utc parameter), the provider refuses to translate.

Depending on exactly what your query does, you should be able to rewrite it so it avoids this issue. For example, you can simply inline DateTime.UtcNow into the LINQ query, so it gets translated into SQL:

var query = ctx.Items.Where(i => i.Dt < DateTime.UtcNow.AddMinutes(i.OffsetMinutes).Date);

At this point, there's no longer any parameter, and the exact type mapping is known when translating.

[petergummer]

That's a very considered response for a "quick look", Shay. Thanks for the reply. Unfortunately, the real production code can't inline the DateTime variable because it's computed (subtracting days or hours from UtcNow, depending on configuration).
The code that you linked to gave me an idea, however. I have changed the variable to a DateTimeOffset:

var utc = DateTimeOffset.UtcNow;
var query = ctx.Items.Where(i => i.Dt < utc.AddMinutes(i.OffsetMinutes).Date);

Now 6.0.22 and 8.0 can translate the expression. This is the output:

-- @__utc_0='2023-12-18T21:48:51.9804746+00:00'
SELECT i."Id", i."Dt", i."OffsetMinutes"
FROM "Items" AS i
WHERE i."Dt" < date_trunc('day', (@__utc_0 + CAST((i."OffsetMinutes"::double precision::text || ' mins') AS interval)) AT TIME ZONE 'UTC')

This WHERE clause differs a bit from 5.0.10's output. I'm worried that AT TIME ZONE 'UTC' might return incorrect results.
I have done a quick test on the real production code -- simply changing the variable to DateTimeOffset -- and 6.0.22 does generate this WHERE condition. Before upgrading we would have to test this thoroughly.
I've noticed something else interesting. 5.0.10 cannot translate the DateTimeOffset variable. The messy use of DateTimeKind.Unspecified was forced on the existing code by 5.0.10, but perhaps I could clean it all up if this project ever has to be upgraded.

[petergummer]

I've been experimenting with different versions of Npgsql.EntityFrameworkCore.PostgreSQL 6, adding this line of code at the start of the test program that I posted above:

AppContext.SetSwitch("Npgsql.EnableLegacyTimestampBehavior", true);

Without this line of code then of course any version 6 release cannot translate the expression. That seems reasonable.
But with this line of code, I would expect legacy behaviour to be preserved. I've discovered that 6.0.6 can translate the expression. This is the output:

-- @__utc_0='2023-12-19T04:38:46.9061835' (DbType = DateTime)
SELECT i."Id", i."Dt", i."OffsetMinutes"
FROM "Items" AS i
WHERE i."Dt" < date_trunc('day', @__utc_0 + CAST((i."OffsetMinutes"::double precision::text || ' mins') AS interval))

The WHERE clause is a bit simpler than 5.0.10. Some CAST calls that were probably redundant have been optimised away, but it looks like the same logic. This makes me think that I could upgrade safely to 6.0.6!
Version 6.0.7 cannot translate the expression. I think that f1a87fa may be the change that broke the legacy behaviour, in particular this:

- if (NpgsqlTypeMappingSource.LegacyTimestampBehavior || instance?.TypeMapping is NpgsqlTimestampTypeMapping)
+ case NpgsqlTimestampTypeMapping:
+ case NpgsqlTimestampTzTypeMapping when NpgsqlTypeMappingSource.LegacyTimestampBehavior:

In my opinion, if I am requesting legacy behaviour then 6.0.6 is correct. Did this get broken accidentally while support for the NpgsqlDateTypeMapping case was being added?

[roji]

@petergummer the idea to use DateTimeOffset as a way to disambiguate timestamp with time zone from timestamp without time zone is clever - DateTimeOffset can indeed only map to the former (whereas with DateTime it depends on the Kind). However, your code sample doesn't actually make use of the utc variable - maybe you copy-pasted incorrectly?

I've noticed something else interesting. 5.0.10 cannot translate the DateTimeOffset variable. The messy use of DateTimeKind.Unspecified was forced on the existing code by 5.0.10, but perhaps I could clean it all up if this project ever has to be upgraded.

Yep, support for DateTimeOffset was very partial before 6.0; 6.0 included a general, comprehensive rethinking of how date/time types are mapped. It's still perfectly fine to use DateTime with Kind=Utc to represent timestamp with time zone (rather than DateTimeOffset) - there's only a problem when it's used as a variable/parameter as above, in which case the Kind isn't known etc.

BTW if you're considering a bigger change here, I'd strongly recommend giving NodaTime a look. NodaTime offers a much better set of date/time types, doesn't have the ambiguities and problems associated with DateTime.Kind, and is fully supported by Npgsql as well.

[roji]

Re what happens in legacy mode, I think you're right - I've opened #3031 and will fix it.

[petergummer]

... However, your code sample doesn't actually make use of the utc variable - maybe you copy-pasted incorrectly?

Correct, @roji, I copied while experimenting with something else. I've corrected the code snippet now.
Thanks for suggesting NodaTime, but this project won't be rewritten.

[roji]

Thanks for the reply. Unfortunately, the real production code can't inline the DateTime variable because it's computed (subtracting days or hours from UtcNow, depending on configuration).

OK. Note that it may still be possible to inline the calculation - and have it translate to SQL - rather than computing it in .NET and sending it as a parameter. But the workaround with the DateTimeOffset is fine too.

[petergummer]

Yes, I thought of inlining the calculation, but it would risk introducing bugs to a system that is not undergoing further development. Extra joins would risk performance degradation too.
And thanks for opening 3031. It will make it much easier and safer for us to upgrade the project to the latest version at some point in the future.

[roji]

@petergummer OK. Thanks for reporting - I've merged #3031, and I'll go ahead and close this issue as I think everything is clear (but feel free to continue posting if necessary).