Skip to content

Support LINQ GroupBy subquery #2210

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
Sep 27, 2019
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
35 changes: 34 additions & 1 deletion src/NHibernate.Test/Async/Linq/NestedSelectsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -362,5 +362,38 @@ public async Task OrdersIdWithOrderLinesNestedWhereIdAsync()
Assert.That(orders.Count, Is.EqualTo(830));
Assert.That(orders[0].OrderLinesIds, Is.Empty);
}

[Test]
public async Task NoNestedSelects_AnyOnGroupBySubqueryAsync()
{
var subQuery = from vms in db.Animals
group vms by vms.Father
into vmReqs
select vmReqs.Max(x => x.Id);

var outerQuery = from vm in db.Animals
where subQuery.Any(x => vm.Id == x)
select vm;
var animals = await (outerQuery.ToListAsync());
Assert.That(animals.Count, Is.EqualTo(2));
}

//NH-3155
[Test]
public async Task NoNestedSelects_ContainsOnGroupBySubqueryAsync()
{
var subQuery = from vms in db.Animals
where vms.BodyWeight > 0
group vms by vms.Father
into vmReqs
select vmReqs.Max(x => x.Id);

var outerQuery = from vm in db.Animals
where subQuery.Contains(vm.Id)
select vm;

var animals = await (outerQuery.ToListAsync());
Assert.That(animals.Count, Is.EqualTo(2));
}
}
}
}
35 changes: 34 additions & 1 deletion src/NHibernate.Test/Linq/NestedSelectsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -364,5 +364,38 @@ public void OrdersIdWithOrderLinesNestedWhereId()
Assert.That(orders.Count, Is.EqualTo(830));
Assert.That(orders[0].OrderLinesIds, Is.Empty);
}

[Test]
public void NoNestedSelects_AnyOnGroupBySubquery()
{
var subQuery = from vms in db.Animals
group vms by vms.Father
into vmReqs
select vmReqs.Max(x => x.Id);

var outerQuery = from vm in db.Animals
where subQuery.Any(x => vm.Id == x)
select vm;
var animals = outerQuery.ToList();
Assert.That(animals.Count, Is.EqualTo(2));
}

//NH-3155
[Test]
public void NoNestedSelects_ContainsOnGroupBySubquery()
{
var subQuery = from vms in db.Animals
where vms.BodyWeight > 0
group vms by vms.Father
into vmReqs
select vmReqs.Max(x => x.Id);

var outerQuery = from vm in db.Animals
where subQuery.Contains(vm.Id)
select vm;

var animals = outerQuery.ToList();
Assert.That(animals.Count, Is.EqualTo(2));
}
}
}
}
3 changes: 2 additions & 1 deletion src/NHibernate/Linq/GroupBy/AggregatingGroupByRewriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ public static class AggregatingGroupByRewriter
typeof(FirstResultOperator),
typeof(SingleResultOperator),
typeof(AnyResultOperator),
typeof(AllResultOperator)
typeof(AllResultOperator),
typeof(ContainsResultOperator),
};

public static void ReWrite(QueryModel queryModel)
Expand Down
7 changes: 7 additions & 0 deletions src/NHibernate/Linq/NestedSelects/NestedSelectRewriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ public static void ReWrite(QueryModel queryModel, ISessionFactory sessionFactory
replacements.Add(expression, processed);
}

if (!replacements.Any())
return;

var key = Expression.Property(group, IGroupingKeyProperty);

