ExecuteDelete: Generate correct Contains method when query has unsupported operators #28755
Conversation
|
worked-around |
| { | ||
| protected override string StoreName => "NonSharedModelBulkUpdatesTests"; | ||
|
|
||
| #nullable enable |
There was a problem hiding this comment.
No problem with this, but any particular reason to turn this on for this particular test? I hope to do a pass and turn on NRTs on at least some of our tests.
There was a problem hiding this comment.
The original repro had it, in order to avoid any unrelated differences from repro to creep in and potentially making the test pass even though the original issue wasn't fixed, just added it. I think it is ok to add nullable when required in tests especially query tests because LINQ loves NRTs.
| @@ -1082,7 +1082,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp | |||
| if (pk.Properties.Count == 1) | |||
| { | |||
| predicateBody = Expression.Call( | |||
| QueryableMethods.Contains.MakeGenericMethod(clrType), source, entityParameter); | |||
| EnumerableMethods.Contains.MakeGenericMethod(clrType), source, entityParameter); | |||
There was a problem hiding this comment.
BTW, if didn't have the special condition for pk.Property.Count == 1, and just went through the flow below (constructing AnyWithPredicate instead of Contains), wouldn't that still get picked up as entity equality and do the right thing? In general, I thought we treat blogs.Contains(blog) and blogs.Any(b => b == blog) in the same way.
(not blocking for merging this, just a question)
There was a problem hiding this comment.
They generate different SQL. But more-or-less same output. But this gives a good idea for this PR that, we can just fallback to Any code path for now and improve this later (post rc1)
| @@ -1082,7 +1082,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp | |||
| if (pk.Properties.Count == 1) | |||
| { | |||
| predicateBody = Expression.Call( | |||
| QueryableMethods.Contains.MakeGenericMethod(clrType), source, entityParameter); | |||
| EnumerableMethods.Contains.MakeGenericMethod(clrType), source, entityParameter); | |||
There was a problem hiding this comment.
Is it OK that here we use EnumerableMethods, but below with AnyWithPredicate we still use QueryableMethods?
There was a problem hiding this comment.
Yes. We don't translate Enumerable.Any.
Contains both variations can be translated. Though only enumerable code path deals with entity equality since it is inside sqltranslator.
Work-around for #28745
smitpatel
force-pushed
the
smit/executebugs1
branch
from
332ee5a
to
07a9ddd
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ajcvickers
changed the title
Resolves #28745