Simplify `COALESCE` based on the nullability of its arguments #33938

ranma42 · 2024-06-09T10:49:55Z

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

roji

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'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).

/cc @maumar

roji · 2024-06-09T11:04:21Z

src/EFCore.SqlServer/Query/Internal/Translators/SqlServerStringAggregateMethodTranslator.cs

@@ -105,7 +105,7 @@ public class SqlServerStringAggregateMethodTranslator : IAggregateMethodCallTran
                    source,
                    enumerableArgumentIndex: 0,
                    nullable: true,
-                    argumentsPropagateNullability: new[] { false, true },
+                    argumentsPropagateNullability: new[] { false, false },


Thanks, good catch.

ranma42 · 2024-06-09T11:41:51Z

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 #33889 (which would require more invasive changes).

roji · 2024-06-09T12:21:34Z

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 #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 · 2024-06-09T12:22:04Z

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

maumar · 2024-06-11T05:31:17Z

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

@maumar

This change aims at ensuring that the expressions within the queries being tested cannot be trivially optimized away. Base on the suggestion by @maumar in dotnet#33938 (comment)

ranma42 · 2024-06-17T18:20:55Z

@roji I implemented the SqlExpressionFactory changes in #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).

@maumar

This change aims at ensuring that the expressions within the queries being tested cannot be trivially optimized away. Based on the suggestion by @maumar in dotnet#33938 (comment)

ranma42 · 2024-06-24T17:45:28Z

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 · 2024-06-27T05:46:42Z

Rebased to resolve merge conflict

roji

Looks great as always, thanks. See two minor suggestions below.

roji · 2024-06-27T06:42:28Z

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

+            if (coalesceArguments.Count == 1)
+            {
+                return coalesceArguments[0];
+            }
+            else if (coalesceArguments.Count == sqlFunctionExpression.Arguments.Count)
+            {
+                return sqlFunctionExpression.Update(sqlFunctionExpression.Instance, coalesceArguments);
+            }
+            else
+            {
+                return _sqlExpressionFactory.Function(
+                    sqlFunctionExpression.Name,
+                    coalesceArguments,
+                    sqlFunctionExpression.IsNullable,
+                    argumentsPropagateNullability: coalesceArguments.Select(_ => false).ToArray(),
+                    sqlFunctionExpression.Type,
+                    sqlFunctionExpression.TypeMapping);
+            }


nit: I'm a big fan of switches (especially expressions):

Suggested change

if (coalesceArguments.Count == 1)

{

return coalesceArguments[0];

}

else if (coalesceArguments.Count == sqlFunctionExpression.Arguments.Count)

{

return sqlFunctionExpression.Update(sqlFunctionExpression.Instance, coalesceArguments);

}

else

{

return _sqlExpressionFactory.Function(

sqlFunctionExpression.Name,

coalesceArguments,

sqlFunctionExpression.IsNullable,

argumentsPropagateNullability: coalesceArguments.Select(_ => false).ToArray(),

sqlFunctionExpression.Type,

sqlFunctionExpression.TypeMapping);

}

return coalesceArguments switch

{

[var singleArgument] => singleArgument,

_ when coalesceArguments.Count == sqlFunctionExpression.Arguments.Count

=> sqlFunctionExpression.Update(sqlFunctionExpression.Instance, coalesceArguments),

_ => _sqlExpressionFactory.Function(

sqlFunctionExpression.Name,

coalesceArguments,

sqlFunctionExpression.IsNullable,

argumentsPropagateNullability: coalesceArguments.Select(_ => false).ToArray(),

sqlFunctionExpression.Type,

sqlFunctionExpression.TypeMapping)

};

roji · 2024-06-27T06:48:39Z

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

+            }
+            else
+            {
+                return _sqlExpressionFactory.Function(


I think it makes sense to just have SqlFunctionExpression.Update accept an optional argumentsPropagateNullability argument

roji

One last tiny nit...

roji · 2024-06-27T07:57:07Z

src/EFCore.Relational/Query/SqlExpressions/SqlFunctionExpression.cs

+        SqlExpression? instance,
+        IReadOnlyList<SqlExpression>? arguments,
+        IReadOnlyList<bool>? argumentsPropagateNullability = null)
+        => instance != Instance


We should do a pass to invert the logic of all of these across all expression types at some point (I hate negation + else)

as we are handling this, should we also check that the arguments make sense?

Currently foo.Update(foo.Instance, null) always returns foo (which might not be intended).
Similarly, if bar is niladic (aka bar.ArgumentsPropagateNullability is null), bar.Update(bar.Instance, [_factory.Constant(3)]) returns bar (arguments is ignored).

We could prevent switching from/to niladic, but I am unsure if this is a check we care about (it is an internal API).

roji · 2024-06-27T07:57:55Z

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

            }

            nullable = coalesceNullable;

