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.

Sign up for GitHub

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

Jump to bottom

Fix for HQL query plan regenerate problem #3069 #3168

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Fix for HQL query plan regenerate problem #3069 #3168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d408649
NH3069 HQL query plan cache problem resolved
Sep 27, 2022
caff0be
Commented line removed
Sep 27, 2022
90498b9
Sync unit tests added
Sep 27, 2022
8ef7b54
Merge branch 'master' into NH3069
cokalyoncu Sep 27, 2022
7f22e24
Generate async files
github-actions[bot] Sep 27, 2022
7055f7e
SelectClauseNominator constant improvment
Sep 28, 2022
484bb14
queries should be cached
Sep 29, 2022
e447a85
Generate async files
github-actions[bot] Sep 29, 2022
17721fe
imrovments
Sep 30, 2022
a9a9e11
Merge branch 'NH3069' of https://github.com/cokalyoncu/nhibernate-cor…
Sep 30, 2022
b474a48
fix for some failed cases
Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 87 additions & 4 deletions src/NHibernate.Test/Async/Linq/ConstantTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
//------------------------------------------------------------------------------


using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
Expand Down Expand Up @@ -136,7 +137,7 @@ public async Task ConstantNonCachedInMemberInitExpressionWithConditionAsync()
return db.Shippers.Where(o => o.ShipperId == id)
.Select(o => new ShipperDto {Number = id, CompanyName = o.CompanyName}).SingleAsync(cancellationToken);
}
catch (System.Exception ex)
catch (Exception ex)
{
return Task.FromException<ShipperDto>(ex);
}
Expand Down Expand Up @@ -323,7 +324,7 @@ public async Task DmlPlansAreCachedAsync()
}

[Test]
public async Task PlansWithNonParameterizedConstantsAreNotCachedAsync()
public async Task PlansWithNonParameterizedConstantsAreCachedAsync()
{
var queryPlanCacheType = typeof(QueryPlanCache);

Expand All @@ -338,8 +339,90 @@ public async Task PlansWithNonParameterizedConstantsAreNotCachedAsync()
select new { c.CustomerId, c.ContactName, Constant = 1 }).FirstAsync());
Assert.That(
cache,
Has.Count.EqualTo(0),
"Query plan should not be cached.");
Has.Count.EqualTo(1),
"Query should be cached.");
}

[Test]
public async Task PlansWithConstantExpressionsAreNotCachedAsync()
{
var queryPlanCacheType = typeof(QueryPlanCache);
var cache = (SoftLimitMRUCache) queryPlanCacheType.GetField("planCache", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(Sfi.QueryPlanCache);
cache.Clear();

var input = new { Name = "ALFKI" };
await ((from c in db.Customers
where c.CustomerId == "ALFKI"
select new { c.CustomerId, c.ContactName, DummyBooleanColumn = c.CustomerId == input.Name }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(1), "Query should be cached");

await ((from c in db.Customers
where c.CustomerId == "ALFKI"
select new { c.CustomerId, c.ContactName, DummyBooleanColumn = c.CustomerId != "ALFKI" }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(2), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyBooleanColumn = p.Discontinued && true }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(3), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyBooleanColumn = p.Discontinued || true }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(4), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyBooleanColumn = p.ShippingWeight > 10 }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(5), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyBooleanColumn = p.ShippingWeight < 10 }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(6), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyBooleanColumn = p.ShippingWeight >= 10 }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(7), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyBooleanColumn = p.ShippingWeight <= 10 }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(8), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyColumn = p.ShippingWeight > 0 ? DateTime.Now : default(DateTime?) }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(9), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyColumn = p.UnitPrice + 10 }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(10), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyColumn = p.ShippingWeight - 10 }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(11), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyColumn = p.ShippingWeight * 10 }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(12), "Query should be cached");

