Support any expression type inside inline primitive collections #31046
Conversation
roji
force-pushed
the
InlineCollectionsWithEverything
branch
2 times, most recently
from
93e05c7
to
de656e7
Compare
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindAggregateOperatorsQuerySqlServerTest.cs
Show resolved
Hide resolved
roji
force-pushed
the
InlineCollectionsWithEverything
branch
from
de656e7
to
1ef5b54
Compare
| @@ -2745,14 +2745,11 @@ SELECT c | |||
|
|
|||
| public override async Task Array_of_parameters_Contains_OrElse_comparison_with_constant_gets_combined_to_one_in(bool async) | |||
| { | |||
| await base.Array_of_parameters_Contains_OrElse_comparison_with_constant_gets_combined_to_one_in(async); | |||
| // #31051 | |||
| await AssertTranslationFailed( | |||
There was a problem hiding this comment.
Note this breaking change: since NewArrayExpression with parameters is no longer client-evaluated in ParameterExtractingEV (into a single parameter with an array-of-constants value), the node reaches translation where it fails translation (NewArrayExpression with all constants is handled).
Opened #30966 to track, and will bring to design.
There was a problem hiding this comment.
This breaking change has been removed by having Cosmos handle NewArrayExpression properly within Contains. However, it still does not opt into query roots, so the whole thing is handled as enumerable, not queryable - #30966 still tracks that work.
|
@maumar @ajcvickers I pushed an additional commit which makes the "Contains over array of parameters" scenario work again on Cosmos, so there's no more breaking change. This also generally improves the Cosmos infrastructure around Contains, in case we want to improve further in the future (e.g. any expression type inside the inline collection, like this PR does for relational). |
| break; | ||
|
|
||
| default: | ||
| throw new ArgumentOutOfRangeException(); |
There was a problem hiding this comment.
I think I like this style over throw new InvalidOperationException("IMPOSSIBLE"); Either way, should we unify across the board?
There was a problem hiding this comment.
Yeah, however... .NET 7.0 introduced UnreachableException, which is exactly the right thing for this case (and many others, e.g. Debug.Fail). We currently only target net6.0 so we can't use it, but we should do a pass once we change TFMs to net8.0 and just switch to that...
Sounds good? I'd be happy to do the pass once we switch TFMs...
| // for each one. | ||
| // non_nullable IN (1, 2, nullable) -> non_nullable IN (1, 2) OR (non_nullable = nullable AND nullable IS NOT NULL) (full) | ||
| // non_nullable IN (1, 2, NULL, nullable) -> non_nullable IN (1, 2) OR (non_nullable = nullable AND nullable IS NOT NULL) (full) | ||
| return nullableValues.Aggregate( |
There was a problem hiding this comment.
@roji now I see why you wanted to get rid of "Negated" from all those expressions. This logic is not easy :D
There was a problem hiding this comment.
Yes, that's exactly how it went :) I started working on this, and then decided it's crazy to have to deal with negation as well...
roji
force-pushed
the
InlineCollectionsWithEverything
branch
from
f3e4c6d
to
641975d
Compare
Closes #30732
Closes #30734