Skip to content

NH-3474 - Fix support of GroupBy with constant keys #434

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.

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

Merged
merged 1 commit into from
Jul 9, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
94 changes: 94 additions & 0 deletions src/NHibernate.Test/Linq/ByMethod/GroupByTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,100 @@ public void GroupByComputedValueInObjectArray()
Assert.AreEqual(830, orderGroups.Sum(g => g.Count));
}

[Test(Description = "NH-3474")]
public void GroupByConstant()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generates
select
cast(count(*) as INT) as col_0_0_,
cast(sum(order0_.Freight) as DECIMAL(19,
5)) as col_1_0_
from
Orders order0_

{
var totals = db.Orders.GroupBy(o => 1).Select(g => new { Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight) }).ToList();
Assert.That(totals.Count, Is.EqualTo(1));
Assert.That(totals, Has.All.With.Property("Key").EqualTo(1));
}

[Test(Description = "NH-3474")]
public void GroupByConstantAnonymousType()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

select
    cast(count(*) as INT) as col_0_0_,
    cast(sum(order0_.Freight) as DECIMAL(19,
    5)) as col_1_0_ 
from
    Orders order0_

{
var totals = db.Orders.GroupBy(o => new { A = 1 }).Select(g => new { Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight) }).ToList();
Assert.That(totals.Count, Is.EqualTo(1));
Assert.That(totals, Has.All.With.Property("Key").With.Property("A").EqualTo(1));
}

[Test(Description = "NH-3474")]
public void GroupByConstantArray()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

select
    cast(count(*) as INT) as col_0_0_,
    cast(sum(order0_.Freight) as DECIMAL(19,
    5)) as col_1_0_ 
from
    Orders order0_

{
var totals = db.Orders.GroupBy(o => new object[] { 1 }).Select(g => new { Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight) }).ToList();
Assert.That(totals.Count, Is.EqualTo(1));
Assert.That(totals, Has.All.With.Property("Key").EqualTo(new object[] { 1 }));
}

[Test(Description = "NH-3474")]
public void GroupByKeyWithConstantInAnonymousType()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

select
    order0_.ShipVia as col_0_0_,
    cast(count(*) as INT) as col_1_0_,
    cast(sum(order0_.Freight) as DECIMAL(19,
    5)) as col_2_0_ 
from
    Orders order0_ 
group by
    order0_.ShipVia

{
var totals = db.Orders.GroupBy(o => new { A = 1, B = o.Shipper.ShipperId }).Select(g => new { Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight) }).ToList();
Assert.That(totals.Count, Is.EqualTo(3));
Assert.That(totals, Has.All.With.Property("Key").With.Property("A").EqualTo(1));
}

[Test(Description = "NH-3474")]
public void GroupByKeyWithConstantInArray()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

select
    order0_.ShipVia as col_0_0_,
    cast(count(*) as INT) as col_1_0_,
    cast(sum(order0_.Freight) as DECIMAL(19,
    5)) as col_2_0_ 
from
    Orders order0_ 
group by
    order0_.ShipVia

{
var totals = db.Orders.GroupBy(o => new[] { 1, o.Shipper.ShipperId }).Select(g => new { Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight) }).ToList();
Assert.That(totals.Count, Is.EqualTo(3));
Copy link
Member

Choose a reason for hiding this comment

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

What sql/hql is generated in each of these cases?

Assert.That(totals, Has.All.With.Property("Key").Contains(1));
}

private int constKey;
[Test(Description = "NH-3474")]
public void GroupByKeyWithConstantFromVariable()
{
constKey = 1;
var q1 = db.Orders.GroupBy(o => constKey).Select(g => new {Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight)});
var q2 = db.Orders.GroupBy(o => new {A = constKey}).Select(g => new {Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight)});
var q3 = db.Orders.GroupBy(o => new object[] {constKey}).Select(g => new {Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight)});
var q4 = db.Orders.GroupBy(o => new {A = constKey, B = o.Shipper.ShipperId}).Select(g => new {Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight)});
var q5 = db.Orders.GroupBy(o => new[] {constKey, o.Shipper.ShipperId}).Select(g => new {Key = g.Key, Count = g.Count(), Sum = g.Sum(x => x.Freight)});

var r1_1 = q1.ToList();
Assert.That(r1_1.Count, Is.EqualTo(1));
Assert.That(r1_1, Has.All.With.Property("Key").EqualTo(1));

var r2_1 = q2.ToList();
Assert.That(r2_1.Count, Is.EqualTo(1));
Assert.That(r2_1, Has.All.With.Property("Key").With.Property("A").EqualTo(1));

var r3_1 = q3.ToList();
Assert.That(r3_1.Count, Is.EqualTo(1));
Assert.That(r3_1, Has.All.With.Property("Key").EqualTo(new object[] { 1 }));

var r4_1 = q4.ToList();
Assert.That(r4_1.Count, Is.EqualTo(3));
Assert.That(r4_1, Has.All.With.Property("Key").With.Property("A").EqualTo(1));

var r5_1 = q5.ToList();
Assert.That(r5_1.Count, Is.EqualTo(3));
Assert.That(r5_1, Has.All.With.Property("Key").Contains(1));

