Skip to content

NH-3944 - preliminary work, Nh clauses eliminated. #594

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

Closed

Conversation

fredericDelaporte
Copy link
Member

@fredericDelaporte fredericDelaporte commented Apr 10, 2017

As stated in @hazzik's PR in a comment, current implementation of Linq provider seems to diverge from Relinq expected use cases.

The NhHavingClause, NhWithClause and NhJoinClause added to the Relinq model does not match Linq semantic. Moreover, Relinq v2 make it quite harder to add such clauses.

This PR removes these clauses. Contrary to the linked comment expected resolution, I have not moved rewriters using those clauses to the last step. I have instead enriched the VisitorParameters class for allowing tracking Linq clauses requiring a different Hql translation than the default.

This has far less impacts.

This may have a weakness, as stated in a newly added comment in PagingRewriter: if a Clone operation is done on the model after VisitorParameters has been fetched, we may no more be able to match those clauses requiring a different Hql translation. Currently, only PagingRewriter does such a Clone, and in a case not supporting the above clauses, so this does not cause any issue.

hazzik
hazzik previously requested changes Apr 10, 2017
/// </summary>
/// <param name="join">The join.</param>
/// <returns>A list of where clauses to apply as HQL with to the join.</returns>
public List<WhereClause> GetRestrictions(AdditionalFromClause join)
Copy link
Member

Choose a reason for hiding this comment

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

Can we not expose concrete collections?

/// </summary>
/// <param name="join">The join to add.</param>
/// <param name="restrictions">Its restrictions if any.</param>
public void AddLeftJoin(AdditionalFromClause join, List<WhereClause> restrictions)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@hazzik hazzik dismissed their stale review April 11, 2017 11:06

To look closer

@hazzik hazzik self-requested a review April 11, 2017 11:06
@fredericDelaporte fredericDelaporte force-pushed the NH-3944 branch 2 times, most recently from e5486d0 to 802f61b Compare April 11, 2017 15:28
// Cloning may cause having/join/with clauses listed in VisitorParameters to no more be matched.
// Not a problem for now, because those clauses imply a projection, which is not supported
// by the "new WhereClause(new SubQueryExpression(newSubQueryModel))" below. See
// SingleKeyGroupAndCountWithHavingClausePagingAndOuterWhere test by example.
Copy link
Member Author

@fredericDelaporte fredericDelaporte Apr 11, 2017

Choose a reason for hiding this comment

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

This assertion was tested by adding following test to NHibernate.Test.Linq.ByMethod.GroupByHavingTests:

[Test, Explicit("Demonstrate an unsupported case for PagingRewriter")]
public void SingleKeyGroupAndCountWithHavingClausePagingAndOuterWhere()
{
	var orderCounts = db.Orders
		.GroupBy(o => o.Customer.CompanyName)
		.Where(g => g.Count() > 10)
		.Select(g => new { CompanyName = g.Key, OrderCount = g.Count() })
		.OrderBy(oc => oc.CompanyName)
		.Skip(5)
		.Take(10)
		.Where(oc => oc.CompanyName.Contains("F"))
		.ToList();

	Assert.That(orderCounts, Has.Count.EqualTo(3));
	var frankRow = orderCounts.Single(row => row.CompanyName == "Frankenversand");
	Assert.That(frankRow.OrderCount, Is.EqualTo(15));
}

The WhereClause line yields an ArgumentException with message

The items of the input sequence of type
'<>f__AnonymousType6`2[System.String,System.Int32]' are not
compatible with the item expression of type
'System.Linq.IGrouping`2[System.String,NHibernate.DomainModel.Northwind.Entities.Order]'.

Which lets me think this flattening process does not support projection (the Select(g =>), which are needed for introducing having or left join clauses.

I have included that test case in this PR just now.

queryModel.BodyClauses.Add(join);
foreach (var withClause in restrictions)
{
queryModel.BodyClauses.Add(withClause);
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding them to the query model allows them to get transformed by any later TransformExpressions. This has no other usage currently.

They will not be added as HQL where because the query model visitor now check with visitor parameters each where clause for not being known as a with clause.

mainFromClause.FromExpression,
restrictions);
var join = new AdditionalFromClause(mainFromClause.ItemName, mainFromClause.ItemType, mainFromClause.FromExpression);
var restrictions = subQueryModel.BodyClauses.OfType<WhereClause>().ToList();
Copy link
Member Author

Choose a reason for hiding this comment

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

No cloning here. Reusing directly those from the flattened sub-query.

subQueryMainFromClause.ItemType,
subQueryMainFromClause.FromExpression,
restrictions);
var restrictions = subQueryModel.BodyClauses.OfType<WhereClause>().ToList();
Copy link
Member Author

Choose a reason for hiding this comment

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

No cloning here. Reusing directly those from the flattened sub-query.

queryModel.BodyClauses.Insert(index, @join);
InsertBodyClauses(subQueryModel.BodyClauses.Where(b => !(b is WhereClause)), queryModel, index + 1);
queryModel.BodyClauses.Insert(index, join);
InsertBodyClauses(subQueryModel.BodyClauses, queryModel, index + 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

No more filtering of where clauses: we need them in the model for ensuring expressions transformations are applied to them too.
If we were cloning them for building the restrictions list, then we would have to filter them out here then insert their clones.

They will not be added as HQL where because the query model visitor now check with visitor parameters each where clause for not being known as a with clause.

{
queryModel.BodyClauses.RemoveAt(i);
queryModel.BodyClauses.Insert(i, new NhHavingClause(whereClause.Predicate));
parameters.AddHavingClause(whereClause);
Copy link
Member Author

Choose a reason for hiding this comment

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

Juste letting them in the model for ensuring any later expression transformation get applied to them.

They will not be added as HQL where because the query model visitor now check each where clause with the visitor parameters. If they are known as having clauses, they will be added as HQL having clauses instead of where.

@@ -57,14 +58,14 @@ private static void InsertBodyClauses(IEnumerable<IBodyClause> bodyClauses, Quer
foreach (var bodyClause in bodyClauses)
{
destinationQueryModel.BodyClauses.Insert(destinationIndex, bodyClause);
++destinationIndex;
destinationIndex++;
Copy link
Member Author

Choose a reason for hiding this comment

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

Readability. I have seen some debate about performances of the former vs the later. Not only I do know believe it to be relevant for .Net (as Alexander it seems), but also I consider that such tiny optimization should by default be discarded in favor of readability. (Only in very specific cases where it can actually have a human measurable impact would I left such a thing.)

VisitNhJoinClause(querySourceName, joinClause);
}
else if (fromClause.FromExpression is MemberExpression)
if (fromClause.FromExpression is MemberExpression)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, I have to admit I have left the test cases check for me that apparently, NhJoinClause were never used with something not castable to MemberExpression. Otherwise, some cases previously handled with NhJoinClause but not using a MemberExpression will no more work.

It looks to me reasonable to do such an assumption since HQL permit joins only through member references as far as I know.

@fredericDelaporte
Copy link
Member Author

Rebased for resolving conflicts.

@fredericDelaporte
Copy link
Member Author

Choosing #568 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants