Db side methods

[fredericDelaporte]

This adds to the LINQ provider the ability to translate DateTime.Now and its variants (Utc, Today, Offset) to SQL calls instead of being pre-evaluated before running the query.
It does the same for Guid.NewGuid, Random.Next and Random.NextDouble.

For enabling these capabilities, matching HQL functions are required. This PR also adds them to all dialects whenever possible.

For being as few breaking as possible, when the dialect lacks support of required functions, the pre-evaluation takes place instead.
This looks quite suitable for DateTime.Now and its variants.

But for Guid and Random, I find it more debatable: when pre-evaluated, the same value will be used for each row of result, whereas when converted to SQL, different values for each row will be used. So this could cause bugs when an application targets a different database than previously tested, without an immediate obvious exception.
Maybe this will be a bad legacy, and maybe it would be better to throw a NotSupportedException in those cases, instead of falling back to pre-evaluation.

For queries where this change is breaking, it can be fixed either on a case by case basis or globally.
On a case by case basis, moving the expression newly converted to SQL out of the LINQ query thanks to a local variable will cause it to be evaluated once per request like it was prior to this PR.
Globally, a new boolean setting linqtohql.legacy_preevaluation allows to disable this new feature.

Fixes #959.

[fredericDelaporte]

I had to add a new random HQL function, although most dialects already have the rand one. Unfortunately, rand with SQL-Server and SAP ASE yields the same value for each row, which is useless for our case (better keep it evaluated in runtime in such case).
Changing them for those db could be breaking for users using them while expecting them to yield the same value for each row.
So I have added a random function, in most cases copy of the rand one, but adjusted for SQL-Server and ASE for generating new values for each row.

(By the way, yes I have tested DB2, with some non-committed tweaking of other files, notably for supporting Guid in a quick&dirty way. The LINQ test requires Guid support, which is lacking for DB2 currently.)

[fredericDelaporte]

The 2000 registration uses the dateadd(dd, 0, datediff(dd, 0, getdate())) hack too. As I have overridden current_date with the simplier cast to date here, I have considered the date function should be overridden too for consistency.

[fredericDelaporte]

No random support for SQL-Server CE: rand has the same limitation than SQL-Server one, and it cannot use the SQL-Server hack because it lacks the checksum function.

[fredericDelaporte]

PostgreSQL already has a random function registered, and it works as required.

[fredericDelaporte]

ASE supports uuid, but the adequate registration was lacking. But I have no local installation of ASE (trial is time limited, so I have not bothered with it) to check if it supports DbType.Guid in DbParameter. (Main pain point with Guid and DB2, requiring hacking the driver for tweaking the parameter settings and their values.)

So, although it can generates uuid on db-side, maybe this change should be reverted, since it is not unlikely the change is maybe not enough for actually supporting Guid.

[fredericDelaporte]

Checked by cherry-picking the dialect and driver from the Anywhere pending PR, it works as required for this PR.

[fredericDelaporte]

Some databases may have suitable functions to call for avoiding this kind of constructs, like Oracle, but this seems to infrequent for being worth coding special cases for them.

[hazzik]

Maybe better to leave it for 6.0 then?

[fredericDelaporte]

Or add it as an experimental feature, disabled by default till 6.0. (linqtohql.legacy_preevaluation would default to true at first, and be changed for defaulting to false only in 6.0.)

By the way, I have still not really made my mind about the fallback to pre-evaluation when the feature is enabled but the dialect does not support the required translation. Not fall-backing will cause a failure when using the feature, which should be easier and less surprising to diagnose than the consequences of a fallback to pre-evaluation. But that also reduces the portability of an application to other databases, since it is also likely that the fallback could cause less disturbance to the application than a straight failure.

[fredericDelaporte]

I have added some commits:

The first switch linqtohql.legacy_preevaluation to true by default, with a note in documentation stating it will likely be false in the next major version.

Following commits excepted the last are for an additional option for failing or falling back in case of lack of dialect support: linqtohql.fallback_on_preevaluation, false by default. Documented as having no effect with legacy pre-evaluation. And in case of failure, the exception message hint at this option.

The last one is just a minor adjustments in the documentation of the new pre-evaluation.

[fredericDelaporte]

Rebased, more wording fixed, slightly squashed, commits regrouped for resolving conflicts the fixups were having. Stay as fixup the commit for not enabling this feature by default and the commits for failing or fall-backing.

[hazzik]

Maybe we just need to add the ability for user register the system methods as not-preevaluateable, but not register any functions (maybe even provide the generators and information how to register them).