await ((from p in db.Products
select new { p.Name, DummyColumn = p.ShippingWeight / 10 }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(13), "Query should be cached");

await ((from c in db.Customers
where c.CustomerId == "ALFKI"
select new { c.CustomerId, c.ContactName, DummyColumn = c.CustomerId ?? "TEST" }).FirstAsync());

Assert.That(cache, Has.Count.EqualTo(14), "Query should be cached");
}

[Test]
Expand Down
91 changes: 87 additions & 4 deletions src/NHibernate.Test/Linq/ConstantTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections.Generic;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using NHibernate.Criterion;
Expand Down Expand Up @@ -340,7 +341,7 @@ public void DmlPlansAreCached()
}

[Test]
public void PlansWithNonParameterizedConstantsAreNotCached()
public void PlansWithNonParameterizedConstantsAreCached()
{
var queryPlanCacheType = typeof(QueryPlanCache);

Expand All @@ -355,8 +356,90 @@ public void PlansWithNonParameterizedConstantsAreNotCached()
select new { c.CustomerId, c.ContactName, Constant = 1 }).First();
Assert.That(
cache,
Has.Count.EqualTo(0),
"Query plan should not be cached.");
Has.Count.EqualTo(1),
"Query should be cached.");
}

[Test]
public void PlansWithConstantExpressionsAreNotCached()
{
var queryPlanCacheType = typeof(QueryPlanCache);
var cache = (SoftLimitMRUCache) queryPlanCacheType.GetField("planCache", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(Sfi.QueryPlanCache);
cache.Clear();

var input = new { Name = "ALFKI" };
(from c in db.Customers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gokhanabatay for all these queries you should check for the results returned. Otherwise it might be that not only plan is cached but also results are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me part of the reason is written here:

From simple change it's converted to something else. Let's focus on simple fix in this PR and expression types that require additional handling be moved in separate PR.

I cannot understand, if there is major issues so major changes needs to be applied. We want to fix these problems that we are reporting past 3 years mostly linq performance issues.

@hazzik @fredericDelaporte
Maca just made good improvements my question is what are we waiting, we are volunteered if you guide us we can make necessary changes to merge these pull requests. #2079 and #2375.

Because our changes are much simpler then maca's, we were repeating its changes without knowing its already done.
https://github.com/nhibernate/nhibernate-core/blob/04e40931af0d812810ef176deb452847154b6ee9/src/NHibernate/Linq/Visitors/SelectClauseNominator.cs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me part of the reason is written here:

From simple change it's converted to something else. Let's focus on simple fix in this PR and expression types that require additional handling be moved in separate PR.

I cannot understand, if there is major issues so major changes needs to be applied. We want to fix these problems that we are reporting past 3 years mostly linq performance issues.

@hazzik @fredericDelaporte Maca just made good improvements my question is what are we waiting, we are volunteered if you guide us we can make necessary changes to merge these pull requests. #2079 and #2375.

Because our changes are much simpler then maca's, we were repeating its changes without knowing its already done. https://github.com/nhibernate/nhibernate-core/blob/04e40931af0d812810ef176deb452847154b6ee9/src/NHibernate/Linq/Visitors/SelectClauseNominator.cs

@maca88 could you please tell us, what are the necessary changes your pull request neeeds to merge. Or dou you have a plan to work on this. We are talking these issues with you 2019 to today. #2079 and #2375.

where c.CustomerId == "ALFKI"
select new { c.CustomerId, c.ContactName, DummyBooleanColumn = c.CustomerId == input.Name }).First();

Assert.That(cache, Has.Count.EqualTo(1), "Query should be cached");

(from c in db.Customers
where c.CustomerId == "ALFKI"
select new { c.CustomerId, c.ContactName, DummyBooleanColumn = c.CustomerId != "ALFKI" }).First();

Assert.That(cache, Has.Count.EqualTo(2), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyBooleanColumn = p.Discontinued && true }).First();

Assert.That(cache, Has.Count.EqualTo(3), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyBooleanColumn = p.Discontinued || true }).First();