constKey = 2;

var r1_2 = q1.ToList();
Assert.That(r1_2.Count, Is.EqualTo(1));
Assert.That(r1_2, Has.All.With.Property("Key").EqualTo(2));

var r2_2 = q2.ToList();
Assert.That(r2_2.Count, Is.EqualTo(1));
Assert.That(r2_2, Has.All.With.Property("Key").With.Property("A").EqualTo(2));

var r3_2 = q3.ToList();
Assert.That(r3_2.Count, Is.EqualTo(1));
Assert.That(r3_2, Has.All.With.Property("Key").EqualTo(new object[] { constKey }));

var r4_2 = q4.ToList();
Assert.That(r4_2.Count, Is.EqualTo(3));
Assert.That(r4_2, Has.All.With.Property("Key").With.Property("A").EqualTo(2));

var r5_2 = q5.ToList();
Assert.That(r5_2.Count, Is.EqualTo(3));
Assert.That(r5_2, Has.All.With.Property("Key").Contains(2));
}

[Test(Description = "NH-3801")]
public void GroupByComputedValueWithJoinOnObject()
{
Expand Down
19 changes: 19 additions & 0 deletions src/NHibernate/Linq/GroupBy/AggregatingGroupByRewriter.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using NHibernate.Linq.Clauses;
using NHibernate.Linq.ReWriters;
using NHibernate.Linq.Visitors;
Expand Down Expand Up @@ -58,6 +59,7 @@ public static void ReWrite(QueryModel queryModel)
if (groupBy != null)
{
FlattenSubQuery(queryModel, subQueryExpression.QueryModel, groupBy);
RemoveCostantGroupByKeys(queryModel, groupBy);
}
}
}
Expand Down Expand Up @@ -101,5 +103,22 @@ private static void FlattenSubQuery(QueryModel queryModel, QueryModel subQueryMo
// Replace the outer query source
queryModel.MainFromClause = subQueryModel.MainFromClause;
}

private static void RemoveCostantGroupByKeys(QueryModel queryModel, GroupResultOperator groupBy)
{
var keys = groupBy.ExtractKeyExpressions().Where(x => !(x is ConstantExpression)).ToList();

if (!keys.Any())
{
// Remove the Group By clause completely if all the keys are constant (redundant)
queryModel.ResultOperators.Remove(groupBy);
Copy link
Member

Choose a reason for hiding this comment

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

Please include test for NH-2500 (overcaching) in conclusion with this feature. It might be the case.

}
else
{
// Re-write the KeySelector as an object array of the non-constant keys
// This should be safe because we've already re-written the select clause using the original keys
groupBy.KeySelector = Expression.NewArrayInit(typeof (object), keys.Select(x => x.Type.IsValueType ? Expression.Convert(x, typeof(object)) : x));
}
}
}
}
22 changes: 22 additions & 0 deletions src/NHibernate/Linq/GroupResultOperatorExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Text;
using NHibernate.Util;
using Remotion.Linq.Clauses.ResultOperators;

namespace NHibernate.Linq
{
internal static class GroupResultOperatorExtensions
{
public static IEnumerable<Expression> ExtractKeyExpressions(this GroupResultOperator groupResult)
{
if (groupResult.KeySelector is NewExpression)
return (groupResult.KeySelector as NewExpression).Arguments;
if (groupResult.KeySelector is NewArrayExpression)
return (groupResult.KeySelector as NewArrayExpression).Expressions;
return new [] { groupResult.KeySelector };
}
}
}
15 changes: 15 additions & 0 deletions src/NHibernate/Linq/Visitors/ExpressionKeyVisitor.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
Expand Down Expand Up @@ -92,9 +93,22 @@ protected override Expression VisitConstantExpression(ConstantExpression express
else
{
if (expression.Value == null)
{
_string.Append("NULL");
}
else if (expression.Type.IsArray)
{
// Const arrays all look the same (they just display the type of array and not the initializer contents
// Since the contents might be different, we need to include those as well so we don't use a cached query plan by mistake
_string.Append(expression.Value);
_string.Append(" {");
_string.Append(String.Join(",", (object[]) expression.Value));
_string.Append("}");
}
else
{
_string.Append(expression.Value);
}
}

return base.VisitConstantExpression(expression);
Expand Down Expand Up @@ -142,6 +156,7 @@ protected override Expression VisitMethodCallExpression(MethodCallExpression exp
case "Single":
case "SingleOrDefault":
case "Select":
case "GroupBy":
insideSelectClause = true;
break;
default:
Expand Down
1 change: 1 addition & 0 deletions src/NHibernate/NHibernate.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@
<Compile Include="Linq\Functions\EqualsGenerator.cs" />
<Compile Include="Linq\GroupBy\KeySelectorVisitor.cs" />
<Compile Include="Linq\GroupBy\PagingRewriter.cs" />
<Compile Include="Linq\GroupResultOperatorExtensions.cs" />
<Compile Include="Linq\NestedSelects\NestedSelectDetector.cs" />
<Compile Include="Linq\NestedSelects\Tuple.cs" />
<Compile Include="Linq\NestedSelects\SelectClauseRewriter.cs" />
Expand Down