At this moment following will work and translated to sql call despite being independent of any data:

class SqlFunctions
{
        [LinqExtensionMethodAttribute("current_timestamp", LinqExtensionPreEvaluation.NoEvaluation)]
        public static DateTime Now() => DateTime.Now;
 }

However, there is no way to register just DateTime.Now.

Also, I feel we need to privde a way to disable some of the functions instead of all of them.

[fredericDelaporte]

Maybe we just need to add the ability for user register the system methods as not-preevaluateable, but not register any functions (maybe even provide the generators and information how to register them).

It seems overly complex to me, when all the user has to do for those functions he does not want to get translated to SQL, is to call them outside of the query.

There is only the breaking change aspect of the feature we need to handle. And this PR does it by supplying configuration option, defaulting to legacy behavior for 5.x.

[maca88]

In my opinion the default behavior should be the following:

DateTime/DateTimeOffset properties:

• Should be pre-evaluated when located in the expression and not supported by the database

from o in db.Orders where o.OrderDate <= DateTime.Today // DateTime.Today should be pre-evaluated

• Should not be pre-evaluated when located in the expression and supported by the database

from o in db.Orders where o.OrderDate <= DateTime.Today // DateTime.Today should be converted to SQL

• Should be pre-evaluated when there are additional method calls/members afterwards that are not supported by the database

from o in db.Orders where o.OrderDate <= DateTime.Today.AddDays(1)

• Should not be pre-evaluated when there are additional method calls/members afterwards that are supported by the database

from o in db.Orders where o.OrderDate <= DateTime.Today.Day

• Should be pre-evaluated when located outside of the expression

var date = DateTime.Today;
from o in db.Orders where o.OrderDate <= date

Guid and Random:

• Should not be pre-evaluated when they are located in the expression and supported by the database

from o in db.Orders where o.Guid != new Guid()

• Should be pre-evaluated when located outside of the expression and they have no additional method calls/members afterwards

var guid = new Guid();
from o in db.Orders where o.Guid != guid 

• Should throw when they are located in a non select expression or sub-query and not supported by the database

from o in db.Orders where o.Guid != new Guid()

• Should be evaluated on the client side when they are located in a select expression that is not part of a sub-query and not supported by the database

select new Guid() from o in db.Orders

• Should throw when they are located outside/inside of a non select expression or sub-query and have additional method calls/members afterwards that are not supported by the database

var random = new Random();
from o in db.Orders where o.OrderId != random.NextDouble().GetHashCode()
from o in db.Orders where o.Guid != new Guid().GetHashCode() 

• Should be evaluated on the client side when located outside/inside of a select expression that is not part of a sub-query and have additional method calls/members afterwards that are not supported by the database

var random = new Random();
select random.NextDouble().GetHashCode() from o in db.Orders
select new Guid().GetHashCode()  from o in db.Orders

So LinqToHqlLegacyPreEvaluation should by default be set to false and LinqToHqlFallbackOnPreEvaluation to true.

Instead of adding IAllowPreEvaluationHqlGenerator.AllowPreEvaluation, it would be more useful to have a method like IEnumerable<string> GetSqlFunctions(MemberInfo member) for IHqlGeneratorForMethod and IHqlGeneratorForProperty interfaces, which would return all sql function names that are used in the hql. Such method could be also used in other PRs like #2079 in order to detect whether a client side operation should be done instead. All AllowPreEvaluation methods in this PR have similar logic that could be written in one place by having such method.

I will give it a try to see what has to be done in order to achieve such behavior.

[fredericDelaporte]

For a minor release (5.x), if we can avoid possible breaking change, we should.
This PR current state avoids possible breaking changes with its default configuration values.
I think we should set LinqToHqlLegacyPreEvaluation to false only in a next major version (6.0 if this gets merged in 5.x).

I am not convinced we should handle fallbacks for DateTime cases differently than for guid/random. I would rather give one simple option, fallback for all or fail for all. DateTime fallback can be as wrong as guid/random fallback: when the application host is not in the same time zone than the database, the result will be just plain wrong.

Should be pre-evaluated when there are additional method calls/members afterwards that are not supported by the database

And if that member ends up supported in some later version, end of pre-evaluation? That could be a serious breaking change, if the database is not in the same timezone, again.
I do not think it is a good idea, because it will likely bring more complexity into the logic for determining whether we should pre-evaluate or not, and it will also cause the end result to be harder to predict for the user.
I think we should keep it simple. And just fail if the Now or Today is translated but followed by something unsupported: the user will obviously see that when testing, and the fix, if pre-evaluation is fine for him, is easy: just compute it out of the query. (Or switch back to legacy behavior.)

