Translate Queryable.Reverse

[smitpatel]

We don't translate for any provider (I thought we did for relational).
We never had this functionality in past so not needed for 3.0

[ajcvickers]

Notes from triage: Reverse is only meaningful if we have a stable order to begin with. In that case the implementation here is to reverse that order. Throw for other cases.

[Pankraty]

Hi all. I've chosen this issue to start getting familiar with the EF sources.

I've drafted the first implementation and found that it is easy to do if .Reverse() is allowed only after one of OrderBy, OrderByDescending, ThenBy, ThenByDescending. In other cases, it would be not that easy.

For example, should this be a valid query?

ss.Set<Employee>()
    .OrderBy(e => e.EmployeeID)
    .Select(e => e.EmployeeID)
    .Reverse();

[smitpatel]

@Pankraty - Thanks for showing interest in contributing for this issue. As per triage decision above, it is fine to throw exception if there is no ordering coming before Reverse. For InMemory it would just work how it works in linq. So nothing complicated needed there either.

The query you posted above is valid query since, there is ordering present before select. Many operators preserves inner ordering, especially when they are not doing any comparison.

For implementation, I would recommended adding some tests (which would of course fail) first since there would be different parts of query pipeline which may need to handle this operator. I would happy to guide you if you are unsure what should happen to this operator in any part.
The main translation would be in the implementation of QueryableMethodTranslatingExpressionVisitor.TranslateReverse method.

[Pankraty]

@smitpatel thanks for offering help! Before you wrote I prepared this implementation (#19185) but tomorow I will try to do as you've suggested.

[Davilink]

IMO, i don't think it's a good idea for to implement the .Reverse() method.

What do i expect from that:

// case #1
_context.Set<Customer>()
                .AsQueryable()
                .OrderBy(c => c.Name)
                    .ThenByDescending(c => c.BillingAccountNumber)
                    .ThenBy(c => c.CustomerSignatoryEmail)
                    .Reverse();

// case #2
_context.Set<Customer>()
                .AsQueryable()
                .OrderBy(c => c.Name)
                    .ThenByDescending(c => c.BillingAccountNumber)
                    .Reverse()
                    .OrderByDescending(c => c.CustomerSignatoryEmail)
                    .Reverse();

The OrderBy should be self-explanatory.
I don't think it's a good practice to reverse the order neither should be encouraged.

Notes from triage: Reverse is only meaningful if we have a stable order to begin with. In that case the implementation here is to reverse that order. Throw for other cases.

That the perfect scenario for some dev to not understand why the code work sometime and note in other scenario. Even if the functionality is documented, many developers will not have the reflex to check it.

I wonder why do we need this functionality ?

[smitpatel]

@Davilink - Both of the case you posted are different LINQ queries and will generate different results. Reverse affects ordering of everything not just last. And doing an OrderBy on an ordered enumerable will make existing ordering secondary and use the last OrderBy as primary. I would suggest reading up documentation of OrderBy/ThenBy in LINQ enumerables.

As for implementation or why we added this functionality:
Queryable.Reverse is defined by LINQ in runtime. EF Core is just merely translating it here to server rather than throwing client eval exception. EF Core does not control what is defined in LINQ. If you believe that Reverse should not exist then please file an issue over https://github.com/dotnet/runtime repository. If Queryable.Reverse did not exist then we may not have added this in EF Core. Don't kill the messenger.

[Davilink]

@smitpatel Hi, i'm sorry if my message seem to attack someone, it wasn't my intention. English isn't my first language. I was only indicating my concern with the move forward of this implementation.

[smitpatel]

@Davilink - Sorry, I did not mean to indicate any attack. I was just indicating that EF Core is just a bridge to translate something already existing in C# to the database.