Fix to #12657 - Query: logic protecting SUM from returning null doesn't work for complex scenarios #21203
Conversation
| @@ -918,8 +918,13 @@ SqlExpression AddNullConcatenationProtection(SqlExpression argument, RelationalT | |||
| arguments[i] = Visit(sqlFunctionExpression.Arguments[i], out _); | |||
| } | |||
|
|
|||
|
|
|||
| return sqlFunctionExpression.Update(instance, arguments); | |||
| return sqlFunctionExpression.IsBuiltIn | |||
There was a problem hiding this comment.
I'm on the fence here - we do provide Sum translation into SqlFunction "SUM" in relational, but if there is a provider that translates it to a different function it would have to rewrite the entire visit sql function method, so that SUM doesn't get coalesce wrapped around it incorrectly. thoughts? @roji @smitpatel
There was a problem hiding this comment.
For best solution it would be good to put at translation of sum but there is going to be some refactoring coming in when implementing more group by operation and #20633 may be needed for it.
I think this is ok compromise for now, it may not be the best solution but avoids wrong results for customers.
There was a problem hiding this comment.
Of a provider translates to different function, wouldn't they just be responsible for applying the same coalscing in their own nullability processor?
There was a problem hiding this comment.
yeah, its just harder than it needs to be (do the translation + call base vs rewrite entire visit sql function method) but maybe its fine
test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs
Show resolved
Hide resolved
|
updated |
…'t work for complex scenarios Adding COALESCE around SUM sql function expression Fixes #12657
Adding COALESCE around SUM sql function expression
Fixes #12657