[maca88]

I would rather give one simple option, fallback for all or fail for all. DateTime fallback can be as wrong as guid/random fallback

On second thought, I do agree that we should not provide a fallback for DateTime/DateTimeOffset properties, so it should throw no matter where they are located in the expression.

And if that member ends up supported in some later version, end of pre-evaluation?

For the pre-evaluation I now agree with how it is handled by this PR. Also we need to consider that pre-evaluation is a hot path, as it is executed every time the query is executed, so we should keep it simple.

I think we should keep it simple. And just fail if the Now or Today is translated but followed by something unsupported

I agree that it should fail when Now/Today/UtcNow are used in a non select statement or a select statement of a sub-query, but I think for the following examples:

db.Orders.Select(o => DateTime.Now.DayOfYear).ToList(); // Does not work
db.Orders.Select(o => DateTime.Now.AddDays(1)).ToList(); // Works

we should support client side evaluation when only DateTime.Now is supported by the database.

And for Guid and Random I think that we should allow client side evaluation when located in a non subquery select statement:

var random = new Random();
db.Orders.Select(o => random.NextDouble().GetHashCode()).ToList(); // Works when random is supported, throws otherwise
db.Orders.Select(o => Guid.NewGuid().GetHashCode()).ToList(); // Works when guid is supported, throws otherwise

even when random and guid are not supported by the database. Whether values are generated on the client or on the database doesn't matter, what matters is that the values are random/unique. What are your thoughts on it?
Even if you agree with it, the work should be done/tested in #2079 as client side evalaution is calculated by SelectClauseHqlNominator which is heavily modified in the mentioned PR.

[fredericDelaporte]

we should support client side evaluation when only DateTime.Now is supported by the database.

Assuming you mean the Now is still evaluated on db side, but then its value would be returned in the result set and the additional method/property call evaluated in runtime, it may be a nice to have, provided this does not add much more complexity to the Linq provider. Same for the guid/random case having for this case a fallback to runtime. It is acceptable since it preserves their feature, as in this case they would be run for each row of the result set, not just once for the whole query.

I just write "may" because this does introduce some discrepancy between when those expressions are used in a "final" Select and when they are used elsewhere in a Linq expression. As an user, when I was not knowing that NHibernate may evaluate in runtime some projection cases, I had a hard time understanding why some expression seemed supported by the Linq provider in the Select but not elsewhere. It was looking to me as a NHibernate bug. The answer was they were not supported at all by the Linq provider, but there were not even "seen" by the provider in those cases, and instead evaluated by the runtime after query execution. When you do not know this, it is a bit confusing.

So overall why not, but I think it would be better to handle it in a dedicated PR. (Maybe not in #2079 either, but in a PR depending on #2079.)

[maca88]

As an user, when I was not knowing that NHibernate may evaluate in runtime some projection cases, I had a hard time understanding why some expression seemed supported by the Linq provider in the Select but not elsewhere.

I had the same issue, but once understood it is very useful to have such feature. I saw many times developers using ToList before Select method because they thought that it wouldn't work or it actually didn't worked. Having a smart client side evaluation reduces the amount of columns that needs to be selected and does not require to have a ToList prior Select method call, which avoids iterating the collection twice. Unfortunately, there is also a downside of such feature, when the developer does not understand how NHiberante linq provider works and writes the following query:

db.Orders.Select(o => GetValue(o)).ToList();

private string GetValue(Order order)
{
  return order.Employee?.Superior?.FirstName;
}

the query will work but it will trigger additional queries, which can cause performance issues. This could be fixed by adding a configuration option whether to allow additional queries to be executed while calling ToList and could be overridden for specific queries, but it is a matter of another issue/PR.

Back on the topic, with the last commit two issues were addressed:

• Add NullableExpressionDetector support for static properties, by flagging them as not nullable.
• Fix DateTime.Kind for functions that are supported by this PR. For other functions like current_time, I think a separate PR should be made whether we want to change them.

[maca88]

Removed new_uuid support from Sqlite, which I think can be added in a separate PR (optionally with corrected SelectGUIDString property) once System.Data.SQLite.Core will ship with version 3.31 or higher. Feel free to revert the commit whether you don't agree.

[fredericDelaporte]

Review dismissed due to having to resolve conflicting changes in Oracle 10g dialect.

[fredericDelaporte]

Furthermore, rebased, since I consider merging rather than squashing, so I have squashed @maca88 fix commits into the three main commits, adding co-author.