Implementing virtual property $count #738

athoscouto · 2016-12-30T00:00:19Z

Issues

This pull request creates infrastructure to solve issue #773 of WebAPI project. It also addresses issue #562 of this repo.

Description

In this change I add support to parse into lexical and semantical trees aggregation queries that uses the virtual property $count.

Checklist (Uncheck if it is not completed)

Test cases added
Build and test script passing

msftclas · 2016-12-30T00:00:22Z

Hi @athoscouto, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Athos Couto). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

msftclas · 2016-12-30T00:01:04Z

Hi @athoscouto, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Athos Couto). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

athoscouto · 2017-01-20T17:54:48Z

Hi @LaylaLiu, I couldn't build ODL via build.cmd in any branches. Since all the tests are passing in VS, I think I'm done. Do you want me to address any issues in the code?

LaylaLiu · 2017-01-26T05:45:35Z

src/Microsoft.OData.Core/UriParser/Aggregation/AggregationMethod.cs

+        /// The aggregation method Count.
+        /// Used only internally to represent the virtual property $count.
+        /// </summary>
+        VirtualPropertyCount,


If you take a look at the aggregation doc, you will find that VirtualPropertyCount actually is not a method.

So I think it shouldn't be added here

The doc says it's a virtual property. If I'm not mistaken, ODL does not have any other virtual property implemented. Since I didn't find any guideline to follow about implementing these properties, I created a internal aggregation method count and mapped each call of the virtual property to it. This internal method is not visible to the end user, unlike the other aggregation methods.
I'd be happy to know about better approaches to implement it.

LaylaLiu · 2017-01-26T05:48:07Z

src/Microsoft.OData.Core/UriParser/Aggregation/ApplyBinder.cs

@@ -116,6 +129,7 @@ private IEdmTypeReference CreateAggregateExpressionTypeReference(SingleValueNode
                                    expressionPrimitiveKind));
                    }

+                case AggregationMethod.VirtualPropertyCount:


According to above code, it seems that this line will never be touched, right? If the token is a virtualPropertyCount, the code will always go to line 82 instead of line 87.

Your observation is correct. I added this line to be able to use CreateAggregateExpressionTypeReference for both cases, but ended up not using it in the $count one. I'll correct it.

Moved the logic to handle $count to EndPathBinder. It made the logic simpler and now this line is used.

The change looks good. However, it looks like both CountDistinct and VirtualPropertyCount should return Decimal not Int64. I don't think you should change CountDistinct as part of your change, could you simply add a TODO comment that these should be Decimal instead of Int64? Something like:

// TODO: CountDistinct and $Count should return type Edm.Decimal with Scale="0" and sufficient Precision. return EdmCoreModel.Instance.GetPrimitive(EdmPrimitiveTypeKind.Int64, false);

3.1.3.5 Standard Aggregation Method countdistinct
The result property MUST have type Edm.Decimal with Scale="0" and sufficient Precision.

3.1.5 Virtual Property $count
The result property will have type Edm.Decimal with Scale="0" and sufficient Precision.

Created issue #758 to address problem and will reference it on code.

LaylaLiu · 2017-01-26T05:51:07Z

src/Microsoft.OData.Core/UriParser/Parsers/UriQueryExpressionParser.cs