-            return sqlFunctionExpression.Update(sqlFunctionExpression.Instance, coalesceArguments);
+            return coalesceArguments switch


nit: now that this has only two arms, it can be a conditional operator

roji · 2024-06-27T09:33:34Z

src/EFCore.Relational/Query/SqlExpressions/SqlFunctionExpression.cs

+    {
+        argumentsPropagateNullability ??= ArgumentsPropagateNullability;
+
+        if (IsNiladic)


Shouldn't these checks be in the constructor?

We could add them to the constructor if we want to, but they would not be sufficient to capture the misuse as mentioned in #33938 (comment)

Specifically, if either arguments or Arguments is null, the code would skip the constructor and just return this.

@roji what is the best way forward? should I add checks both to the constructor and here?

Thanks for the ping.

I took another look... I'm not sure why SqlFunctionExpression has a separate IsNiladic property (I've been wanting to take a look at this for a while): the arguments being null (as opposed to empty) seems like the natural way to express a niladic function, and it indeed seems like whenever niladic is true, arguments is null and vice versa.

So unless I'm mistaken, we can probably just remove the IsNliadic property as an actual property (i.e. just make it return Arguments is not null). At that point, it's fine for Update() to switch to/from niladic - that's just an operation to change Arguments from null to non-null etc. And in any case argumentsPropagateNullability should correspond to arguments (both in nullness and in number of arguments when not null).

How does that sound?

I especially like defining IsNiladic from arguments (and we can also annotate it so that the compiler knows about their relation).

I am not sure if it makes sense to ever switch between niladic (FOOBAR) and non-niladic (FOOBAR(), FOOBAR(1, 2, 3), ...), but I guess it should not hurt (well, as long as the caller knows what it is doing 😅 ).

Yeah, I mean, if they can use Update() to switch between 1 to two arguments, or even to zero, I don't see why they shouldn't also switch to niladic... It's on them to get it right...

Is this something you want to do as part of this PR? Or separately? If you prefer I can submit a PR for the niladic change.

src/EFCore.Relational/Query/SqlExpressions/SqlFunctionExpression.cs

roji · 2024-07-01T10:18:19Z

src/EFCore.Relational/Query/SqlExpressions/SqlFunctionExpression.cs

