Avoid unnecessary casting for Linq provider

[maca88]

This PR contains an improvement for parameter detection that are related to nullable expressions, which was needed to avoid unnecessary casting reported in #2490.

Fixes #2490

[maca88]

Prior this PR there would be 3 parameters.

[gokhanabatay]

unnecessary cast is exists short/long with == / <= / >= operators could you add additional test cases.

[maca88]

When comparing two different types short == long, a cast will always be added when the mapped sql type of short and long are different. I am thinking of restoring the previous behavior of Equals in case a user wants to compare two different types without an explicit cast as this is not possible anymore in 5.3.

[gokhanabatay]

I think the operators and methods should work exactly the same, otherwise the software developer might make an error about which one to use and under what condition. If not careful, sql clauses are created which can cause performance problems.

The strange thing is that it casts unnecessarily in short to short or long to long comparisons.

Equals method with == operator
CompareTo method result == 0 with == operator
CompareTo method result < 0 with <= operator
CompareTo method result > 0 with >= operator

[fredericDelaporte]

Does this case still qualifies as a regression? It is not really clear when reading #2490.

[maca88]

The problem is that with #1996, Equals method was also modified to work the same as == operator, where previously it was directly converted without any additional logic. The new logic does not behave the same as it may add a cast where previously by using Equals was not added. This PR improves the new logic to not add a cast when both sides are casted by the compiler due to lack of an operator (short == short), but still does not match the old Equals behavior, for example:

o.Short.Equals(3)

In 5.2.x a cast is not added, where in 5.3 it is added due to a type mismatch (short and int). I think it would be better to restore the old Equals behavior in case the user does not want the new logic to add the is null check and possible a cast.

Does this case still qualifies as a regression?

Technically it is a regression but this PR does not solves it entirely, that is why I am thinking to restore the old Equals behavior, so then this PR may be added in the next version as an improvement or to the next minor as a bug fix.

[fredericDelaporte]

I think we have a flaw in the casting logic: it should cast the parameter whenever possible, instead of the column. Casting a parameter has far less potential for causing performances issues than casting a column. And a parameter cast could be even pre-evaluated.
Another possibility may be to handle implicit cast detection, where it is known the SQL engine does not need the cast because it does it implicitly between the involved types. But that is very likely to be database dependent and as such quite more heavy to implement.

[gokhanabatay]

@fredericDelaporte @maca88
Take a look below test case with sql statements, as you can see there is no short parameter.
Two short column compared with casting. cast(addresses1_.ADDRESS_TYPE as int4)=cast(addresses1_.ADDRESS_TYPE2 as int4)

https://github.com/gokhanabatay/NHibernateBugTest/blob/master/NHibernateBugTest/CustomTypeSelectFailsTest.cs
AnyExistsWithNullableShort

var query = session.Query<Customer>().Where(x => x.Addresses.Any(y => y.AddressType.Value == y.AddressType2.Value));

select customer0_.GUID as guid1_1_, customer0_.NAME as name2_1_ from CUSTOMER customer0_ 
where exists (select addresses1_.GUID from ADDRESS addresses1_ where customer0_.GUID=addresses1_.CUSTOMER_GUID 
and (cast(addresses1_.ADDRESS_TYPE as int4)=cast(addresses1_.ADDRESS_TYPE2 as int4) 
or (cast(addresses1_.ADDRESS_TYPE as int4) is null) and (cast(addresses1_.ADDRESS_TYPE2 as int4) is null)))

[maca88]

it should cast the parameter whenever possible, instead of the column

I do agree, I will try to implement the following rules:

• When comparing integral numeric types, pre-evaluate the parameter o.Integer == 5L || o.Short < 4 || o.Long >= 5UL
• When comparing integral numeric parameters with floating-point numeric columns, pre-evaluate the parameter o.Decimal == 4 || o.Double < 5L
• When comparing floating-point numeric types, pre-evaluate the parameter o.Double == 2.3f || o.Float < 2.1d
• When a floating-point numeric type parameter is used for integral and floating-point numeric columns, cast columns o.Double < doubleParam || o.Integer >= doubleParam
• When a floating-point numeric type parameter is used for different floating-point numeric columns, cast the parameter o.Double < floatParam || o.Float > floatParam
• When an integral numeric type parameter is used for different numeric columns, cast the parameter o.Double < intParam || o.Float > intParam || o.Short > intParam

Another possibility may be to handle implicit cast detection

I think that the above rules should be good enough.

Two short column compared with casting

This is already fixed with the current state of this PR, I will add additional tests for that.

[maca88]

The last commit implements almost all rules as described, except for rule:

• When a floating-point numeric type parameter is used for different floating-point numeric columns, cast the parameter

which is implemented partially. For o.Double > singleParam the parameter is casted but for o.Single > decimalParam the column is casted instead.

[maca88]

Rebased.

[maca88]

Reverted the possible regex fix for Firebird as there are other tests that would need to be modified, so tests are now skipped instead. Maybe a better fix would be to use transparentcast, similar what was done for SqLite.

[hazzik]

I did some refactoring for readability.

[maca88]

Thanks!

[bahusoid]

I find it a bit confusing when 2 methods with the same name have some subtle difference in functionality - so I merged them. I hope readability is not affected )

[hazzik]

@maca88 does it solve #2490? Asking for a friend Just want to know if the ticket can be closed with this PR or not.

[maca88]

Yes it does, updated the description of this PR.