Assert.That(cache, Has.Count.EqualTo(4), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyBooleanColumn = p.ShippingWeight > 10 }).First();

Assert.That(cache, Has.Count.EqualTo(5), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyBooleanColumn = p.ShippingWeight < 10 }).First();

Assert.That(cache, Has.Count.EqualTo(6), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyBooleanColumn = p.ShippingWeight >= 10 }).First();

Assert.That(cache, Has.Count.EqualTo(7), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyBooleanColumn = p.ShippingWeight <= 10 }).First();

Assert.That(cache, Has.Count.EqualTo(8), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyColumn = p.ShippingWeight > 0 ? DateTime.Now : default(DateTime?) }).First();

Assert.That(cache, Has.Count.EqualTo(9), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyColumn = p.UnitPrice + 10 }).First();

Assert.That(cache, Has.Count.EqualTo(10), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyColumn = p.ShippingWeight - 10 }).First();

Assert.That(cache, Has.Count.EqualTo(11), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyColumn = p.ShippingWeight * 10 }).First();

Assert.That(cache, Has.Count.EqualTo(12), "Query should be cached");

(from p in db.Products
select new { p.Name, DummyColumn = p.ShippingWeight / 10 }).First();

Assert.That(cache, Has.Count.EqualTo(13), "Query should be cached");

(from c in db.Customers
where c.CustomerId == "ALFKI"
select new { c.CustomerId, c.ContactName, DummyColumn = c.CustomerId ?? "TEST" }).First();

Assert.That(cache, Has.Count.EqualTo(14), "Query should be cached");
}

[Test]
Expand Down
93 changes: 84 additions & 9 deletions src/NHibernate/Linq/Visitors/SelectClauseNominator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
using NHibernate.Param;
using NHibernate.Util;
using Remotion.Linq.Parsing;
using Remotion.Linq.Clauses.Expressions;
using Remotion.Linq.Clauses;
using NHibernate.Linq.Clauses;

namespace NHibernate.Linq.Visitors
{
Expand Down Expand Up @@ -49,7 +52,7 @@ internal Expression Nominate(Expression expression)
ContainsUntranslatedMethodCalls = false;
_canBeCandidate = true;
_stateStack = new Stack<bool>();
_stateStack.Push(false);
_stateStack.Push(true);

return Visit(expression);
}
Expand All @@ -67,16 +70,14 @@ public override Expression Visit(Expression expression)
return innerExpression;
}

var projectConstantsInHql = _stateStack.Peek() || expression.NodeType == ExpressionType.Equal || IsRegisteredFunction(expression);
var isRegisteredFunction = IsRegisteredFunction(expression);
var projectConstantsInHql = (_stateStack.Peek() && IsConstantExpression(expression)) || isRegisteredFunction;

// Set some flags, unless we already have proper values for them:
// projectConstantsInHql if they are inside a method call executed server side.
// ContainsUntranslatedMethodCalls if a method call must be executed locally.
var isMethodCall = expression.NodeType == ExpressionType.Call;
if (isMethodCall && (!projectConstantsInHql || !ContainsUntranslatedMethodCalls))
if (expression.NodeType == ExpressionType.Call && (!projectConstantsInHql || !ContainsUntranslatedMethodCalls))
{
var isRegisteredFunction = IsRegisteredFunction(expression);
projectConstantsInHql = projectConstantsInHql || isRegisteredFunction;
ContainsUntranslatedMethodCalls = ContainsUntranslatedMethodCalls || !isRegisteredFunction;
}

Expand All @@ -96,7 +97,7 @@ public override Expression Visit(Expression expression)

if (_canBeCandidate)
{
if (CanBeEvaluatedInHqlSelectStatement(expression, projectConstantsInHql))
if (CanBeEvaluatedInHqlSelectStatement(expression, projectConstantsInHql, isRegisteredFunction))
{
HqlCandidates.Add(expression);
}
Expand All @@ -115,6 +116,80 @@ public override Expression Visit(Expression expression)
return expression;
}

