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.

Sign up for GitHub

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

Jump to bottom

[Q] SqlNullabilityProcessor.VisitSqlFunction, Nullability of SUM function #26357

Closed
dmitry-lipetsk opened this issue Oct 14, 2021 · 1 comment · Fixed by #26401
Closed

[Q] SqlNullabilityProcessor.VisitSqlFunction, Nullability of SUM function #26357

dmitry-lipetsk opened this issue Oct 14, 2021 · 1 comment · Fixed by #26401
Assignees
@maumar
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone
7.0.0

Comments

@dmitry-lipetsk
Copy link
Contributor

Hello @maumar,

In case of "SUM" function, VisitSqlFunction returns not-nullable expression COALSECE(0, SUM(....)).

But does not set out-variable nullable to false

efcore/src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

Lines 1048 to 1067 in 07d281b

nullable = sqlFunctionExpression.IsNullable;
if (sqlFunctionExpression.IsNiladic)
{
return sqlFunctionExpression.Update(instance, sqlFunctionExpression.Arguments);
}
var arguments = new SqlExpression[sqlFunctionExpression.Arguments.Count];
for (var i = 0; i < arguments.Length; i++)
{
arguments[i] = Visit(sqlFunctionExpression.Arguments[i], out _);
}
return sqlFunctionExpression.IsBuiltIn
&& string.Equals(sqlFunctionExpression.Name, "SUM", StringComparison.OrdinalIgnoreCase)
? _sqlExpressionFactory.Coalesce(
sqlFunctionExpression.Update(instance, arguments),
_sqlExpressionFactory.Constant(0, sqlFunctionExpression.TypeMapping),
sqlFunctionExpression.TypeMapping)
: sqlFunctionExpression.Update(instance, arguments);

It is correct?

Thank you in advance for your response.
@maumar maumar self-assigned this Oct 15, 2021
@maumar
Copy link
Contributor

maumar commented Oct 15, 2021

@dmitry-lipetsk good catch!

@ajcvickers ajcvickers added this to the 7.0.0 milestone Oct 15, 2021
maumar added a commit that referenced this issue Oct 18, 2021
@maumar
Fix to #26357 - [Q] SqlNullabilityProcessor.VisitSqlFunction, Nullabi…
8f70a1e 
…lity of SUM function

Small optimization, Nullability processor was already doing the right thing during expand null semantics step (since it was evaluating nullability of the COALESCE(sum, 0) which is never null), but now we are bit more efficient. We mark the SUM function as non nullable earlier so we never need to compute its nullability during the later step.

Fixes #26357
@maumar maumar mentioned this issue Oct 18, 2021
Merged
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 18, 2021
@maumar maumar closed this as completed in 7ccb53a Oct 19, 2021
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview1 Feb 14, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview1, 7.0.0 Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maumar maumar

Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Milestone
7.0.0
3 participants
@ajcvickers @maumar @dmitry-lipetsk