+
+        return instance == Instance
+            && ((arguments == Arguments) || (arguments != null && Arguments != null && arguments.SequenceEqual(Arguments)))
+            && ((argumentsPropagateNullability == ArgumentsPropagateNullability)


OK, the problem this creates is that argumentsPropagateNullability being null could mean two things: either the user is switching to niladic (and so arguments is null too), or they want to just change the arguments without changing argumentsPropagateNullability (which seems like a common scenario).

Maybe we should just have two overloads, one only for changing arguments (doesn't have argumentsPropagateNullability parameter, and importantly, cannot change the number of arguments), and another with arguments and argumentsPropagateNullability?

I'll do that (it was indeed my initial approach).
Note that the "common" scenario occurs just 3 times in the codebase (there are only 4 uses of this method)

Redefined it based on `Arguments`, consistent with its annotation.

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

roji

Thanks @ranma42!

ranma42 mentioned this pull request Jun 9, 2024

Optimize away Coalesce over non-nullable expression #33890

Open

roji reviewed Jun 9, 2024

View reviewed changes

roji self-assigned this Jun 9, 2024

ranma42 mentioned this pull request Jun 9, 2024

Fix Nullable<bool>.ToString() conversion #33940

Merged

ranma42 mentioned this pull request Jun 16, 2024

Optimize away Coalesce for trivial cases #34002

Merged

ranma42 force-pushed the simplify-coalesce branch from fea7752 to 8b16233 Compare June 17, 2024 18:10

ranma42 force-pushed the simplify-coalesce branch from 8b16233 to a4e7fe3 Compare June 17, 2024 19:00

ranma42 mentioned this pull request Jun 17, 2024

Fix Nullable<T>.ToString() #34014

Merged

ranma42 force-pushed the simplify-coalesce branch 2 times, most recently from be230cd to 473eb2e Compare June 24, 2024 15:21

ranma42 marked this pull request as ready for review June 24, 2024 17:45

ranma42 force-pushed the simplify-coalesce branch from 473eb2e to 43ca30d Compare June 27, 2024 05:46

roji approved these changes Jun 27, 2024

View reviewed changes

ranma42 force-pushed the simplify-coalesce branch 2 times, most recently from 8d6a473 to 3387737 Compare June 27, 2024 07:46

roji reviewed Jun 27, 2024

View reviewed changes

ranma42 force-pushed the simplify-coalesce branch from 3387737 to 1f18de8 Compare June 27, 2024 09:22

roji reviewed Jun 27, 2024

View reviewed changes

Fix nullability of group_concat and STRING_AGG

1d44c17

ranma42 force-pushed the simplify-coalesce branch from 1f18de8 to e7705b9 Compare July 1, 2024 10:00

roji reviewed Jul 1, 2024

View reviewed changes

src/EFCore.Relational/Query/SqlExpressions/SqlFunctionExpression.cs Show resolved Hide resolved

roji reviewed Jul 1, 2024

View reviewed changes

ranma42 force-pushed the simplify-coalesce branch from e7705b9 to 1a76432 Compare July 1, 2024 10:25

ranma42 added 5 commits July 1, 2024 12:27

Simplify SqlFunctionExpression.IsNiladic

91c269a

Redefined it based on `Arguments`, consistent with its annotation.

Add support for changing arity in SqlFunctionExpression.Update

88f596a

Add test for COALESCE short-circuiting

f152204

Simplify COALESCE

8861ada

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

Update test baselines

6a3488a

ranma42 force-pushed the simplify-coalesce branch from 1a76432 to 6a3488a Compare July 1, 2024 10:27

ranma42 requested a review from roji July 2, 2024 18:42

roji approved these changes Jul 2, 2024

View reviewed changes

roji merged commit 798fbc8 into dotnet:main Jul 2, 2024
7 checks passed

ranma42 deleted the simplify-coalesce branch July 2, 2024 21:53

roji added the community-contribution label Jul 3, 2024

roji mentioned this pull request Jul 3, 2024

[9.0] New translations and SQL optimization/simplification #34151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify `COALESCE` based on the nullability of its arguments #33938

Simplify `COALESCE` based on the nullability of its arguments #33938

ranma42 commented Jun 9, 2024

roji left a comment

roji Jun 9, 2024

ranma42 commented Jun 9, 2024

roji commented Jun 9, 2024

roji commented Jun 9, 2024

maumar commented Jun 11, 2024

ranma42 commented Jun 17, 2024

ranma42 commented Jun 24, 2024

ranma42 commented Jun 27, 2024

roji left a comment

roji Jun 27, 2024

roji Jun 27, 2024

roji left a comment

roji Jun 27, 2024

ranma42 Jun 27, 2024 •

edited

Loading

roji Jun 27, 2024

roji Jun 27, 2024

ranma42 Jun 27, 2024

ranma42 Jul 1, 2024

roji Jul 1, 2024 •

edited

Loading

ranma42 Jul 1, 2024

roji Jul 1, 2024

roji Jul 1, 2024

ranma42 Jul 1, 2024

roji left a comment

Simplify COALESCE based on the nullability of its arguments #33938

Simplify COALESCE based on the nullability of its arguments #33938

Conversation

ranma42 commented Jun 9, 2024

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranma42 commented Jun 9, 2024

roji commented Jun 9, 2024

roji commented Jun 9, 2024

maumar commented Jun 11, 2024

ranma42 commented Jun 17, 2024

ranma42 commented Jun 24, 2024

ranma42 commented Jun 27, 2024

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranma42 Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji left a comment

Choose a reason for hiding this comment

Simplify `COALESCE` based on the nullability of its arguments #33938

Simplify `COALESCE` based on the nullability of its arguments #33938

ranma42 Jun 27, 2024 •

edited

Loading

roji Jul 1, 2024 •

edited

Loading