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

Fix parsing of floats in $apply/aggregate/average expression #1811

Conversation

gathogojr
Copy link
Contributor

@gathogojr gathogojr commented Jun 17, 2020

Issues

This pull request fixes issue #1809.

Description

Fixes a bug where ApplyBinder does not cater for float/single for the average aggregate method. This causes the following average expression to be rejected:

$apply=aggregate(SingleProp with average as AverageSingleProp)

while related sum, min, and max are accepted

$apply=aggregate(SingleProp with sum as SumSingleProp)
$apply=aggregate(SingleProp with min as MinSingleProp)
$apply=aggregate(SingleProp with min as MaxSingleProp)

Note: The return value from averaging floats should be a float
https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.average?view=netcore-3.1#System_Linq_Enumerable_Average__1_System_Collections_Generic_IEnumerable___0__System_Func___0_System_Single__
https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.average?view=netcore-3.1#System_Linq_Enumerable_Average__1_System_Collections_Generic_IEnumerable___0__System_Func___0_System_Nullable_System_Single___

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

@gathogojr gathogojr added the Ready for review Use this label if a pull request is ready to be reviewed label Jun 17, 2020
Copy link
Contributor

@KenitoInc KenitoInc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -40,6 +40,9 @@ internal class FakeBindMethods
public static readonly SingleValueNode FakeSingleIntPrimitive =
new ConstantNode(100);

public static readonly SingleValueNode FakeSingleSinglePrimitive =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though a float is EdmPrimitiveTypeKind.Single, Maybe for a better description, you could have used FakeSingleFloatPrimitive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could do that

Copy link
Member

@xuzhg xuzhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link
Contributor

@kosinsky kosinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@gathogojr gathogojr force-pushed the fix/1809-fix-parsing-of-floats-in-apply-average-expression branch from b724cb5 to d14b757 Compare June 18, 2020 19:12
@gathogojr gathogojr merged commit ad7f491 into OData:master Jun 19, 2020
@gathogojr gathogojr deleted the fix/1809-fix-parsing-of-floats-in-apply-average-expression branch June 19, 2020 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review Use this label if a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug affecting $apply/aggregate when using average aggregation method against floats and nullable floats
4 participants