GroupBy to custom class fails with ArgumentException

[ador-mg]

When a custom class is used as the GroupBy object:

var results = Session.Query<Project>()
                .GroupBy(
                    a => new GroupByKey(a.Purpose)
                )
                .Select(
                    a => new GroupBySelect(a.Key, a.Count(), a.Sum(o => o.Budget))
                )
                .ToList();

an ArgumentException is raised:

 System.ArgumentException : Incorrect number of arguments for the given members 
   at System.Linq.Expressions.Expression.ValidateNewArgs(ConstructorInfo constructor, ReadOnlyCollection`1& arguments, ReadOnlyCollection`1& members)
   at System.Linq.Expressions.Expression.New(ConstructorInfo constructor, IEnumerable`1 arguments, IEnumerable`1 members)
   at NHibernate.Linq.GroupBy.GroupKeyNominator.VisitNew(NewExpression expression)
   at NHibernate.Linq.GroupBy.GroupKeyNominator.Visit(Expression expression)
   at NHibernate.Linq.GroupBy.GroupKeyNominator.VisitInternal(Expression expr)
   at NHibernate.Linq.GroupBy.GroupKeyNominator.Visit(GroupResultOperator groupBy)
   at NHibernate.Linq.GroupBy.GroupBySelectClauseRewriter..ctor(GroupResultOperator groupBy, QueryModel model)
   at NHibernate.Linq.GroupBy.GroupBySelectClauseRewriter.ReWrite(Expression expression, GroupResultOperator groupBy, QueryModel model)
   at NHibernate.Linq.GroupBy.AggregatingGroupByRewriter.<>c__DisplayClass2_0.<FlattenSubQuery>b__2(Expression s)
   at Remotion.Linq.Clauses.SelectClause.TransformExpressions(Func`2 transformation)
   at NHibernate.Linq.GroupBy.AggregatingGroupByRewriter.FlattenSubQuery(QueryModel queryModel, QueryModel subQueryModel, GroupResultOperator groupBy)
   at NHibernate.Linq.GroupBy.AggregatingGroupByRewriter.ReWrite(QueryModel queryModel)
   at NHibernate.Linq.Visitors.QueryModelVisitor.GenerateHqlQuery(QueryModel queryModel, VisitorParameters parameters, Boolean root, Nullable`1 rootReturnType)
   at NHibernate.Linq.NhLinqExpression.Translate(ISessionFactoryImplementor sessionFactory, Boolean filter)
   at NHibernate.Hql.Ast.ANTLR.ASTQueryTranslatorFactory.CreateQueryTranslators(IQueryExpression queryExpression, String collectionRole, Boolean shallow, IDictionary`2 filters, ISessionFactoryImplementor factory)
   at NHibernate.Engine.Query.QueryExpressionPlan.CreateTranslators(IQueryExpression queryExpression, String collectionRole, Boolean shallow, IDictionary`2 enabledFilters, ISessionFactoryImplementor factory)
   at NHibernate.Engine.Query.QueryExpressionPlan..ctor(IQueryExpression queryExpression, Boolean shallow, IDictionary`2 enabledFilters, ISessionFactoryImplementor factory)
   at NHibernate.Engine.Query.QueryPlanCache.GetHQLQueryPlan(IQueryExpression queryExpression, Boolean shallow, IDictionary`2 enabledFilters)
   at NHibernate.Impl.AbstractSessionImpl.GetHQLQueryPlan(IQueryExpression queryExpression, Boolean shallow)
   at NHibernate.Impl.AbstractSessionImpl.CreateQuery(IQueryExpression queryExpression)
   at NHibernate.Linq.DefaultQueryProvider.PrepareQuery(Expression expression, IQuery& query)
   at NHibernate.Linq.DefaultQueryProvider.Execute(Expression expression)
   at NHibernate.Linq.DefaultQueryProvider.Execute[TResult](Expression expression)
   at Remotion.Linq.QueryableBase`1.GetEnumerator()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)

Test case will follow.

[hazzik]

I doubt this would ever be supported. The reason is simple: the constructor of a class can have a custom logic which could possible be untranslatable to SQL, but the Linq provider has no way to know if this is translatable or not.

The only possible solution is to throw a NotSupportedException with explanations why this is not supported.

[ador-mg]

I found the issue has to do with this method of GroupKeyNominator

protected override Expression VisitNew(NewExpression expression)
{
	_transformed = true;
	// Transform each initializer recursively (to allow for nested initializers)
	return Expression.New(expression.Constructor, expression.Arguments.Select(VisitInternal), expression.Members);
}

Expression.New raises the exception, because expression.Members is null.
The description of the Expression.New method states:

If members is null, the Members property of the resulting NewExpression is an empty collection.

If we call the other overload of the New method (without the members) everything works as expected, i.e.:

protected override Expression VisitNew(NewExpression expression)
{
	_transformed = true;
	// Transform each initializer recursively (to allow for nested initializers)
	if(members == null)
		return Expression.New(expression.Constructor, expression.Arguments.Select(VisitInternal));

	return Expression.New(expression.Constructor, expression.Arguments.Select(VisitInternal), expression.Members);
}

@hazzik: what do you think?

[ador-mg]

Also this used to work in version 4.1.2.GA

[fredericDelaporte]

I have cherry-picked your test case on the 4.1.x branch which currently matches 4.1.2.GA: it fails the same.

By the way, like Alexander, I do not get why grouping by a custom class should work, since the LINQ provider has no way to know what should be the equality semantic for that class. (Grouping by its constructor arguments may not always be suitable: the class own logic could consider some cases of instances built with different arguments values to be equal.)

Rewriting the test case as:

.GroupBy(a => a.Country)
.Select(a => new { groupby = new GroupByEntity(a.Key), cnt = a.Count(), sum = a.Sum(o => o.Budget) })

Makes more sens and does work on 4.1.x and master branch.

That said, VisitNew implementation should yield a better error in that case, as proposed by Alexander.

[ador-mg]

You are right, it does fail on the 4.1.x.
But it does work on 4.0.x, Do you have any idea why?

We have been using that "feature" for a while now because we want to dynamically create group by expressions, using Expression Trees and we couldn't (at the time, not sure now) find another way rather than using class types which we were building on the fly.

In any case I think that Expression.New should not raise an exception when the 3rd argument (members) is null (they state so in the documentation).

Honestly, I don't see the difference between using an anonymous type and a regular class type in the GroupBy clause, as far as the LinqProvider is concerned. e.g.:

.GroupBy(a => new { a.Country })
.Select(a => new { groupby = a.Key, cnt = a.Count(), sum = a.Sum(o => o.Budget) })

It needs to understand which members are used so that it can use them in the sql Group By statement.
Finally, I just saw that my commit passed all the tests (until the time of this) and did not break anything,
so maybe it makes sense to accept the pull request?
Thank you guys for your time.

[fredericDelaporte]

But it does work on 4.0.x, Do you have any idea why?

Indeed. It seems not supporting usage of custom classes in the group by key definition is a unexpected regression caused by #442, affecting version 4.1.0 and higher.

Honestly, I don't see the difference between using an anonymous type and a regular class type in the GroupBy clause, as far as the LinqProvider is concerned.

An anonymous type cannot override Equals, while a regular class can. This can change the grouping results. Linq-to-object will honor the class Equals semantic, while Linq-to-NH will not be able to do this. That is why it can make sens to throw a not supported exception in this case.

Now since this a regression, and also since NHibernate dates back to times at which anonymous types were not available, at which using custom classes for grouping by many columns could be a more convenient and cleaner solution than any other, why not supporting it.

so maybe it makes sense to accept the pull request

There are a bunch of issues to fix in it first, see upcoming review.

[hazzik]

since NHibernate dates back to times at which anonymous types were not available, at which using custom classes for grouping by many columns could be a more convenient and cleaner solution than any other

Both Linq and Anonymous Classes have come together in .NET 3.5.

[fredericDelaporte]

So there is just the fact that losing the support of this case is a possible breaking change for applications migrated from NHibernate v4.0. Either it has to be more explicit by raising an appropriate not supported exception, or it has to be fixed back to the old behavior, grouping by the constructor arguments.

I am for the later, as it seems to me quite explainable than one should not expect to have a custom equality implementation honored by anything else than linq-to-object.