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

Add support for AT TIME ZONE #26199

Closed
omfgicbf opened this issue Sep 29, 2021 · 11 comments · Fixed by #26972
Closed

Add support for AT TIME ZONE #26199

omfgicbf opened this issue Sep 29, 2021 · 11 comments · Fixed by #26972
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@omfgicbf
Copy link

I'm trying to use HasDbFunction and HasTranslation to make use of the SQL AT TIME ZONE clause.

I have the following in OnModelCreating:

var methodInfo = typeof(Extensions).GetMethod(nameof(Extensions.ToTimeZone));

static string DelimitIdentifier(string identifier) => $"[{identifier.Replace("]", "]]")}]";

modelBuilder
    .HasDbFunction(methodInfo)
    .HasTranslation(x =>
    {
        var columnExpression = x.First() as ColumnExpression;

        var timeZoneExpression = x.Skip(1).First() as SqlConstantExpression;

        var columnIdentifier = $"{DelimitIdentifier(columnExpression.Table.Alias)}.{DelimitIdentifier(columnExpression.Name)}";

        var expression = new SqlFragmentExpression($"{columnIdentifier} AT TIME ZONE {timeZoneExpression.TypeMapping.GenerateSqlLiteral(timeZoneExpression.Value)}");

        Console.WriteLine($"Translation => {expression.Sql}");

        return expression;
    });

Which is intended to translate calls to the following extension method:

public static DateTimeOffset ToTimeZone(this DateTimeOffset value, string name)
    => throw new NotImplementedException("This should be implemented in HasDbFunction.");

If I try to query using this extension method like this:

var foos = from f in context.Foos
           select new
           {
               Created = f.Created.ToTimeZone("E. Australia Standard Time")
           };

Instead of the ToTimeZone method being translated, the method itself seems to be called and I get the following exception:

Unhandled exception. System.NotImplementedException: This should be implemented in HasDbFunction.
MRE.EFCore5.HasDbFunction.Extensions.ToTimeZone(System.DateTimeOffset, string) in Extensions.cs
Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable<T>.Enumerator.MoveNext() in SingleQueryingEnumerable.cs
MRE.EFCore5.HasDbFunction.Program.Main(string[]) in Program.cs

Even though the call to ToTimeZone isn't translated, the translation itself is being called (twice) and is generating the correct SQL fragment, however, the fragment is not included in the query and the exception from the extension method is thrown.

There is a minimal reproducible example here.

Technical Details

EF Core version: 5.0.10
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 5.0
Operating system: Windows 10
IDE: Visual Studio 2019 16.11.3

@ajcvickers
Copy link
Member

/cc @maumar

@maumar
Copy link
Contributor

maumar commented Sep 30, 2021

Problem here is with using SqlFragmentExpression - it's supposed to represent a SQL token, not an arbitrary sql statement. As such, it doesn't have a way to specify type or type mapping. We retrieve the translation from metadata, but don't apply default type mapping either (if we could, we would have applied string mapping which is default for sql fragment and that is wrong in this case anyway), so the expression is marked as not-translatable and falls back to client eval.

Note to triage: As a solution, we could extend the usage of SqlFragmentExpression, or create new expression that could represent such construct (similar to https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/master/src/EFCore.MySql/Query/Expressions/Internal/MySqlComplexFunctionArgumentExpression.cs). I actually thought we already have something like this.

@roji
Copy link
Member

roji commented Sep 30, 2021

We may simply want to add an AtTimeZone SqlExpression, and it may even make sense in relational (IIRC it's part of the SQL standard, and at least PG supports it too). This could be used in translating DateTime.ToUniversalTime and similar.

@omfgicbf
Copy link
Author

omfgicbf commented Oct 1, 2021

@maumar, thanks, that explains why it wasn't working.

Is there any documentation on which expressions are supported in HasTranslation?

All I could find was here and here, however, neither seems to indicate that SqlFragmentExpression is not supported and the silent fall-back can be a bit confusing.

Am I right in thinking that solving this using my own custom expression (subclassing SqlExpression) used to be supported but is no longer possible because of SqlNullabilityProcessor?

I came up with two workarounds; one involves wrapping AT TIME ZONE in a user-defined function and calling it with SqlFunctionExpression, the other is wrapping the call in a "no-op" like IIF(1 = 1, ...:

static string DelimitIdentifier(string identifier) => $"[{identifier.Replace("]", "]]")}]";