-            var verb = this.ParseAggregateWith();
+            if (endPathExpression != null && endPathExpression.Identifier == ExpressionConstants.QueryOptionCount)
+            {
+                expression = new EndPathToken("$count virtual property", endPathExpression.NextToken);


the identifier should be the $count

You're right. Done.

robward-ms · 2017-02-07T23:15:14Z

src/Microsoft.OData.Core/UriParser/Parsers/UriQueryExpressionParser.cs

-            var verb = this.ParseAggregateWith();
+            if (endPathExpression != null && endPathExpression.Identifier == ExpressionConstants.QueryOptionCount)
+            {
+                expression = new EndPathToken("$count", endPathExpression.NextToken);


Isn't new EndPathToken() redundant? Why does expression not already == EndPathToken("$count", endPathExpression.NextToken)?

Could you also add a few comment in here for examples you are trying to parse, such as:

(.e.g. aggregate($count as SalesCount)
(.e.g. aggregate(UnitPrice with sum as TotalUnitPrice)

Yes, it is redundant. We can't really use the ==, because it would compare the object reference. So I just removed the assignment to the new object and added the comments you asked for.

robward-ms · 2017-02-07T23:24:21Z

src/Microsoft.OData.Core/UriParser/Aggregation/ApplyBinder.cs

@@ -116,6 +129,7 @@ private IEdmTypeReference CreateAggregateExpressionTypeReference(SingleValueNode
                                    expressionPrimitiveKind));
                    }

+                case AggregationMethod.VirtualPropertyCount:


The change looks good. However, it looks like both CountDistinct and VirtualPropertyCount should return Decimal not Int64. I don't think you should change CountDistinct as part of your change, could you simply add a TODO comment that these should be Decimal instead of Int64? Something like:

// TODO: CountDistinct and $Count should return type Edm.Decimal with Scale="0" and sufficient Precision. return EdmCoreModel.Instance.GetPrimitive(EdmPrimitiveTypeKind.Int64, false);

3.1.3.5 Standard Aggregation Method countdistinct
The result property MUST have type Edm.Decimal with Scale="0" and sufficient Precision.

3.1.5 Virtual Property $count
The result property will have type Edm.Decimal with Scale="0" and sufficient Precision.

robward-ms · 2017-02-07T23:26:23Z

src/Microsoft.OData.Core/UriParser/SemanticAst/CountVirtualPropertyNode.cs

+        }
+
+        public override IEdmTypeReference TypeReference {
+            get {


Nit: put { on its own line.

robward-ms · 2017-02-07T23:31:16Z

...unctionalTests/Microsoft.OData.Core.Tests/UriParser/Parsers/UriQueryExpressionParserTests.cs

+
+            VerifyAggregateExpressionToken("$count", AggregationMethod.VirtualPropertyCount, "Count", aggregate.Expressions.First());
+        }
+


A negative test would be nice, something like "aggregate(#count with sum as Count)" and possible "aggregate(#count).

From the spec:
"It MUST always specify an alias and MUST NOT specify an aggregation method."

Good idea. Done.

…dant code and adding comments referencing issue OData#758.

robward-ms · 2017-02-14T20:55:07Z

Merged.

msftclas added the cla-not-required label Dec 30, 2016

athoscouto closed this Dec 30, 2016

athoscouto reopened this Dec 30, 2016

msftclas added the cla-not-required label Dec 30, 2016

LaylaLiu added the in-progress label Jan 4, 2017

markdstafford assigned LaylaLiu Jan 4, 2017

athoscouto changed the title ~~Implementing virtual property .~~ Implementing virtual property $count Jan 19, 2017

LaylaLiu reviewed Jan 26, 2017

View reviewed changes

Adding support and tests for virtual property count.

bfae1b8

athoscouto force-pushed the count branch from f1ec857 to bfae1b8 Compare February 7, 2017 19:35

robward-ms reviewed Feb 7, 2017

View reviewed changes

athoscouto mentioned this pull request Feb 9, 2017

countdistinct and $count are returning Edm.Int64 which is not spec compliant #758

Open

shinlges added 2 commits February 8, 2017 16:50

Removing compiler warnings, adding negative test case, removing redun…

e5a33a0

…dant code and adding comments referencing issue OData#758.

Reordering comments to avoid compiler warnings.

dff0b04

robward-ms closed this Feb 14, 2017

LaylaLiu removed the in-progress label Feb 14, 2017

robward-ms mentioned this pull request Feb 14, 2017

Custom aggregation methods #752

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing virtual property $count #738

Implementing virtual property $count #738

athoscouto commented Dec 30, 2016 •

edited

Loading

msftclas commented Dec 30, 2016

msftclas commented Dec 30, 2016

athoscouto commented Jan 20, 2017

LaylaLiu Jan 26, 2017

athoscouto Jan 26, 2017

LaylaLiu Jan 26, 2017

athoscouto Jan 26, 2017

athoscouto Jan 26, 2017

robward-ms Feb 7, 2017

athoscouto Feb 9, 2017

LaylaLiu Jan 26, 2017

athoscouto Jan 26, 2017

robward-ms Feb 7, 2017

robward-ms Feb 7, 2017

athoscouto Feb 9, 2017

robward-ms Feb 7, 2017

robward-ms Feb 7, 2017

athoscouto Feb 9, 2017

robward-ms Feb 7, 2017

athoscouto Feb 9, 2017

robward-ms commented Feb 14, 2017


		VerifyAggregateExpressionToken("$count", AggregationMethod.VirtualPropertyCount, "Count", aggregate.Expressions.First());
		}

Implementing virtual property $count #738

Implementing virtual property $count #738

Conversation

athoscouto commented Dec 30, 2016 • edited Loading

Issues

Description

Checklist (Uncheck if it is not completed)

msftclas commented Dec 30, 2016

msftclas commented Dec 30, 2016

athoscouto commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robward-ms commented Feb 14, 2017

athoscouto commented Dec 30, 2016 •

edited

Loading