Pomelo.EntityFrameworkCore.MySql icon indicating copy to clipboard operation
Pomelo.EntityFrameworkCore.MySql copied to clipboard

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

Open lauxjpn opened this issue 11 months ago • 4 comments

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?

lauxjpn avatar Mar 10 '24 11:03 lauxjpn

Probably an oversight. @roji?

ajcvickers avatar Mar 11 '24 08:03 ajcvickers

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.

roji avatar Mar 11 '24 09:03 roji

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

moander avatar Mar 11 '24 12:03 moander

@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.)

lauxjpn avatar Mar 12 '24 00:03 lauxjpn