modelBuilder
    .HasDbFunction(typeof(Extensions).GetMethod(nameof(Extensions.ToTimeZone)))
    .HasTranslation(x =>
    {
        var columnExpression = x.First() as ColumnExpression;

        var timeZoneExpression = x.Skip(1).First() as SqlConstantExpression;

        var columnIdentifier = $"{DelimitIdentifier(columnExpression.Table.Alias)}.{DelimitIdentifier(columnExpression.Name)}";

        var valueExpression = new SqlFragmentExpression($"{columnIdentifier} AT TIME ZONE {timeZoneExpression.TypeMapping.GenerateSqlLiteral(timeZoneExpression.Value)}");

        var testExpression = new SqlFragmentExpression("1 = 1");

        var expression = new SqlFunctionExpression(
            "IIF",
            new List<SqlExpression>() { testExpression, valueExpression, valueExpression },
            false,
            new List<bool>() { false, false, false },
            typeof(DateTimeOffset),
            null
            );

        return expression;
    }

Both seem to work, but I'm unsure what the impact on performance (if any) will be. I'd appreciate any suggestions on a different way to approach this.

@roji, I agree having an AtTimeZone method would be good, perhaps as a part of EF.Functions, however, there may still be other clauses that fall in the same boat (OUTPUT, READTEXT, etc, though I concede these are poor examples).

@roji roji changed the title HasDbFunction and HasTranslation not working as expected Add support for AT TIME ZONE Oct 1, 2021
@roji roji added the area-query label Oct 1, 2021
@roji
Copy link
Member

roji commented Oct 1, 2021

Is there any documentation on which expressions are supported in HasTranslation?

Not really - HasDbFunction is mostly used to map CLR functions to a simple SQL function call, it's rather rare for users to have to map to other SQL expression types. The relational (database-independent) SQL expressions can be found here, while the SQL Server-specific ones go here (though we currently have very few).

All I could find was here and here, however, neither seems to indicate that SqlFragmentExpression is not supported and the silent fall-back can be a bit confusing.

Note that SqlFragmentExpression is supported, just not for this kind of usage (which requires a type mapping). We can add docs for this, though this seems like a very rare edge case.

Am I right in thinking that solving this using my own custom expression (subclassing SqlExpression) used to be supported but is no longer possible because of SqlNullabilityProcessor?

No - you are free to replace the SqlNullabilityProcessor and add support for your custom expressions. However, creating your own SQL expression type is complex and goes beyond just implementing support in SqlNullabilityProcessor; it's simply not something that users are supposed to do, only providers (it has always been this way). See the recent discussion in #26147 for more on this (especially this comment by @smitpatel).

Your workarounds may very well work OK - I'd need to see actual SQL to comment on the performance. But the right way to fix this is for us to add AT TIME ZONE support in EF Core, either in the SQL Server provider or in relational.

there may still be other clauses that fall in the same boat (OUTPUT, READTEXT, etc, though I concede these are poor examples).

I don't think these are very similar... Both simply modify the statement they're in and wouldn't require a type mapping (they're more similar to query hints in this sense).

@smitpatel
Copy link
Member

I don't think "AT TIME ZONE" belongs to relational layer.

@roji
Copy link
Member

roji commented Oct 1, 2021

@smitpatel AT TIME ZONE exists in SQL Server, PostgreSQL and Oracle, and might also exist in the same way in MySQL.

The point here was less whether this belongs in relational or not, and more to introduce it (could be a provider-specific expression). We should also consider an EF.Functions.AtTimeZone, again either in relational or provider-specific.

@omfgicbf
Copy link
Author

omfgicbf commented Oct 3, 2021

@roji, thanks for the feedback.

Not really - HasDbFunction is mostly used to map CLR functions to a simple SQL function call

Right, it's just that the document here refers to "Mapping a method to a custom SQL" and uses the following example:

100 * ABS(first - second) / ((first + second) / 2)

Which isn't so simple, and implies support of more complex SQL where specific use-cases haven't been covered by other expressions.

I accept that the example given can be expressed as an expression tree, however, given my use-case doesn't seem possible to do with the provided SQL expression types, a first guess might be that SqlFragementExpression is the way to go.

In this case, the use of "fragment" rather than "token" causes confusion around its intent, in the absence of documentation suggesting otherwise.

Note that SqlFragmentExpression is supported

Right, but I can't return one from HasTranslation, even though it inherits from SqlExpression, as all it does is silently fail. Whereas I can construct an SqlFunctionExpression, SqlBinaryExpression, etc, and they will work, except none of them supports this use-case.

No - you are free to replace the SqlNullabilityProcessor and add support for your custom expressions

The solutions I found online suggested that prior to SqlNullabilityProcessor I could subclass SqlExpression and resolve this, without having to replace SqlNullabilityProcessor, which is, as you point out, not recommended.

If replacing SqlNullabilityProcessor is recommended only for providers, what is the recommended (end-user) way to support arbitrary SQL when none of the provided expressions supports it?

I'd need to see actual SQL to comment on the performance

Using the IIF workaround, I end up with:

IIF(1 = 1, [f].[Created] AT TIME ZONE N'E. Australia Standard Time', [f].[Created] AT TIME ZONE N'E. Australia Standard Time') AS [Created]

I don't think these are very similar

I agree that they were bad examples; it seems (for now) that AT TIME ZONE is the only clause that might appear in the select list. However, was there a way to do this with HasTranslation, we needn't be dependant on the introduction of AtTimeZone, or for whatever other use-cases that might turn up in the future.

@roji
Copy link
Member

