Fix to #35024 - Query could not be translated when using a static ICollection/IList field

[maumar]

Problem was that when converting primitive collection to inline query root we were not matching the expression if it was constant wrapped in convert. Fix is to remove convert for the purpose of pattern match for the transformation.

Fixes #35024

[ChrisJollyAU]

@maumar That looks fine.

I did though ask myself where the Convert was actually coming in from.

Turns out it gets added as part of the funcletizer

efcore/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs

Line 1941 in 507e7f1

return constantExpression.Type != returnType

return constantExpression.Type != returnType
            ? Convert(constantExpression, returnType)
            : constantExpression;

LH Type is List`1 and RH Type is IList`1

If you want to prevent the convert you could go something like this

return constantExpression.Type != returnType && !constantExpression.Type.GetInterfaces().Contains(returnType)`

[maumar]

@ChrisJollyAU I'm bit apprehensive about adding (more) reflection to the funcletizer as it is a perf-sensitive component. We'd also need to cover the case of DeriveType : BaseType (e.g. ObservableCollection), so that's some more reflection. Although it would fix the problem at the very source. @roji any thoughts on this?

[ChrisJollyAU]

As a side note just FYI.

The IEnumerable case does also get the Convert added at the same point. Its just that in the QueryableMethodNormalizingExpressionVisitor, in the VistMethodCall function it can get handled. With IEnumerable it calls TryConvertEnumerableToQueryable which strips the convert away. If you have a IList or ICollection (and it is a Contains) it calls TryConvertCollectionContainsToQueryableContains. This does not strip the same type of convert

[roji]

This seems like a correct fix for the specific issue, but I think the general behavior of constantizing "init-only" (code) is problematic... The fact that a (static) member is init-only (readonly) doesn't mean that it's immutable; for example, in the example of the test you just wrote, there's nothing preventing the contents StaticIds from being mutated (even if StaticIds itself is readonly). But the way we currently translate, the contents of the list are captured and cached the first time the query is compiled (because they're constantized).

In other words, I thing we have an unrelated bug here; if that bug is fixed (by no longer constantizing init-only fields), then we'd no longer have the Convert node that is also causing the trouble here.

(note that if users really do want to constantize, they can still do that by introducing EF.Constant()).

What do you think?

[roji]

Is there a specific reason this is in this test class (there doesn't seem to be anything aggregate about the query).

Maybe PrimitiveCollectionsQueryTestBase is a better place...

[maumar]

there is a lot of Contains tests in that class, so just keeping them all together. I don't mind moving them all to PCQTB as I do agree it's better place for them.

[roji]

Not very important obviously... Whatever you prefer

[roji]

I'm bit apprehensive about adding (more) reflection to the funcletizer as it is a perf-sensitive component. We'd also need to cover the case of DeriveType : BaseType (e.g. ObservableCollection), so that's some more reflection. Although it would fix the problem at the very source. @roji any thoughts on this?

Aside from my above comment, @ChrisJollyAU's suggestion makes sense to me - I think simply checking constantExpression.Type.IsAssignableTo(returnType) should be sufficient, and take care of interfaces/subclasses/etc. I think perf here should be OK - the funcletizer is indeed perf-sensitive, but not to the point where we can't allow this kind of check where it's needed (remember we use the LINQ interpreter just below to evaluate parameters, which is much slower). So I'd do this regardless of the removal of init-only constantization (discussed above).

[ChrisJollyAU]

This seems like a correct fix for the specific issue, but I think the general behavior of constantizing "init-only" (code) is problematic... The fact that a (static) member is init-only (readonly) doesn't mean that it's immutable; for example, in the example of the test you just wrote, there's nothing preventing the contents StaticIds from being mutated (even if StaticIds itself is readonly). But the way we currently translate, the contents of the list are captured and cached the first time the query is compiled (because they're constantized).

I did experiment with changing that to be containsCapturedVariable: true. It mostly works with the values as a parameter instead of a constant but there are a couple of interesting tests.

Indexof_with_emptystring

SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE CASE
    WHEN [c].[Region] IS NOT NULL THEN 0
END = 0

new sql

@__Empty_0='' (Size = 15)

SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE CHARINDEX(@__Empty_0, [c].[Region]) - CASE
    WHEN @__Empty_0 = N'' THEN 0
    ELSE 1
END = 0

Note that we seem to lose the optimization for that depends on a literal empty string constant. string.Empty is static readonly

Contains_on_readonly_enumerable

SELECT [w].[Id], [w].[AmmunitionType], [w].[IsAutomatic], [w].[Name], [w].[OwnerFullName], [w].[SynergyWithId]
FROM [Weapons] AS [w]
WHERE [w].[AmmunitionType] = 1

New sql

@___weaponTypes_0='[1]' (Size = 4000)

SELECT [w].[Id], [w].[AmmunitionType], [w].[IsAutomatic], [w].[Name], [w].[OwnerFullName], [w].[SynergyWithId]
FROM [Weapons] AS [w]
WHERE [w].[AmmunitionType] IN (
    SELECT [w0].[value]
    FROM OPENJSON(@___weaponTypes_0) WITH ([value] int '$') AS [w0]
)

Array_cast_to_IEnumerable_Contains_with_constant

SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE [c].[CustomerID] IN (N'ALFKI', N'WRONG')

New sql

@___customers_0='["ALFKI","WRONG"]' (Size = 4000)

SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE [c].[CustomerID] IN (
    SELECT [c0].[value]
    FROM OPENJSON(@___customers_0) WITH ([value] nchar(5) '$') AS [c0]
)

Specially in the above cases it probably makes more sense to constantize.

It also breaks SpatialSqlServerTest.Translators_handle_static_members with Microsoft.Data.SqlClient.SqlException : The incoming tabular data stream (TDS) remote procedure call (RPC) protocol stream is incorrect. Parameter 1 ("@__Empty_0"): The supplied value is not a valid instance of data type geography. Check the source data for invalid values. An example of an invalid value is data of numeric type with scale greater than precision.

[roji]

@ChrisJollyAU thanks for taking a look; it definitely makes sense that some optimizations would be lost due to moving from constantization to parameterization. The problem here is that we can't reliably know when the type in question is fully immutable, which is when it's safe to constantize. We could have a hard-coded list of the common immutable types (string, int, DateTime...) and constantize for those (when they're init-only of course), but constantizing for a type list List<T> (as in the example above) simply seems like a bug...

[ChrisJollyAU]

Off the top of my head, I think the only ones with the problem are those like List, Collection, Queue, HashSet, Dictionary i.e. those that implement IList and ICollection.

But definitely for your basic types that you know are completely immutable (like a readonly string or int) my thoughts are towards keeping the constantizing

but constantizing for a type list List<T> (as in the example above) simply seems like a bug

Is that because of the caching or something like that (because its not in the actual sql thats generated as that works)

[roji]

But definitely for your basic types that you know are completely immutable (like a readonly string or int) my thoughts are towards keeping the constantizing

Yeah, agree.

but constantizing for a type list List (as in the example above) simply seems like a bug

Is that because of the caching or something like that (because its not in the actual sql thats generated as that works)

Apologies - I thought a little bit about this, and I was wrong above.

The problem with constantization of a mutable thing (e.g. List<T>), is that if that list is then actually mutated, we get a cache miss - the funcletizer outputs a different tree. This means that the query gets fully recompiled every time the mutable thing has a different actual value: the recompilation is quite expensive, going through the entire query pipeline. We're basically missing the advantage of having parameters in the first place: that the same query compilation artifacts can be reused without recompilation even though the (parameterized) values change across executions.

So I don't think it's quite a bug, more like a problematic performance problem.