Simplify `COALESCE` based on the nullability of its arguments

[ranma42]

Drop all of the arguments after the first non-nullable sub-expressions. Simplify unary COALESCE to its argument.

[ranma42]

This looks great - there's only the problem I mentioned in #33890:

The main difficulty here is that we have various tests exercising Coalesce functionality, which are implemented over non-nullable columns; the Coalesce node would be stripped away there, and the tests would become useless. These need to be updated.

In other words this reduces our test coverage for coalesce but optimizing it away... Ideally we'd modify the tests to ensure that they actually exercise coalesce etc.

I'll look into adding/updating tests before marking this as ready for review. Currently I am thinking of:

• updating the tests that explicitly want a COALESCE so that it is not optimized away
• adding some tests that check that n-ary COALESCE can be simplified to a COALESCE of a prefix of its arguments or just its first argument (with no COALESCE at all)

I'd also implement the SqlExpressionFactory (optimize at the source) as in #33890 at the same time (though it could be done separately too of course).

We can do that, but SqlExpressionFactory is currently lacking quite a lot of information to optimize as effectively as nullability can only be (trivially) computed for constants and columns, so optimizing in this location would still be useful.

This might change if SqlExpressions were extended with nullability information, as proposed in https://github.com/dotnet/efcore/issues/33889 (which would require more invasive changes).

[roji]

I'll look into adding/updating tests before marking this as ready for review. Currently I am thinking of:

Sounds great. Hopefully there are nullable alternatives in the model for the tests in question:::

We can do that, but SqlExpressionFactory is currently lacking quite a lot of information to optimize as effectively as nullability can only be (trivially) computed for constants and columns, so optimizing in this location would still be useful.

Oh I completely agree - both optimizations make sense for sure. The SqlExpressionFactory really is more of a nice-to-have compared to the one implemented here in SqlNullabilityProcessor. I've updated #33890 to track both these things.

This might change if SqlExpressions were extended with nullability information, as proposed in https://github.com/dotnet/efcore/issues/33889 (which would require more invasive changes).

Yeah, that's definitely a long-term discussion more than anything immediately actionable - we should proceed for now with the current query architecture regardless of that possible direction.

[roji]

Assigning to myself but @maumar definitely let us know what you think etc.

[maumar]

I'll look into adding/updating tests before marking this as ready for review. Currently I am thinking of:

Sounds great. Hopefully there are nullable alternatives in the model for the tests in question:::

I actually don't think we have a "natural" null value in a bool column in gears model. But you can always make one by doing a left join.

CogTag.Gear is optional 1:1 and there is a cog tag without corresponding gear, so

ss.Set<CogTag>().Where(x => (bool?)x.Gear.HasSoulPatch ?? false) will exercise the code path and will be reflected in the results

[ranma42]

@roji I implemented the SqlExpressionFactory changes in https://github.com/dotnet/efcore/pull/34002 and based this PR on top of that (in the first PR I update the test so that they keep checking the appropriate COALESCE handling; in this PR I added tests for the optimization).

[ranma42]

The first commit belongs to #34078 (I rebased on top of that to have a working build). Apart from that, it is ready for review 🚀

[ranma42]

Rebased to resolve merge conflict