Minimize Linq query parameters

[maca88]

Fixes: #2241

With this PR, only one sql parameter will be generated for each variable, except for ODBC drivers that do not support named parameters.

Possible breaking changes:

• VisitorParameters.ConstantToParameterMap may contain the same parameter for multiple constant expressions.

[hazzik]

There are complictions with this approach - it would mean that the same queries but different parameter values would have different query cache key (otherwise there will be an "overcaching issue"). This would lead to the re-translation of the linq query.

[maca88]

it would mean that the same queries but different parameter values would have different query cache key

Correct, in the current state this PR may create different query plans when there are parameters with the same type, which is not ideal.
What do you think if we would have the following logic for calculating parameters:

• A inline constant value would not be translated to a sql parameter

db.Products.Where(o => o.Name == "teest" && o.Active == true) // Zero sql parameters, both values would be translated to sql

• Each variable would be translated to a single sql parameter

var value = "test";
db.Products.Where(o => o.Name == value && o.Name != value) // One sql parameter

• Each inline non constant value would be translated to a separate sql parameter (remains the same)

db.Products.Where(o => o.Name == GetName(true) && o.Name != GetName(true)) // Two sql parameter

string GetName(bool value) 
{
	return value ? "test" : "test2";
}

I am not sure what it would take to implement such logic, but I would like to hear you opinions.

[maca88]

I forgot the case when an external libary can dynamically create a linq expression (e.g. OData) which means that the first rule (inline constant) cannot be applied. I will try to implement only the second rule (parameter for each variable).

[bahusoid]

Each variable would be translated to a single sql parameter

That's IMHO the only change that needs to be implemented in scope of this PR. And keep existing behavior for other cases. (I assume that existing behavior - new param generation for all cases)

A inline constant value would not be translated to a sql parameter

I'm not sure that this behavior should be done as default. But as optional it would be actually very useful for query optimization purposes

[maca88]

With the last commit only the second rule was implemented and the value matching logic was removed. Also, now the following queries will produce only one parameter:

var value = new Product {Name = "test"};
db.Products.Where(o => o.Name == value.Name && o.Name != value.Name).ToList();
db.Orders.Where(o => o.OrderId == -value.ProductId && o.OrderId != -value.ProductId).ToList();

[maca88]

I created a separate class in order to be able to add additional data in the future.

[bahusoid]

Maybe QueryMode should be added here? And store PartialEvaluationResult as field in ExpressionParameterVisitor

[bahusoid]

Or maybe not... I see it's not very convenient..

[maca88]

Maybe QueryMode should be added here?

If we add it here, then we would need to add it as internal otherwise would be confusing for users as it is not populated by the PreTransform method. But if we add it as internal, then users (also in our tests) cannot simulate what nhibernate does as QueryMode would be set internally by the NhLinqExpression. That is why I added it as an additional parameter. Also, with #2079 ExpressionParameterVisitor will be obsolete.

[bahusoid]

Well PartialEvaluate can take QueryMode as additional parameter... But OK - maybe it's not worth it.

Also, with #2079 ExpressionParameterVisitor will be obsolete.

Could you please clarify how it correlates with this PR. Will you need to modify ExpressionKeyVisitor or what's used instead in similar way as you modified ExpressionParameterVisitor?

[maca88]

Well PartialEvaluate can take QueryMode as additional parameter

That is another option which I think it is even better. By passing the information to the PreTransform method we could skip the variable detection logic for DML queries and avoid passing the QueryMode to ExpressionParameterVisitor as QueryVariables dictionary will be empty. With the last commit the QueryMode is passed into PreTransform method instead.

Will you need to modify ExpressionKeyVisitor or what's used instead in similar way as you modified ExpressionParameterVisitor?

The logic of ExpressionParameterVisitor that detects whether an expression is a parameter or not, was moved into ExpressionKeyVisitor, other stuff like removing MappedAs calls and detecting parameter types was moved in the translation phase. So yes, the ExpressionKeyVisitor will need to be modified similar than ExpressionParameterVisitor is modified here.

[maca88]

Used the same condition as Microsoft uses in their ORM in order to detect the compiler generated class containing the variables.

[bahusoid]

Just curious how will it handle new[]{ value, 2}.Contains(o.OrderId) && new []{value, 3}.Contains(o.OrderId)?
3 or 4 parameters will be added?

[maca88]

With the current logic 4 parameters will be added. This is quite a special case as new[]{ value, 2} is treated as one parameter and expanded later when executing the query.

[bahusoid]

Maybe cover it with test? And could you also add test for DML LINQ query.
Will this logic work for queries like (I'm actually not sure if it should be done for DML linq queries for UPDATE part):

db.Customers.Where(c => c.CustomerId == "UNKNOWN").Update(x => new Customer {ContactName = value, CompanyName = value});

[maca88]

Maybe cover it with test?

Added.

And could you also add test for DML LINQ query

I forgot about them. After some testing I found out that HQL does not support reusing parameters for DML queries, so now the new logic is not used for them anymore. Also, tests were added.

[maca88]

Rebased.

[maca88]

Added a separate class, in order to be able to add addtitional information to pre-transformation phase in the future. One example would be to add support for:

• A inline constant value would not be translated to a sql parameter, described here.

in order to add such feature we would need to extend NhLinqExpression constructor with NhQueryableOptions parameter that would be passed to PreTransform method, which would contain the information whether the parameters needs to be inlined or not.

[maca88]

Not a breaking change as the overload was added in #1868, which is not yet released.

[maca88]

Rebased.