Fixes #3165: UriParser - Aggregation method Average doesn't support Int16 #3168
Conversation
…expresion creation.
|
@microsoft-github-policy-service agree company="TescoSW" |
xuzhg
requested review from
habbes,
mikepizzo,
xuzhg,
ElizabethOkerio and
gathogojr
|
The change looks straightforward, do you mind to add test to cover the changes? |
|
Hello @xuzhg, the test has been added. ;) |
|
@xuzhg Can I have a question? When will this change, after merging into master, be published in the new nuget version? Is it in matter of days or weeks? Also will I have to create PRs into other branches? Thanks |
| case EdmPrimitiveTypeKind.SByte: | ||
| case EdmPrimitiveTypeKind.Byte: | ||
| case EdmPrimitiveTypeKind.Int16: |
There was a problem hiding this comment.
I just realized that the types we support here correspond with to the types supported by the generic Average method supported by the framework https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.average?view=net-9.0. There's no Average method that accepts a collection of Int16.
Furthermore, we'd need to ensure parity with the ASP.NET Core OData library otherwise we'd end up with a bug in the dependent library due to the mappings here:
https://github.com/OData/AspNetCoreOData/blob/0ceeb086e4fd7a6396b4c6e0fd4f123283d55736/src/Microsoft.AspNetCore.OData/Query/ExpressionHelperMethods.cs#L86
We could cast Int16 to Int32 in order to support your scenario, but we need to discuss this more broadly before moving forward with this change.
Finally, I don't think we should add SByte and Byte for now. I think such scenarios are much less likely. Either way, this pull request doesn't add tests for those two additions.
There was a problem hiding this comment.
Alright, I'll wait with the changes to the PR till your response with the result of your discussion. Is that ok?
There was a problem hiding this comment.
And just to add my two cents to this conversation. I do know that .NET's Average method does not contain sufficient override for Int16, but in my opinion thatis just another bug and UriParser should be agnostic to this. Why should a parser determine that having average over Int16 is forbidden? Especially when Sum is supported for Int16 by the parser.
Also, even if the types of sbyte and byte are less common, it still makes sense to average over them.
| [Fact] | ||
| public void BindApplyWithAverageWithInt16PropertyShouldReturnApplyClause() | ||
| { | ||
| IEnumerable<QueryToken> tokens = _parser.ParseApply("aggregate(FavoriteNumber with average as AverageFavoriteNumber)"); |
There was a problem hiding this comment.
FavoriteNumber is defined as UInt16 in the referenced Edm model. I'd advise that we also add a test for Int16 to cover all our bases
|
@davidKolar175 is it possible to an End to End tests. You can add them here: |
Issues
This pull request fixes issue where the problem was that during the parsing of average aggregation function the parser could throw an exception saying that it does not support int16. (Or Byte and SByte for that matter)
Description
Easy fix that required just adding few cases into a switch.
Checklist (Uncheck if it is not completed)
Additional work necessary
None