dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.49k stars 3.13k forks source link

Consider translating `System.Math.Round(double, MidpointRounding)` #15427

Open dmitry-lipetsk opened 5 years ago

dmitry-lipetsk commented 5 years ago

Hello.

System.Math.Round(0.5) returns 0

MSSQL ROUND(0.5) returns 1

You know about these difference in implementation of ROUND function?

PS. Firebird ROUND(0.5) returns 1 also

AndriySvyryd commented 5 years ago

Math.Round() rounds towards even by default.

To get the expected behavior Math.Round(0.5, MidpointRounding.AwayFromZero) should be called

dmitry-lipetsk commented 5 years ago

Hello @AndriySvyryd

The problem in a different results of following expressions (with 0.5 value):

  1. var recs=db.testTable.Where(r => System.Math.Round(r.DoubleFields)==1);

It is server calculation.

  1. var recs=db.testTable.Where(r => System.Math.Round(DoubleLocalVariable)==1);

It is local calculation.


In (1) condition is TRUE, because SQL ROUND(0.5) returns 1.

In (2) condition is FALSE, because Math.Round(0.5) returns 0.

For my point of view - this is real problem.

In second case (2) need translate System.Math.Round(DoubleLocalVariable) to System.Math.Round(DoubleLocalVariable, MidpointRounding.AwayFromZero) or no ?

AndriySvyryd commented 5 years ago

@dmitry-lipetsk Yes, call System.Math.Round(DoubleLocalVariable, MidpointRounding.AwayFromZero) for client evaluation. In 3.0 we'll make it more obvious when something will be evaluated on the client.

dmitry-lipetsk commented 5 years ago

Ok.

I understood.

Some time ago my client ask me - "when my provider for EFCore will be completed?". I answered - "the primary probelm in EFCore itself, it needs to be improved" :)

Math.Round has the another problem - it does not allow digits<0. SQL ROUND - allows.

ajcvickers commented 5 years ago

@dmitry-lipetsk In case it might help, from observation of your posts it seems to me that your desire/expectation is that any query run on the store will have exactly the same behavior as that query run in-memory with LINQ to Objects. This is not a goal of EF Core. Instead we have a goal of creating reasonable, simple, translations that preserve the intent of the query, but may behave differently depending on how that actual database engine executes that query.

We usually talk about this in a different way. Specifically, that EF does not try to abstract all operations and ensure that every database behaves the same. It instead tries to provide common pattern and abstractions that work across databases while still maintaining and exposing the native capabilities of that database.

dmitry-lipetsk commented 5 years ago

Hello @ajcvickers,

I tried to follow EFCore principles, but very quickly realized that they would lead me into a dead end.

Therefore, I do so that local and server calculations give identical results.

roji commented 5 years ago

@dmitry-lipetsk, as @ajcvickers wrote above, identical results for client and server operations is an explicit non-goal of EF Core - there are some good reasons for that. Here are some examples.

Now, for these examples, it may be possible to produce complex SQL constructs to exactly match the C# behavior (e.g. count the number of trailing blanks and add that to the result of LEN()). However, that would be significantly complex SQL, and more importantly, is likely to be very inefficient (because of index usage).

At the end of the day, databases are a different execution environment with behaviors that sometimes differ considerably from .NET/C#. In addition, the different databases differ between themselves, and it is both impractical and inefficient for EF Core to entirely abstract all that away. EF Core's philosophy is to try to produce the C# behavior wherever that's natural, but to leave the normal database behavior as-is otherwise.

ajcvickers commented 5 years ago

From triage: we will not be changing the default translation. However, leaving this on the backlog to at some point consider translating the System.Math.Round(double, MidpointRounding) overload, for which it would not be unreasonable to have a fairly complex translation.