Remove OrderByClause from query models with Contains, All and Any result operators

[hazzik]

Improvement for an unwanted behavior found on #2480

[hazzik]

/cc @ngbrown

[bahusoid]

You can't simply remove OrderBy as it does matter for sub-queries with TOP:

var ordersQuery = db.Orders
                    .Where(x => x.Employee.EmployeeId > 5)
                    .OrderBy(x => x.OrderId)
		    .Take(2);

var orderLinesFuture = db.OrderLines
                         .Where(x => ordersQuery.Any(o => o == x.Order))
                         .OrderBy(x => x.Id)
                         .ToFuture();

In case paging rewriter this OrdereBy and TOP goes to IN subquery so it's OK to remove it in outer query for exists and in:

select
*
from
    OrderLines orderline0_ 
where
    exists (
        select
            order1_.OrderId 
        from
            Orders order1_ 
        where
            order1_.OrderId=orderline0_.OrderId 
            and (
                order1_.OrderId in (
                    select
                        TOP (@p0) order2_.OrderId 
                    from
                        Orders order2_ 
                    where
                        order2_.EmployeeId>@p1 
                    order by
                        order2_.OrderId asc -- this order by CAN'T be removed
                )
            --order by order2_.OrderId asc -- this can be removed
            )
        ) 
    order by
        orderline0_.OrderLineId asc;

[hazzik]

Yeah, it seems it runs a second time again. And removes after PagingRewriter....

[hazzik]

@bahusoid ok. Seems to be fixed.

[bahusoid]

Now it seems we are doing twice job that should be done only once by your code. How safe would it be to move RemoveUnnecessaryBodyOperators right after PagingRewriter? Then it seems we could drop check introduced in 848b915:

nhibernate-core/src/NHibernate/Linq/GroupBy/PagingRewriter.cs

Line 57 in 848b915

!(queryModel.ResultOperators.Count == 1 && queryModel.ResultOperators.All(x => x is AnyResultOperator || x is ContainsResultOperator || x is AllResultOperator)))

[bahusoid]

How safe would it be

Not quite... CanExecuteCountWithOrderByArguments fails...

[hazzik]

This thing would not pick up the QueryModel which is processed by PagingRewriter. Or at least it should not.

[bahusoid]

It' just got me thinking that #2480 doesn't fix subqueries with COUNT operator:

[Test(Description = "GH2479")]
public void OrdersWithSubquery9Count()
{
	if (Dialect is MySQLDialect)
		Assert.Ignore("MySQL does not support LIMIT in subqueries.");

	var ordersQuery = db.Orders
	                    .Where(x => x.Employee.EmployeeId > 5)
	                    .OrderByDescending(x => x.OrderId)
	                    .Take(2);

	var orderLinesFuture = db.OrderLines
							.Where(x => ordersQuery.Count(o => o == x.Order) == 2)
							.OrderBy(x => x.Id)
							.ToFuture();

	var orderLines = orderLinesFuture.ToList();
}

Though for some reasons I don't see Count result operator in generated subquery.

[bahusoid]

This thing would not pick up the QueryModel which is processed by PagingRewriter. Or at least it should not.

Not sure what you meant but calling RemoveUnnecessaryBodyOperators right after PagingRewriter does remove ORDER BY without changes applied in 848b915

So in theory we can just call it right from PagingRewriter when we detect that it might be useful instead of copy/paste checks. But for some reasons COUNT for subquery is not present in ResultOperators so it won't fix Count queries

[hazzik]

Though for some reasons I don't see Count result operator in generated subquery.

It is rewrited to NhCountExpression earlier.

[hazzik]

calling RemoveUnnecessaryBodyOperators right after PagingRewriter

But RemoveUnnecessaryBodyOperators needs to be called earlier because some other rewriters could be dependent on the removed queries.

[bahusoid]

But RemoveUnnecessaryBodyOperators needs to be called earlier

I meant calling it second time only when query is converted to Contains subquery. But maybe not as it's anyway doesn't handle Count

[hazzik]

@bahusoid how about that implementation?

Let's deal with count in a separate request.

[bahusoid]

how about that implementation?

I like it