var expressions = new List<ExpressionHolder>();
Expand Down Expand Up @@ -92,6 +95,10 @@ private static Expression ProcessExpression(QueryModel queryModel, ISessionFacto

private static Expression ProcessSubquery(ISessionFactory sessionFactory, ICollection<ExpressionHolder> elementExpression, QueryModel queryModel, Expression @group, QueryModel subQueryModel)
{
var resultTypeOverride = subQueryModel.ResultTypeOverride;
if (resultTypeOverride != null && !resultTypeOverride.IsArray && !resultTypeOverride.IsEnumerableOfT())
return null;

var subQueryMainFromClause = subQueryModel.MainFromClause;

var restrictions = subQueryModel.BodyClauses
Expand Down
22 changes: 21 additions & 1 deletion src/NHibernate/Linq/Visitors/QueryModelVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ public override void VisitNhJoinClause(NhJoinClause joinClause, QueryModel query
public override void VisitResultOperator(ResultOperatorBase resultOperator, QueryModel queryModel, int index)
{
PreviousEvaluationType = CurrentEvaluationType;
CurrentEvaluationType = resultOperator.GetOutputDataInfo(PreviousEvaluationType);
CurrentEvaluationType = GetOutputDataInfo(resultOperator, PreviousEvaluationType);

if (resultOperator is ClientSideTransformOperator)
{
Expand All @@ -382,6 +382,26 @@ public override void VisitResultOperator(ResultOperatorBase resultOperator, Quer
ResultOperatorMap.Process(resultOperator, this, _hqlTree);
}

private static IStreamedDataInfo GetOutputDataInfo(ResultOperatorBase resultOperator, IStreamedDataInfo evaluationType)
{
//ContainsResultOperator contains data integrity check so for `values.Contains(x)` it checks that 'x' is proper type to be used inside 'values.Contains()'
//Due to some reasons (possibly NH expression rewritings) those types might be incompatible (known case NH-3155 - group by subquery). So resultOperator.GetOutputDataInfo throws something like:
//System.ArgumentException : The items of the input sequence of type 'System.Linq.IGrouping`2[System.Object[],EntityType]' are not compatible with the item expression of type 'System.Int32'.
//Parameter name: inputInfo
//at Remotion.Linq.Clauses.ResultOperators.ContainsResultOperator.GetOutputDataInfo(StreamedSequenceInfo inputInfo)
//But in this place we don't really care about types involving inside expression, all we need to know is operation result which is bool for Contains
//So let's skip possible type exception mismatch if it allows to generate proper SQL
switch (resultOperator)
Copy link
Member

@hazzik hazzik Sep 12, 2019

Choose a reason for hiding this comment

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

Instead of doing it here, can we fix the condition at its source?

Copy link
Member Author

@bahusoid bahusoid Sep 12, 2019

Choose a reason for hiding this comment

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

I don't really understand the real source of the issue. You should know this parts much better than me. And as this issue hangs since 2012 I think it's worth to implement such simple workaround.

Copy link
Member

Choose a reason for hiding this comment

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

The issue is removing MaxResultOperator and replacing it with NhMaxExpression.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about Min and others aggregate functions? It's Group By specific not Max specific.

Copy link
Member

Choose a reason for hiding this comment

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

All of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's exactly the reason not to do it. To keep workaround in one place and it doesn't affect perf
(instead of type integrity check inside we do this resultOperator check)

Copy link
Member Author

Choose a reason for hiding this comment

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

If you are suggesting that changing NhMaxExpression and etc. will fix issue at its core and it won't be workaround than please make those changes yourself and I will close this PR. As I still don't understand what fix exactly do you have in mind.

Copy link
Member

Choose a reason for hiding this comment

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

I'm probably fine with the proposed change.

{
case ContainsResultOperator _:
case AnyResultOperator _:
Copy link
Member Author

Choose a reason for hiding this comment

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

AnyResultOperator actually currently doesn't throw on type mismatch. But it looks like omission in ReLinq as I see no reasons why it should behave any differently. So added it just in case if it's changed in ReLinq.

case AllResultOperator _:
return new StreamedScalarValueInfo(typeof(bool));
}

return resultOperator.GetOutputDataInfo(evaluationType);
}

public override void VisitSelectClause(SelectClause selectClause, QueryModel queryModel)
{
CurrentEvaluationType = selectClause.GetOutputDataInfo();
Expand Down