private static bool IsAllowedToProjectInHql(System.Type type)
{
return (type.IsValueType || type == typeof(string)) && typeof(DateTime) != type && typeof(DateTime?) != type && typeof(TimeSpan) != type && typeof(TimeSpan?) != type;
}

private static bool IsValueType(System.Type type)
{
return type.IsValueType || type == typeof(string);
}

private static bool IsConstantExpression(Expression expression)
{
//if (expression.NodeType != ExpressionType.Equal) return false;

if(expression == null) return true;

switch (expression.NodeType)
{
case ExpressionType.Equal:
case ExpressionType.NotEqual:
case ExpressionType.Not:
case ExpressionType.MemberInit:
case ExpressionType.New:
case ExpressionType.NewArrayInit:
case ExpressionType.ListInit:
return true;
case ExpressionType.Extension:
if( expression is QuerySourceReferenceExpression extension &&
(extension.ReferencedQuerySource is NhClauseBase ||
(extension.ReferencedQuerySource is MainFromClause fromClause &&
fromClause.FromExpression.Type.IsGenericType &&
fromClause.FromExpression.Type.GetGenericTypeDefinition() == typeof(NhQueryable<>))))
{
return true;
}
return false;
case ExpressionType.Convert:
var convert = (UnaryExpression) expression;
return convert.Method == null && IsAllowedToProjectInHql(convert.Operand.Type) && IsConstantExpression(convert.Operand);
case ExpressionType.Constant:
var constant = (ConstantExpression) expression;
return constant.Value == null || IsValueType(expression.Type);
case ExpressionType.MemberAccess:
var member = (MemberExpression) expression;
return IsConstantExpression(member.Expression);
case ExpressionType.LessThan:
case ExpressionType.LessThanOrEqual:
case ExpressionType.GreaterThan:
case ExpressionType.GreaterThanOrEqual:
case ExpressionType.And:
case ExpressionType.AndAlso:
case ExpressionType.Or:
case ExpressionType.OrElse:
case ExpressionType.Add:
case ExpressionType.Subtract:
case ExpressionType.Multiply:
case ExpressionType.Divide:
case ExpressionType.Modulo:
var binary = (BinaryExpression) expression;
return IsAllowedToProjectInHql(binary.Left.Type) && IsConstantExpression(binary.Left) &&
IsAllowedToProjectInHql(binary.Right.Type) && IsConstantExpression(binary.Right);
case ExpressionType.Coalesce:
var coalesce = (BinaryExpression) expression;
return IsConstantExpression(coalesce.Left) && IsConstantExpression(coalesce.Right);
case ExpressionType.Conditional:
var conditional = (ConditionalExpression) expression;
return IsConstantExpression(conditional.Test) &&
IsValueType(conditional.IfTrue.Type) && IsConstantExpression(conditional.IfTrue) &&
IsValueType(conditional.IfFalse.Type) && IsConstantExpression(conditional.IfFalse);
default:
return false;
}
}

private bool IsRegisteredFunction(Expression expression)
{
if (expression.NodeType == ExpressionType.Call)
Expand All @@ -141,7 +216,7 @@ expression is NhMaxExpression ||
return false;
}

private bool CanBeEvaluatedInHqlSelectStatement(Expression expression, bool projectConstantsInHql)
private bool CanBeEvaluatedInHqlSelectStatement(Expression expression, bool projectConstantsInHql, bool isRegisteredFunction)
{
// HQL can't do New or Member Init
if (expression.NodeType == ExpressionType.MemberInit ||
Expand All @@ -166,7 +241,7 @@ private bool CanBeEvaluatedInHqlSelectStatement(Expression expression, bool proj
if (expression.NodeType == ExpressionType.Call)
{
// Depends if it's in the function registry
return IsRegisteredFunction(expression);
return isRegisteredFunction;
}

if (expression.NodeType == ExpressionType.Conditional)
Expand Down