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

Support for CONVERT_TZ() function with appropriate translation. #1579

Closed
crozone opened this issue Dec 6, 2021 · 7 comments · Fixed by #1860
Closed

Support for CONVERT_TZ() function with appropriate translation. #1579

crozone opened this issue Dec 6, 2021 · 7 comments · Fixed by #1860

Comments

@crozone
Copy link
Contributor

crozone commented Dec 6, 2021

Preface

MySQL supports a function called CONVERT_TZ(dt,from_tz,to_tz), which allows datetime values to be converted from a given source timezone into a given target timezone. The timezone values can either be specified as a fixed offset (eg +10:00), or more interestingly, an IANA timezone identifier (eg. Australia/Melbourne). This is particularly useful when writing database queries that perform GROUP BY over days/weeks/months, since the GROUP BY query can be run over the appropriately shifted datetime value such that the edges of the day line up correctly with the target timezone. Since an actual timezone identifier can be specified, this also seamlessly accounts for changes in the daylight savings. This is very useful when writing queries for generating reports.

A basic example of such a query:

SELECT DATE(CONVERT_TZ(r.timestamp, @@session.time_zone, "Australia/Melbourne")) AS date, r.id
FROM some_table AS r
GROUP BY date
ORDER BY date;

If r.timestamp is a TIMESTAMP column, it is implicitly converted into the connection timezone before being passed into the CONVERT_TZ function, so @@session.time_zone is used as the from_tz parameter. For converting arbitrary DATETIME values, the from_tz would be set to the actual source timezone.

Issue

Pomelo currently has no translation for the CONVERT_TZ(dt,from_tz,to_tz) method.

Proposed API for consideration

Consider adding additional functions to Microsoft.EntityFrameworkCore.MySqlDbFunctionsExtensions to allow the CONVERT_TZ() function to be expressed:

namespace Microsoft.EntityFrameworkCore
{
    public static class MySqlDbFunctionsExtensions
    {
        // CONVERT_TZ(dateTime,fromTimezone,toTimezone)
        public static DateTime? ConvertDateTimeTimezone([CanBeNullAttribute] this DbFunctions _, DateTime dateTime, string fromTimezone, string toTimezone);

        // ... And an additional function for converting specifically from the connection timezone, for TIMESTAMP columns:

        // CONVERT_TZ(dateTime,@@session.time_zone,toTimezone)
        public static DateTime? ConvertDateTimeFromSessionTimezone([CanBeNullAttribute] this DbFunctions _, DateTime dateTime, string toTimezone);
    }
}

This would allow the above SQL query to be rewritten as the following LINQ query:

var results = await dbContext.SomeTable
    .Select(r => new {
        id = r.Id,
        date = EF.Functions.ConvertDateTimeFromSessionTimezone(r.Timestamp, "Australia/Melbourne").Date
        })
    .GroupBy(d => d.date)
    .OrderBy(d => d.Key)
    .ToListAsync();
@lauxjpn lauxjpn added this to the 7.0.0 milestone Dec 6, 2021
@lauxjpn
Copy link
Collaborator

lauxjpn commented Dec 6, 2021

Does ConvertDateTimeFromSessionTimezone() need its own name, or should we just add another overload to ConvertDateTimeTimezone()?

@crozone
Copy link
Contributor Author

crozone commented Dec 7, 2021

An overload can probably do it, it'll just have to be clearly documented in the method XML docstring.

I can't see an elegant way to specify @@session.time_zone as the parameter otherwise.

@crozone
Copy link
Contributor Author

crozone commented Dec 8, 2021

I'm going to begin some work on this soon and try to get a prototype running, should I branch off master? Ideally I'd like this to be backported into the 6.0.0 release (as a minor or patch release?) since that aligns with the .NET 6 LTS release, if that's feasible.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Dec 10, 2021

I'm going to begin some work on this soon and try to get a prototype running, should I branch off master?

Yes.

Ideally I'd like this to be backported into the 6.0.0 release (as a minor or patch release?) since that aligns with the .NET 6 LTS release, if that's feasible.

This will depend on the impact the PR has on the code base. While we usually do not add features to already released major versions, it is possible as long as there is very litte risk involved. Just adding another translation is potentially one of those features that could make it into the current release instead of the next.

@lauxjpn lauxjpn modified the milestones: 7.0.0, 8.0.0 Jan 16, 2023
@naitikmalaviya
Copy link

Another approach would be mapping TimeZoneInfo.ConvertTime methods to CONVERT_TZ mysql function. I've not gone into details of all ConvertTime methods but if this approach looks good to you guys then I'd be happy to submit a PR in next few days.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Mar 4, 2024

@crozone I went ahead and provided the EF.Functions implementations with #1860. Feel free to review the PR if you got time.


Another approach would be mapping TimeZoneInfo.ConvertTime methods to CONVERT_TZ mysql function. I've not gone into details of all ConvertTime methods but if this approach looks good to you guys then I'd be happy to submit a PR in next few days.

@naitikmalaviya I will currently not implement this translation myself, because it might be cumbersome to ensure that the MySQL time zone information table is in sync with .NET. If you feel strongly about adding those translations as well, please open a separate issue and feel free to provide a PR to discuss the implementation. Thanks!

@crozone
Copy link
Contributor Author

crozone commented Mar 4, 2024

Looks fantastic, thank you!! Also seems like a very clean example of how to add additional translations in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants