Add more DateTimeOffset member translations

[stevendarby]

Currently some DateTimeOffset members shared with DateTime are translated, e.g. https://github.com/dotnet/efcore/blob/c8c6c21cafb9d25b7d8f3be316fd806cb2132de5/src/EFCore.SqlServer/Query/Internal/SqlServerDateTimeMemberTranslator.cs

Could add some more translations for instance members specific to DateTimeOffset. Some thoughts/suggestions for SQL Server:

• DateTimeOffset.DateTime -> CONVERT(datetime2, @dateTimeOffset)
  -- Lop off the offset

• DateTimeOffset.UtcDateTime -> CONVERT(datetime2, SWITCHOFFSET(@dateTimeOffset, '+00:00'))
  -- Adjust to 0 offset then lop off the offset
  -- @dateTimeOffset AT TIME ZONE 'UTC' is an alternative for the adjustment part, but is only supported in 2016+

• DateTimeOffset.LocalDateTime
  -- Don't think this can be translated without AT TIME ZONE and a simple way to get the current timezone ID from SQL Server.

[roji]

Looks good - I personally think that using AT TIME ZONE and relying on SQL Server 2016+ is fine at this point (we'll discuss).

DateTimeOffset.LocalDateTime
-- Don't think this can be translated without AT TIME ZONE and a simple way to get the current timezone ID from SQL Server.

There's CURRENT_TIMEZONE which should be good for that.

Note that this translates the concept of the local .NET machine time zone to the SQL Server time zone - which I think is right and consistent with other translations we have (I've done the same for PostgreSQL). But it's probably a good idea to make this explicit in the translation docs.

[stevendarby]

@roji I did come across that but unfortunately it (along with CURRENT_TIMEZONE_ID, which I think is the correct one to use with AT TIME ZONE) only seem to be supported in Azure SQL. For SQL Server there is a way to get it by reading registry keys but it seems a little messy…

[roji]

@stevendarby odd, I just tested this on my local SQL Server (2019) and CURRENT_TIMEZONE() works just fine. It also returns my actual non-UTC local time zone, despite the comment saying that "For SQL Database, the time zone is always set to UTC and CURRENT_TIMEZONE returns the name of the UTC time zone.".

I suspect the docs here are incorrect, and the intention is to say that the function is supported on regular SQL Server too, but always returns UTC on SQL Azure?

[stevendarby]

Ah, I have 2017 installed locally and it doesn't work there. Just found a source saying it's a 2019+ feature. It also has the workaround for older versions.

[roji]

Great, makes sense (I wrote some feedback on the SQL Server docs that they should fix it).

I think it's OK to translate for SQL Server 2019 here, especially since there's no real alternative (EF Core translations can't rely on the registry->variable trick). Users who really need this should be able to define a user function which does this trick though.

[stevendarby]

Just had a quick play with SQL Server 2019, and oddly, while CURRENT_TIMEZONE is supported as you noted, CURRENT_TIMEZONE_ID doesn't appear to be - and is Azure only? Unfortunately it's the latter which is required for AT TIME ZONE as that's the proper ID and not just the description.

[roji]

Yeah, seems like you're right @stevendarby. DateTimeOffset.LocalDateTime may not be translatable.

I have my doubts about the usefulness of a time zone conversion that uses SQL Server's configured local time zone though. I'm not sure many users will miss this.

[ajcvickers]

Note from triage: yes for the first two.

[roji]

@stevendarby if you're interesting in working on this, we'd be happy to have a PR!

[roji]

@stevendarby note #26972 which adds support for AT TIME ZONE (though not sure this is needed for this issue per se).

[stevendarby]

I personally think that using AT TIME ZONE and relying on SQL Server 2016+ is fine at this point (we'll discuss).

Hi @roji, just to check, is 2016+ ok then? I know there is now support for AT TIME ZONE but I think that's only via an explicit EF.Functions rather than a translation of a .NET method, and wondered if there were different expectations for compatibility based on this. I previously went by this comment (#2981 (comment)) but I see STRING_AGG is being used there after all so appreciate any clarity on this. Thanks!

[roji]

I think we should be OK with translating a .NET method in a way which doesn't work on old database versions. Especially when when that .NET method isn't currently translated at all (so nobody is being broken), and there doesn't seem to be any satisfactory translation which doesn't rely on a newer database version.

So I don't think there's any particular blocker for adding new DateTimeOffset translations which rely on 2016+, and which we don't currently translate.