Return null when the type is nullable for Cosmos Max/Min/Average #35173
Conversation
Fixes #35094 This was a regression resulting from the major Cosmos query refactoring that happened in EF9. In EF8, the functions Min, Max, and Average would return null if the return type was nullable or was cast to a nullable when the collection is empty. In EF9, this started throwing, which is correct for non-nullable types, but a regression for nullable types.
There was a problem hiding this comment.
LGTM but see comments/suggestions.
One thought - do we have tests where we do Min/Max/Average over a nullable value type, and where the database actually does contain a null (as opposed to an empty result set)? If not, it might be worth adding those, as especially in Cosmos the returns-null vs. returns-nothing are two very distinct cases.
| @@ -1706,29 +1712,7 @@ private static ShapedQueryExpression AggregateResultShaper( | |||
| var nullableResultType = resultType.MakeNullable(); | |||
| Expression shaper = new ProjectionBindingExpression(source.QueryExpression, new ProjectionMember(), nullableResultType); | |||
|
|
|||
| if (throwOnNullResult) | |||
There was a problem hiding this comment.
I think we have the same code in relational - we should look at removing it there (I'd actually do a quick test run without it to see what happens, maybe it'll shed light for why it might be needed on Cosmos too).
| @@ -444,7 +444,25 @@ private ShapedQueryExpression CreateShapedQueryExpression(SelectExpression selec | |||
| /// </summary> | |||
| protected override ShapedQueryExpression? TranslateAverage(ShapedQueryExpression source, LambdaExpression? selector, Type resultType) | |||
| { | |||
| var selectExpression = (SelectExpression)source.QueryExpression; | |||
| var updatedSource = TranslateAggregateCommon(source, selector, resultType, out var selectExpression); | |||
There was a problem hiding this comment.
Just noting that we should try to keep common code similar/identical between Cosmos and relational, as much as possible... Not sure that it makes sense in this specific case - but something to keep in mind.
| return AggregateResultShaper(updatedSource, projection, resultType); | ||
| } | ||
|
|
||
| private ShapedQueryExpression? TranslateAggregateCommon( |
There was a problem hiding this comment.
nit: move this helper down below where all the private stuff lives?
| @@ -457,10 +475,13 @@ private ShapedQueryExpression CreateShapedQueryExpression(SelectExpression selec | |||
| source = TranslateSelect(source, selector); | |||
| } | |||
|
|
|||
| var projection = (SqlExpression)selectExpression.GetMappedProjection(new ProjectionMember()); | |||
| projection = _sqlExpressionFactory.Function("AVG", new[] { projection }, resultType, _typeMappingSource.FindMapping(resultType)); | |||
| if (resultType.IsNullableType()) | |||
There was a problem hiding this comment.
Am noting that this changes to SingleOrDefault for reference types too, not just for nullable value types (for example, Max() can be used over strings). I think this is correct, but maybe worth ensuring we have tests for these?
| return AggregateResultShaper(updatedSource, projection, resultType); | ||
| } | ||
|
|
||
| private ShapedQueryExpression? TranslateAggregateCommon( |
There was a problem hiding this comment.
Not sure, but it seems that the three operators handled here (Average, Max, Min) have the same code in their Translate* method, aside from the name of the function. If so, this TranslateAggregateCommon can just do everything, and just accept the function name as a string, rather than returning here to Translate, which then checks for null and calls another common AggregateResultShaper() in Translate*...
Fixes #35094
This was a regression resulting from the major Cosmos query refactoring that happened in EF9. In EF8, the functions Min, Max, and Average would return null if the return type was nullable or was cast to a nullable when the collection is empty. In EF9, this started throwing, which is correct for non-nullable types, but a regression for nullable types.