FunctionPreprocessingExpressionVisitor causes early assignment of default type mapping

[roji]

While investigating an issue with case-insensitive StartsWith in Npgsql (npgsql/efcore.pg#388), I ran into some problematic behavior.

In PostgreSQL, case-sensitive strings are represented as a special store type (citext). In order to have proper case-insensitive comparisons, both sides of a comparison expression must be properly typed as citext (SELECT CAST('HELLO' AS CITEXT) = CAST('hello' AS TEXT) is false). Now, when the pattern given to StartsWith is a parameter, my string method translator does proper type inference and applies the citext type mapping.

However, FunctionPreprocessingExpressionVisitor detects StartsWith/EndsWith, and introduces a check for whether the pattern is an empty string. Since there is no type inference here, the parameter expression will get whatever the default is for the CLR type (so it's text and not citext). And since this is the first occurrence of the parameter expression encountered by QuerySqlGenerator, the parameter gets configured as a text parameter instead of a citext parameter.

An easy fix for this would be to simply switch the order in the inserted conjunection (i.e. add the empty string check on the right side instead of on the left side). However, I'm wondering why we need this very special casing of StartsWith/EndsWith in a preprocessor rather than handling this logic inside the method translator, as we always do... I can see @maumar introduced this in dbfa82a for null semantics, but maybe we can clean/simplify this.

In the meantime I'll work around the issue by adding SQL casts back to citext on the parameter.

[smitpatel]

I am closing this as not needed because the visitor is not main cause of the issue. The main cause of the issue is same parameter with multiple different type mappings. Filed #19503 to track it.

• The reason for adding such check has been null semantics and discussed many times. If required, @maumar can explain again. Currently we don't have any other better way to deal with the issue. If we ever find a solution we can look into it. As of now the visitor has a purpose and doing correct work.
• Order of inserted conjunction may solve this particular issue but defeats the purpose why this visitor was added in first place. (short-circuiting behavior). Hence it is not possible to change.
• Regardless of order is changed the underlying issue Query: Deal with same querycontext parameter having multiple inferred type mapping #19503 remains. So there is no value in making any change and causing regressions.

[roji]

If required, @maumar can explain again.

I must have missed it - can one of you guys quickly explain? This is also a good example of why a sentence or two in the XML docs could help (i.e. #19415).

Order of inserted conjunction may solve this particular issue but defeats the purpose why this visitor was added in first place. (short-circuiting behavior).

Remember SQL doesn't guarantee evaluation order unlike C#... Are we sure this is really relevant?

Regardless of order is changed the underlying issue #19503 remains. So there is no value in making any change and causing regressions.

It's true that #19503 remains regardless of this. Another potential fix to this is #17598 which would obviate this (since we could generate different SQL for empty-space parameters).

[smitpatel]

I am not referring to short circuiting behavior of SQL, it is of null semantics. So yes, order of terms this visitor adds is highly relevant and necessary otherwise it causes incorrect results.
#17598 has no effect on this. This visitor is not about empty space parameters.

[roji]

I guess I'll wait for the full explanation then.

[maumar]

Function preprocessor is mainly to add null checks before the translation. The code that compares parameters to empty strings can be moved to translation.
Reason that we need null checks is so that null semantics doesn't kick in after the method goes thru translation (we don't have a way to prevent null semantics running on a subtree - either it runs everywhere or nowhere)

Example of the query that gets affected by null semantics:

from e1 in ss.Set<Level1>()
where e1.OneToOne_Optional_FK1.Name.StartsWith(e1.OneToOne_Optional_FK1.Name)

when preprocessor injects null check we produce the following sql:

SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
FROM [LevelOne] AS [l]
LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Optional_Id]
WHERE ([l0].[Name] = N'') OR ([l0].[Name] IS NOT NULL AND (LEFT([l0].[Name], LEN([l0].[Name])) = [l0].[Name]))

and when it doesnt (and null semantics runs):

SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
FROM [LevelOne] AS [l]
LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Optional_Id]
WHERE ([l0].[Name] = N'') OR ((LEFT([l0].[Name], LEN([l0].[Name])) = [l0].[Name]) OR (LEFT([l0].[Name], LEN([l0].[Name])) IS NULL AND [l0].[Name] IS NULL))

The predicate in second query returns true when both sides are null (e.g. when the navigation is null, so we propagate null value to it's Name property).

Similar thing happens (and we have similar mechanism of adding null check) for collection navigations

entity.Select(e => e.Navigation.Collection)

would get translated to:

entity.Select(e => (from ce in CollectionElements
                              where ce.FK == e.Navigation.PK         
                              select ce)

if so happens that ce.FK is null and the e.Navigation is null (therefore the null value propagates to PK) - null semantics would rewrite the query in such a way that those elements would get returned. This is not what we want (only want to create collections for navigations that are actually not null) - so we add a similar null check:

entity.Select(e => (from ce in CollectionElements
                              where e.Navigation.PK != null && ce.FK == e.Navigation.PK         
                              select ce)

Now, the reason why we can't add those null checks as part of the function translation is (as always, when it comes to null semantics) the negated case.

For negated case we add the null checks just the same:

normal case:

a.StartsWith(b) -> a != null && b != null && a.StartsWith(b)

negated case:

!a.StartsWith(b) -> a != null && b != null && !a.StartsWith(b)

That's why we override VisitUnary and handle the case separately.
If we added those checks as part of regular translation, it would have been done at the method level, so it would probably look like this:

!a.StartsWith(b) -> !(a != null && b != null && a.StartsWith(b)) --de morgan-> a == null || b == null || !a.StartsWith(b)

Which is not what we want :(

[smitpatel]

@roji - Please check if this is issue any further once #19503 is merged.