roji commented Oct 3, 2021

Which isn't so simple, and implies support of more complex SQL where specific use-cases haven't been covered by other expressions.

That's true, but complex SQL is very different from implementing custom expressions. Users can translate to arbitrary complex SQL constructs, as long as their basic building blocks (the expression types) are supported by the provider.

Right, but I can't return one from HasTranslation, even though it inherits from SqlExpression, as all it does is silently fail. Whereas I can construct an SqlFunctionExpression, SqlBinaryExpression, etc, and they will work, except none of them supports this use-case.

SqlFragmentExpression simply isn't like these other expressions, and wasn't meant for doing this kind of thing. I'm not sure "token" describes it better than "fragment", but that's a naming decision that is not longer very relevant.

If replacing SqlNullabilityProcessor is recommended only for providers, what is the recommended (end-user) way to support arbitrary SQL when none of the provided expressions supports it?

Some notes to put this in context:

  • This is an extremely rare scenario - AFAIK this issue is the first time it's been flagged in EF Core.
  • As an alternative approach/workaround, users can use raw SQL queries and compose on top of them, rather than translating a function call to unsupported SQL.
  • When it really is desired, users still can replace SqlNullabilityProcessor (and the other relevant visitors). Although this is supported via public APIs, this is considered very advanced usage and we don't recommend it for the average user.

@smitpatel can confirm, but the way I see it, there's no recommended way for end users to do function translation to SQL with unsupported expression types. Either use raw SQL queries or wait for the expression to be implemented on the EF Core side (which is what I think this issue should be about).

@ajcvickers ajcvickers added this to the Backlog milestone Oct 6, 2021
@roji roji changed the title Add support for AT TIME ZONE Add support for AT TIME ZONE and basic translations Dec 12, 2021
@roji roji self-assigned this Dec 12, 2021
@roji roji modified the milestones: Backlog, 7.0.0 Dec 12, 2021
@roji roji changed the title Add support for AT TIME ZONE and basic translations Add support for AT TIME ZONE Dec 12, 2021
@roji
Copy link
Member

roji commented Dec 12, 2021

Info on AT TIME ZONE across databases:

SQL Server

Documentation

-- Convert datetime2 to datetimeoffset (no conversion, just specify time zone):
SELECT CAST('2020-01-01' AS datetime2) AT TIME ZONE 'Central European Standard Time'; -- 2020-01-01 00:00:00.0000000 +01:00

-- Convert datetimeoffset from one time zone to another:
SELECT CAST('2020-01-01 00:00:00+02:00' AS datetimeoffset) AT TIME ZONE 'Central European Standard Time'; -- 2019-12-31 23:00:00.0000000 +01:00

-- Nullability propagation:
SELECT CAST('2020-01-01 00:00:00+02:00' AS datetimeoffset) AT TIME ZONE NULL -- null
SELECT NULL AT TIME ZONE N'UTC' -- null

PostgreSQL

Documentation.

-- Convert UTC timestamp to local Berlin timestamp (timestamptz -> timestamptz)
SELECT CAST('2020-01-01 00:00:00Z' AS timestamptz) AT TIME ZONE 'Europe/Berlin'; -- 2020-01-01 01:00:00.000000

-- Convert local Berlin timestamp to UTC (timestamp -> timestamptz)
SELECT CAST('2020-01-01 00:00:00' AS timestamp) AT TIME ZONE 'Europe/Berlin'; -- 2019-12-31 23:00:00.000000 +00:00

-- Nullability propagation:
SELECT '2020-01-01'::timestamp AT TIME ZONE NULL -- null
SELECT NULL AT TIME ZONE 'UTC' - null

Oracle

Documentation

Firebird

Documentation

SQLite

Not supported

MySQL/MariaDB

Not supported

roji added a commit to roji/efcore that referenced this issue Dec 12, 2021
roji added a commit to roji/efcore that referenced this issue Dec 12, 2021
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Dec 12, 2021
roji added a commit to roji/efcore that referenced this issue Dec 14, 2021
roji added a commit to roji/efcore that referenced this issue Jan 12, 2022
roji added a commit to roji/efcore that referenced this issue Jan 19, 2022
@sommmen
Copy link

sommmen commented Apr 14, 2022

Hiya just wanted to share some usage info quickly,

I often have to make reports grouped by day, week, month or year - quite commin in all kinds of BI applications.
I'm using keyless entities for now that look somewhat like this:

SELECT
    DATEADD(DAY, DATEDIFF(DAY, 0, [Orders].[OrderDateTime] AT TIME ZONE 'W. Europe Standard Time'), 0) as [Day],
    SUM([TotalInclVat]) as [TotalInclVat],
    -- etc...
FROM [Orders]
GROUP BY DATEADD(DAY, DATEDIFF(DAY, 0, [Orders].[OrderDateTime] AT TIME ZONE 'W. Europe Standard Time'), 0)

The query above groups orders daily in the right timezone, rather than the group by grouping daily based on UTC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants