Entity equality support for non-extension Contains

[roji]

EE handled the extension version of {IQueryable,IEnumerable}.Contains,
but not instance methods such as List.Contains.

Fixes #15554, continues work from #16134.

There's a slight complication with the translation of orders.Where(o => customers.Any(c => c.Orders.Contains(o))) (test Where_contains_on_navigation).

As this wasn't handled by EE, nav expansion rewrote this to:

SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Orders] AS [o]
WHERE EXISTS (
    SELECT 1
    FROM [Customers] AS [c]
    WHERE EXISTS (
        SELECT 1
        FROM [Orders] AS [o0]
        WHERE (([c].[CustomerID] = [o0].[CustomerID]) AND [o0].[CustomerID] IS NOT NULL) AND ([o0].[OrderID] = [o].[OrderID])))

With this PR, the translation becomes:

SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Orders] AS [o]
WHERE EXISTS (
    SELECT 1
    FROM [Customers] AS [c]
    WHERE [o].[OrderID] IN (
        SELECT [o0].[OrderID]
        FROM [Orders] AS [o0]
        WHERE ([c].[CustomerID] = [o0].[CustomerID]) AND [o0].[CustomerID] IS NOT NULL
    )
)

Which seems fine. However, EE can't handle composite keys, so that case is specifically left for nav expansion to handle (usually EE throws an informative exception instead). See test Where_contains_on_navigation_with_composite_keys.

[smitpatel]

Which seems fine. However, EE can't handled composite keys, so that case is specifically left for nav expansion to handle (usually EE throws an informative exception instead). See test Where_contains_on_navigation_with_composite_keys.

Navigation expansion does not care about keys. It is not expansion of navigation. EE should rewrite to whatever appropriate query if any.

Hold off on this change for now. It is correct fix but if we are taking care of Queryable.Contains already then my changes in new nav expansion should fix it as it includes changes to make all translatable methods to Queryable ones. So c.Orders.Contains(o) would be re-written to c.Orders.AsQueryable().Contains(o)

[roji]

Navigation expansion does not care about keys. It is not expansion of navigation. EE should rewrite to whatever appropriate query if any.

Sure - the point is only that currently nav expansion also currently rewrites Any(Contains) without needing to care about keys or translate to them (with SQL EXISTS). The translation is different but both seem equally valid (see the SQL samples I posted above from test Where_contains_on_navigation). And the one advantage the current nav expansion mapping has, is that it works on composite keys.

Hold off on this change for now. It is correct fix but if we are taking care of Queryable.Contains already then my changes in new nav expansion should fix it as it includes changes to make all translatable methods to Queryable ones. So c.Orders.Contains(o) would be re-written to c.Orders.AsQueryable().Contains(o)

No problem, that would certainly simplify things in EE. Keep in mind also the (somewhat contrived) scenario where the Contains list isn't an entity type:

customers.Where(c => new List<Customer> { new Customer { CustomerID = "XXX" } }.Contains(c))

Are we planning for the constant list here to also have AsQueryable()? If so then great, otherwise we'd still need some special handling in EE.

[smitpatel]

the point is only that currently nav expansion also currently rewrites Any(Contains) without needing to care about keys or translate to them (with SQL EXISTS).

Is it actually task for expanding a navigation though? If it is not expanding navigation then it does not belong to nav expansion. Could be different visitor if not EE. (EE seem to deal with converting entities to key access where suitable)

List contains - true. Add some tests

[smitpatel]

I missed a bit on list example. You would need to add another runtime parameter or a new constant which evaluates the List.Select. Enumerable methods are not task for translation pipeline.

[roji]

the point is only that currently nav expansion also currently rewrites Any(Contains) without needing to care about keys or translate to them (with SQL EXISTS).

Is it actually task for expanding a navigation though? If it is not expanding navigation then it does not belong to nav expansion. Could be different visitor if not EE. (EE seem to deal with converting entities to key access where suitable)

True, definitely could make sense to split it out to a different visitor. And also true that it doesn't feel like it belongs in entity equality - strictly speaking there is no key-based equality comparison.

Concretely, how do you want to deal with this? Is your new implementation of nav rewrite going to drop the Any(Contains) rewriting? If so, do you want me to write it in a separate visitor?

List contains - true. Add some tests

Will do

I missed a bit on list example. You would need to add another runtime parameter or a new constant which evaluates the List.Select. Enumerable methods are not task for translation pipeline.

Can you explain/give example? I didn't understand this bit...

[smitpatel]

If you just do blind rewrite on this
customers.Where(c => new List<Customer> { new Customer { CustomerID = "XXX" } }.Contains(c))
to
customers.Where(c => new List<Customer> { new Customer { CustomerID = "XXX" } }.Select(c => c.Id).Contains(c.Id))

Then we don't know how to translate constant.Select(c => c.Id) It is enumerable select which does not work on any data source associated with server. If you write the second query yourself outside of EF then The Enumerable select will be processed by ParameterExtractor already. So if you are doing rewrite in compilation phase you have to do the same.

[roji]

Of course, makes sense. But for the constant example we could just evaluate (like we already do for other constant key rewriting in EE), right? The problem would be with a parameterized list, where we'd need to do what you say...

[smitpatel]

Yes.
Constant -> Evaluate
Parameter -> add runtime parameter.
Same thing we do when accessing key when one side was parameter/constant.

[roji]

Makes sense. So just to confirm, this PR still needs to recognize and rewrite List.Contains() - it will not be normalized to IQueryable.Contains()? That means it largely keeps its current form, with the additions discussed.

[roji]

@smitpatel have implemented constant and parameter handling for the source list.

[roji]

@smitpatel rebased this after your changes to nav expansion etc.

Should we have EnumerableMethodProvider just like we have QueryableMethodProvider? The EE Contains logic, for example, needs to wrap non-queryable Lists of keys with Contains, Select, etc. If so I can do that as part of #16300. Otherwise wrapping these with AsQueryable fails in NavigationExpandingEV.VisitMethodCall (NonNavSource), which seems like the way we want it, right?

[smitpatel]

EF Core query pipeline is about translating Methods to server. Only queryble methods can be translated. While certain enumerable method may appear due to collection navigations, we convert them to Queryable methods as once they are expanded it, it would be queryable anyway. So EnumerableMethodProvider does not belong to EF Core. Yes, certain visitor may require to process Enumerable method because of operators working on client side collection. Those visitor should resolve particular methods they need.

Creating generalized class for Enumerable methods passes a wrong message in terms of what we translate. And as @ajcvickers mentioned elsewhere, people would use our library for entirely wrong reason just to get those method resolutions. Hence it is not a surface we want to add. If there many different visitors using many of such methods, we can add pubternal class later.

[smitpatel]

You should not put AsQueryable around something which is not EntityQueryable eventually.
Enumerable.Contains is handled here https://github.com/aspnet/EntityFrameworkCore/blob/release/3.0/src/EFCore.Relational/Query/Internal/ContainsTranslator.cs

[roji]

Thanks for the explanation @smitpatel, all makes sense.

The idea wasn't to add a public EnumerableMethodProvider, just an internal one, to avoid having to look up enumerable LINQ methods in various places, similar to #16504. Searching for typeof(Enumerable) shows quite a few cases where enumerable methods are being looked. Let me know what you think.

In any case, the PR as it is does its own lookup, so it can be reviewed and merged regardless of this question.

[smitpatel]

I did not find enough look ups for enumerable methods which warrants additional class. Only handful of places with very little scope of making it DRY. Also remember .Internal cannot cross assembly boundaries.

[smitpatel]

Make sure build passes. I don't know why everything is red.

[roji]

There were some typing/nullability issues related to #16841 (comment) - I ended up going back to a simpler version where the parameter version is explicitly passed into RegisterRuntimeParameter (instead of simply inferring it from the lambda's return type). I'll revisit as part of #16564.