search

PomeloFoundation / Pomelo.EntityFrameworkCore.MySql

Entity Framework Core provider for MySQL and MariaDB built on top of MySqlConnector
MIT License
2.72k stars 385 forks source link

Change return type of all `EF.Functions.DateDiff` extension methods related to units of `SECOND` or smaller from `int` to `long` #1875

Open lauxjpn opened 8 months ago

lauxjpn commented 8 months ago

The value returned for TIMESTAMPDIFF function calls of valid dates can exceed Int32.MaxValue, but currently, all our EF.Functions.DateDiff extension methods use int as the return type.

We should change this behavior to return long instead of int. At least for extension methods related to units of SECOND or smaller.

@ajcvickers The SQL Server provider currently uses int as the return type for everything as well, even though it translates those calls to DATEDIFF_BIG. An oversight or a conscious choice?

ajcvickers commented 8 months ago

Probably an oversight. @roji?

roji commented 8 months ago

As far as I can tell, the SQL Server EF.Functions.DateDiff methods translate to DATEDIFF, not DATEDIFF_BIG (code); we seem to translate to DATEDIFF_BIG only when the user specifies DateTimeOffset.ToUnixTimeSeconds/ToUnixTimeMilliseconds. We could introduce EF.Functions.DateDiffBig overloads - see https://github.com/dotnet/efcore/issues/32278 which tracks this.

All this seems to be in line with the general policy that stuff in EF.Functions corresponds directly to SQL functions (as much as possible)... FWIW the Pomelo provider deviates from this in having EF.Functions.DateDiff rather than EF.Functions.TimestampDiff. In any case, SQL Server and MySQL seem to be different on this, with SQL Server having two functions (DATEDIFF returning int and DATEDIFF_BIG returning bigint), and MySQL having just one TIMESTAMPDIFF returning a bigint.

moander commented 8 months ago

I vote for new xBig methods and leave todays int functions as is

lauxjpn commented 8 months ago

@roji You're right, I did not look close enough.

As for Pomelo, unless we want to duplicate the extension methods and then mark all the old ones as obsolete (which we could arguably do), it would be a bit late in the game for us to rename them now to mimic the original MySQL function names.

We will probably leave it as is for now (its probably close enough), unless the community thinks a name change is worth it.

I vote for new xBig methods and leave todays int functions as is

@moander If this comment is directed at the Pomelo implementations: We will change the return type of the methods to long (or at least the ones that can return larger values than Int32.MaxValue), so the returned value does not lead to an overflow. There will be no ...Big methods for us (the MySQL function we call hasn't changed, it's still TIMESTAMPDIFF()).

(Shouldn't be an issue, because the compiler should complain, if the types in existing code don't match anymore.)