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

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 this issue Mar 10, 2024 · 4 comments
Labels
Milestone

Comments

@lauxjpn
Copy link
Collaborator

lauxjpn commented Mar 10, 2024

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
Copy link

Probably an oversight. @roji?

@roji
Copy link

roji commented Mar 11, 2024

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 dotnet/efcore#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
Copy link

moander commented Mar 11, 2024

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

@lauxjpn
Copy link
Collaborator Author

lauxjpn commented Mar 12, 2024

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

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

No branches or pull requests

4 participants