Use right hand type mapping for more binary expressions when left is an object

[ChrisJollyAU]

• I've read the guidelines for contributing and seen the walkthrough
• I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
• The code builds and tests pass locally (also verified by our automated build checks)
• Commit messages follow this format:

        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber

• Tests for the changes have been added (for bug fixes / features)
• Code follows the same patterns and style as existing code in this repo

With a query like Select(x => x != null ? x.Id + "" : null).FirstOrDefault(x => x!.Contains("1")); you have a binary expression of x.Id + "". The left hand side comes through from LINQ without any type mapping and when inferring the type mapping for the result of the binary expression it always looks at the left hand side so you end up with a Type of object and a null type mapping for the expression

With the order of the Select it ends up trying to take the type mapping of x.Id + "" for the result of the CASE. When you then do a StartsWith, EndsWith. Contains all those are translated as a LIKE and no type mapping is changed.

It works with the equals Select(x => x != null ? x.Id + "" : null).FirstOrDefault(x => x == "1"); as that code does look at the right hand side if the left is an object.

See line 185 in SqlExpressionFactory

inferredTypeMapping = ExpressionExtensions.InferTypeMapping(left, right)
    // We avoid object here since the result does not get typeMapping from outside.
    ?? _typeMappingSource.FindMapping(
        left.Type != typeof(object) ? left.Type : right.Type,
        Dependencies.Model);

When you flip the Select around (Select(x => x == null ? null : x.Id + "").FirstOrDefault(x => x!.Contains("1"));) it works because the null result with type mapping of string is used for the result of the CASE and the type mapping is applied on all expressions

This change adds the same type of logic elsewhere in the ApplyTypeMappingOnSqlBinary for the Add, Subtract etc operations

Tests added in AdHocMiscellaneousQueryTestBase.

• Is there a better test class to put it in?
• Currently only overriding it in SQL Server to assert the sql - sqlite only uses base
• Cosmos?

Fixes #34618

[maumar]

@ChrisJollyAU sorry for delayed response. Changes look good. Wrt testing, we try to put straightforward queries into classes that use AssertQuery. This way we take advantage of result verification and some other perks. AdHoc should generally be used for tests with complicated mappings (not covered by AssertQuery models) or if there is some other gimmick that is required for the test. There are a lot of those AssertQuery classes and it can sometimes be hard to pick the best place. When in doubt, you can always use GearsOfWar class, which is our dumping ground for all sort of query tests.

When it comes to sql verification, we do require SQL Server, Sqlite is usually very similar to there is no point. If there is some specific sqlite construct or translation differs significantly between sql server and sqlite, it's good idea to add the SQL verification. But we don't require it.

Cosmos doesn't translate this scenario - added translation error verification, so that if in the future the scenario is enabled we will catch it.

Also, I removed some of the tests, there is no need to do all the combinations. It took me way to long to reply, so I made the testing changes, rather than burden you with them.

[maumar]

Thank you for the contribution @ChrisJollyAU !

[ChrisJollyAU]

@maumar Thanks for sorting the tests out. You are right that we probably didn't need all those combinations - especially when things like contains/startswith/endswith all follow the same path (at least in sql server)

Was happy to wait for you to get to it - I expect the current priority was finalizing EFCore 9.

Quick question: Do I need to a backport PR or is this just going to stay on the 10.0 branch?

[roji]

Cosmos doesn't translate this scenario - added translation error verification, so that if in the future the scenario is enabled we will catch it.

@maumar maybe Cosmos doesn't translate this particular query, but isn't the fix as-is relevant to Cosmos as well (whose SqlExpressionFactory was basically a copy-paste of the relational one)? We really should start keeping Cosmos in mind and applying fixes there too, to not have to deal with the same bugs again there later...

[ChrisJollyAU]

@roji It's actually a bit of a deeper problem on Cosmos than just this fix

I installed Cosmos Emulator so I could take a look. It's actually failing on the string concatenation part in the binary expression.

So its this expression: x.OrderID + ""

First of all, at

efcore/src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs

Line 189 in 931a67c

case { Method: var method } when method == ConcatMethodInfo:

it automatically returns the not translated when we have a Concat(object,object) (relational doesn't have this part).

If we remove that and let it continue, we get much further but not quite.

Also in VisitBinary when the left is translated the unary Convert expression is stripped out and just the operand is returned. This operand ends up with type of Int32 as expected for its related OrderId. Note that relational doesn't stript the unary convert out. at this point.

The resultant binary expression then ends up with Type as Int32 and the CosmosTypeMapping has a ClrType also of Int32. Both left and right have this type mapping applied. Bit odd to have an empty string linked to a Int32 in the type mapping

[roji]

@ChrisJollyAU thanks for taking a look... Yeah, the Cosmos provider is definitely behind in many ways - it was basically copy-pasted from relational a long time ago, and not well-maintained since (we did a big push for 9.0 but there's a lot of stuff will remaining). I do have some ideas on how to try to merge more parts of Cosmos and relational so we have less duplication to manage, but in the meantime this situation is precisely why we should try to apply fixes to both relational and Cosmos, to keep them from diverging even further.

Concretely, it sounds like a new issue is needed for the string concatenation problem you ran into. However, the specific fix in this PR seems correct regardless of whether it actually fixes any specific bugs/tests (hopefully it doesn't break any...). So I'd still port the fix across, unless you think there's some reason not to?

[ChrisJollyAU]

Sure, we can apply this fix. It's just a 1 liner.

Quick run through locally doesn't show any that it breaks. (I do have some in JsonQueryCosmosTest that fail on a JsonSerializationException but that doesn't look like anything to do with this error and the tests have the [SkipOnCiCondition] attribute.

First time I'm looking at Cosmos and did find myself wondering why things like the SqlExpressionFactory and others are all on their own separate to the relational ones. Would be nice if the common things were all together and not duplicated

[ChrisJollyAU]

Definitely new issue. Note that test like Concat_string_int passes for Cosmos because it is in the top level projection which falls back to client-side concatenation. Will create a new issue sometime

[roji]

Sure, we can apply this fix. It's just a 1 liner.

Great, thanks! Feel free to submit a PR.

First time I'm looking at Cosmos and did find myself wondering why things like the SqlExpressionFactory and others are all on their own separate to the relational ones. Would be nice if the common things were all together and not duplicated

There are non-trivial reasons for this... For example, in the query pipeline design, SqlExpression represents a scalar and so references a type mapping (which describes the type of that scalar), but type mappings are provider-specific (RelationalTypeMapping vs. CosmosTypeMapping). This causes the need to duplicate SqlExpression, and from there also SqlExpressionFactory.

I definitely agree that we should rethink this design and see how we can get mure reuse across the providers - but that's a deep architectural task that we don't currently have the bandwidth for (there are more urgent deep architectural tasks that would get priority)... So unfortunately for now we need to do our best to keep things more or less aligned, e.g. fixing bugs on both.

[ChrisJollyAU]

but that's a deep architectural task that we don't currently have the bandwidth for (there are more urgent deep architectural tasks that would get priority)... So unfortunately for now we need to do our best to keep things more or less aligned, e.g. fixing bugs on both.

Yeah. that might be a whole feature release on its own