Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize away Coalesce for trivial cases #34002

Merged
merged 4 commits into from
Jun 24, 2024

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented Jun 16, 2024

This changeset contains:

  • improvements to the test suite that ensure that COALESCE is not optimized away where it is the actual target of the test
  • a change to the ISqlExpressionFactory interface that grants its members more flexibility on the instance type of the returned expressions; even though it is technically a breaking change, this only has impact on providers (and even then, the expected impact is limited, as seen in the changes required on the SQLite and SqlServer providers)
  • an improvement to the SqlExpressionFactory.Coalesce implementation that automatically simplifies the expression for trivial cases:
    • COALESCE(null, x) -> x
    • COALESCE(nonNullableConstant, x) -> nonNullableConstant
    • COALESCE(nonNullableColumn, x) -> nonNullableColumn

Contributes to #33890 (the Cosmos part is missing).

@ranma42
Copy link
Contributor Author

ranma42 commented Jun 16, 2024

Note that this change only takes care of trivial optimizations (which is nonetheless convenient to centralize, as shown by the commit that cleans up repeated copies of this pattern).

More general optimizations for COALESCE are being implemented in:

@ranma42
Copy link
Contributor Author

ranma42 commented Jun 17, 2024

we could also move this simplification to the factory, if we want to:

if (extensionExpression is SqlFunctionExpression sqlFunctionExpression
&& IsCoalesce(sqlFunctionExpression))
{
var arguments = new List<SqlExpression>();
foreach (var argument in sqlFunctionExpression.Arguments!)
{
var newArgument = (SqlExpression)Visit(argument);
if (IsCoalesce(newArgument))
{
arguments.AddRange(((SqlFunctionExpression)newArgument).Arguments!);
}
else
{
arguments.Add(newArgument);
}
}
var distinctArguments = arguments.Distinct().ToList();
return distinctArguments.Count > 1
? new SqlFunctionExpression(
sqlFunctionExpression.Name,
distinctArguments,
sqlFunctionExpression.IsNullable,
argumentsPropagateNullability: distinctArguments.Select(_ => false).ToArray(),
sqlFunctionExpression.Type,
sqlFunctionExpression.TypeMapping)
: distinctArguments[0];
}

(aka automatically flatten COALESCE and deduplicate its arguments)

@ranma42
Copy link
Contributor Author

ranma42 commented Jun 18, 2024

The first 2 commits of this PR now also belong to:

hence I am marking this as draft like the other dependent PRs I posted 😇

@ranma42 ranma42 marked this pull request as draft June 18, 2024 12:29
@ranma42 ranma42 marked this pull request as ready for review June 18, 2024 18:45
@cincuranet cincuranet merged commit baaf79e into dotnet:main Jun 24, 2024
7 checks passed
ranma42 added a commit to ranma42/efcore that referenced this pull request Jun 24, 2024
dotnet#33715 and dotnet#34002 were developed concurrently. Their merge does not build because of some changes in the types returned by `ISqlExpressionFactory`.
@ranma42
Copy link
Contributor Author

ranma42 commented Jun 24, 2024

@cincuranet sorry, I did not notice that another change was merged that was not a syntax conflict, but that still resulted in a build issue. I just pushed #34078 to fix this.

@ranma42 ranma42 deleted the generic-exprfactory branch June 24, 2024 15:47
maumar pushed a commit that referenced this pull request Jun 24, 2024
Fix build regression from #34002

#33715 and #34002 were developed concurrently. Their merge does not build because of some changes in the types returned by `ISqlExpressionFactory`.
@ranma42 ranma42 mentioned this pull request Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants