Support for $apply/aggregate #1921

gathogojr · 2020-11-11T13:41:16Z

Issues

This pull request is 1 of 3 from splitting this huge pull request to make it easier to review.

Description

The simple aggregations below are covered. The scope of this PR is aggregations that return a single scalar result.

    // EntitySet?$apply=aggregate(Prop with average as AverageProp)
    EntitySet.Average(d1 => d1.Prop)

    // EntitySet?$apply=aggregate(Prop with sum as SumProp)
    EntitySet.Sum(d1 => d1.Prop)

    // EntitySet?$apply=aggregate(Prop with min as MinProp)
    EntitySet.Min(d1 => d1.Prop)

    // EntitySet?$apply=aggregate(Prop with max as MaxProp)
    EntitySet.Max(d1 => d1.Prop)

    // EntitySet?$apply=aggregate(Prop with countdistinct as CountDistinctProp)
    EntitySet.CountDistinct(d1 => d1.Prop)

    // EntitySet?$apply=aggregate(NavProp/Prop with average as AverageNavPro_Prop)
    EntitySet.Average(d1 => d1.NavProp.Prop)

    // EntitySet?$apply=aggregate(NavProp/Prop with sum as SumNavPro_Prop)
    EntitySet.Sum(d1 => d1.NavProp.Prop)

    // EntitySet?$apply=aggregate(NavProp/Prop with min as MinNavPro_Prop)
    EntitySet.Min(d1 => d1.NavProp.Prop)

    // EntitySet?$apply=aggregate(NavProp/Prop with max as MaxNavPro_Prop)
    EntitySet.Max(d1 => d1.NavProp.Prop)

    // EntitySet?$apply=aggregate(NavProp/Prop with countdistinct as CountDistinctNavPro_Prop)
    EntitySet.CountDistinct(d1 => d1.NavProp.Prop)

    // NOTE: We support nested navigation properties in the aggregate expression, i.e. d1 => d1.NavProp.NestedNavProp.Prop
    // E.g.
    // EntitySet?$apply=aggregate(NavProp/NestedNavProp/Prop with average as AverageNavProp_NestedNavProp_Prop)
    EntitySet.Average(d1 => d1.NavProp.NestedNavProp.Prop)

The filter transformation used together with aggregation is also covered. A filter transformation is used (not $filter) to restrict the set of data to be aggregated.

    // EntitySet?$apply=filter(Amount gt 1)/aggregate(Prop with average as AverageProp)
    EntitySet.Where(d0 => d0.Amount > 1).Average(d1 => d1.Prop)

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.

src/Microsoft.OData.Client/ALinq/ApplyQueryOptionExpression.cs

src/Microsoft.OData.Client/ALinq/UriWriter.cs

marabooy

LGTM

habbes · 2020-11-16T09:23:42Z

src/Microsoft.OData.Client/ALinq/DataServiceQueryProvider.cs

+
+            ODataMessageReaderSettings messageReaderSettings = new ODataMessageReaderSettings
+            {
+                Validations = ~ValidationKinds.ThrowOnUndeclaredPropertyForNonOpenType


What does the ~ operator do in this case?

@habbes The ~ operator performs a bitwise complement operation on its operand. In this case, we're configuring the message reader NOT to throw on undeclared property for non-open type.

habbes · 2020-11-16T09:27:20Z

src/Microsoft.OData.Client/ALinq/ResourceBinder.cs

+                // Disallow unsupported scenarios like the following:
+                // - Average(d1 => d1.Prop.Length)
+                // - Average(d1 => d1.CollectionProp.Count)
+                MemberExpression parentExpr = StripTo<MemberExpression>(memberExpr.Expression);
+                if (parentExpr != null)
+                {
+                    if (PrimitiveType.IsKnownNullableType(parentExpr.Type))
+                    {
+                        throw new NotSupportedException(Strings.ALinq_InvalidAggregateExpression(expr));
+                    }
+
+                    Type collectionType = ClientTypeUtil.GetImplementationType(parentExpr.Type, typeof(ICollection<>));
+                    if (collectionType != null)
+                    {
+                        throw new NotSupportedException(Strings.ALinq_InvalidAggregateExpression(expr));
+                    }
+                }
+            }


What is it that's not supported? Is it accessing the length of properties like strings or collections or is it accessing nested properties? is d1.Prop.ChildProp supported? and is d1.Rectangle.Length supported (assuming Square is not a collection)?

@habbes Nested properties are supported. E.g. If you have a navigation property Product, you can group by property ProductName of Product type as follows:

GroupBy(d1 => d1.Product.ProductName, ... )

What is not supported, is invoking properties of a known primitive type in the grouping expression. E.g. say ProductName in my example is of type string, it would be invalid to have

GroupBy(d1 => d1.Product.ProductName.Length, ... )

We throw a NotSupportedExpression if that is used in the GroupBy keySelector expression

Note that this validation does not stop you from doing this:

GroupBy(d1 => d1.Product.ProductName, (d1, d2) => new { ProductNameLength = d1.Length, ... });

Maybe the comment or error-message should be more clear that the scenario being guarded against is accessing properties of known primitive types.

habbes · 2020-11-16T09:37:49Z

src/Microsoft.OData.Client/ALinq/ResourceBinder.cs

@@ -2910,6 +2988,46 @@ internal static void ValidateExpandPath(Expression input, DataServiceContext con

                throw new NotSupportedException(Strings.ALinq_InvalidExpressionInNavigationPath(input));
            }
+
+            /// <summary>
+            /// Checks whether the specified <paramref name="expr"/> is a valid aggregate expression.


It would be good to know what counts as a valid aggregate expression.

In the specification, the aggregate expression has to meet one of 5 criteria. I'll see how I can improve the docstring to give the user a better idea. The exception message tries to capture what is wrong with the exception.

Have to admit I can't say I fully understand this line :)

An aggregate expression must evaluate to a single-valued property path to an aggregatable property

@odero
Consider the following class:

class Sale { public string Currency { get; set; } public Amount Amount { get; set; } public Product Product { get; set; } public Collection<Review> Reviews { get; set; } }

Currency, Amount and Product are single-valued properties. Reviews is a collection property.

On the subject of aggregatable (or is it aggregable?), only Amount fits the bill - think average, sum, min, and max. You can't aggregate Currency - of type string, or Product - of type Product.

However, Currency would qualify as aggregatable property when using countdistinct aggregation.

The fact that I have used this many words and an illustration to explain tells you how hard it is to come up with a good docstring for the method :) I resorted to borrowing the words from the specification.
If you go through the method there are helpful comments that explain the scenarios we're validating for.

habbes · 2020-11-16T09:39:26Z

src/Microsoft.OData.Client/ALinq/TypeSystem.cs

+            aggregationMethodMap.Add(AggregationMethod.Sum, "sum");
+            aggregationMethodMap.Add(AggregationMethod.Average, "average");
+            aggregationMethodMap.Add(AggregationMethod.Min, "min");
+            aggregationMethodMap.Add(AggregationMethod.Max, "max");
+            aggregationMethodMap.Add(AggregationMethod.CountDistinct, "countdistinct");
+            aggregationMethodMap.Add(AggregationMethod.VirtualPropertyCount, "$count");


Out of curiosity, why doesn't this use the string constants defined in UriHelper?

habbes · 2020-11-16T09:40:59Z

src/Microsoft.OData.Client/ALinq/UriWriter.cs

+            }
+            aggregateBuilder.Append(UriHelper.RIGHTPAREN);


Maybe an empty line after the close }?

KenitoInc

A few comments/questions

KenitoInc · 2020-11-16T06:33:15Z

src/Microsoft.OData.Client/ALinq/DataServiceQueryProvider.cs

+            };
+
+            ODataResource entry = default(ODataResource);
+            using (var messageReader = new ODataMessageReader(httpWebResponseMessage, messageReaderSettings, this.Context.Format.ServiceModel))


I got it from Mike that we should avoid using var as much as we can. Use the explicit type.

KenitoInc · 2020-11-16T12:18:03Z

src/Microsoft.OData.Client/ALinq/ResourceBinder.cs

+            internal static void ValidateAggregateExpression(Expression expr)
+            {
+                MemberExpression memberExpr = StripTo<MemberExpression>(expr);
+


What does StripTo do?

@KenitoInc It strips the expression of intermediate expressions like Expression.Convert() and Expression.Quote(). See more here

KenitoInc · 2020-11-16T12:44:07Z

src/Microsoft.OData.Client/DataServiceExtensions.cs

+        /// <typeparam name="TSource">The type of the elements of <paramref name="source"/></typeparam>
+        /// <typeparam name="TTarget">The type returned by the projection function represented in <paramref name="selector"/>.</typeparam>
+        /// <param name="source">A sequence of values of type <typeparamref name="TSource"/>.</param>
+        /// <param name="selector">A projection function to apply to each element.</param>


For the CountDistinct methods, the selector params description should be different. One is an Expression, the other is a Func.

@KenitoInc Note that in the official docs for Queryable and Enumerable methods, the descriptions don't differ much

selector Func<TSource,TResult> - A transform function to apply to each element.
selector Expression<Func<TSource,TResult>> - A projection function to apply to each element.

KenitoInc · 2020-11-16T12:47:02Z

src/Microsoft.OData.Client/DataServiceExtensions.cs

+            // - new List<int> { 1, 2, 1 }.CountDistinct(d => d)
+
+            // Method: Select<TSource,TResult>(IEnumerable<TSource>, Func<TSource,TResult>)
+            MethodInfo selectMethod = typeof(Enumerable).GetMethods(BindingFlags.Static | BindingFlags.Public)


Is it possible to refractor the GetSelectMethod and move this logic there?

The logic for getting the Select defined in Enumerable type differs a little from the one defined in Queryable type but I'll see how I can refactor and use distinct names for each.

gathogojr · 2020-11-16T13:47:37Z

FYI:

When a Where method call appears before the aggregation method (e.g. Average, Sum, etc.), a filter transformation should be used to restrict the set of data to be aggregated. $filter query option, as well as other query options like $orderby, $expand, $top, $skip, etc., are not allowed before the $apply query option.

http://docs.oasis-open.org/odata/odata-data-aggregation-ext/v4.0/cs02/odata-data-aggregation-ext-v4.0-cs02.html#_Toc435016585

Example

dataServiceContext.Sales.Where(d0 => d0.Amount > 1).Average(d1 => d1.Amount);

should be translated into

/Sales?$apply=filter(Amount gt 1)/aggregate(Amount with average as AverageAmount)

NOT

/Sales?$filter=Amount gt 1&$apply=aggregate(Amount with average as AverageAmount)

Take note of the filter transformation (under $apply with no $ sign). I'll be checking in the logic to support that.

The expand transformation will be handled in a separate pull request

habbes · 2020-11-25T11:00:20Z

src/Microsoft.OData.Client/ALinq/ApplyQueryOptionExpression.cs

+			foreach (Expression expr in this.filterExpressions)
+			{
+				if (isFirst)
+				{
+					combinedPredicate = expr;
+					isFirst = false;
+				}
+				else
+				{
+					combinedPredicate = Expression.And(combinedPredicate, expr);
+				}
+			}


Maybe his could be simplified as

combinedPredicate = filterExpressions.Aggregate((leftExpr, rightExpr) => Expression.And(leftExpr, rightExpr));

If it doesn't negatively impact readability or perf that is.

I like it :)

tbf I find the previous one more readable

habbes · 2020-11-25T11:10:18Z

src/Microsoft.OData.Client/ALinq/QueryableResourceExpression.cs

+            else if (this.Skip != null)
+            {
+                // $skip and $top may be used together with rollup. 
+                // However, support for rollup is currently not implemented in OData WebApi
+                // If $skip and/or $top appears before $apply, its currently ignored. 
+                // Makes sense to throw an exception to avoid giving a false impression.
+                throw new NotSupportedException(Strings.ALinq_QueryOptionOutOfOrder("apply", "skip"));


Should we make the assumption that the client will only be used with servers based on WebAPI?

@habbes I'll sidebar with you on this

@habbes @odero Just to clarify, ODL doesn't support rollup either.
The rollup grouping operator allows requesting additional levels of aggregation in addition to the most granular level defined by the grouping properties. It can be used instead of a property path in the first parameter of groupby. To me its a feature for the future if ever.

Here's a link to the section of the spec that describes how it works http://docs.oasis-open.org/odata/odata-data-aggregation-ext/v4.0/cs02/odata-data-aggregation-ext-v4.0-cs02.html#_Toc435016584

habbes · 2020-11-25T21:06:59Z

src/Microsoft.OData.Client/ALinq/ApplyQueryOptionExpression.cs

+		internal ReadOnlyCollection<Expression> PredicateConjuncts
+		{
+			get
+			{
+				return new ReadOnlyCollection<Expression>(this.filterExpressions);
+			}
+		}


Is this property accessed multiple times? And if so, is it expected to return a new collection each time?

@habbes ReadOnlyCollection is just a wrapper on top of the filterExpressions list. It does not create a new list
https://docs.microsoft.com/en-us/dotnet/api/system.collections.objectmodel.readonlycollection-1?view=net-5.0#examples

habbes · 2020-11-25T21:12:51Z

src/Microsoft.OData.Client/ALinq/QueryableResourceExpression.cs

+                // The $apply query option is evaluated first, then other query options ($filter, $orderby, $select) are evaluated, 
+                // if applicable, on the result of $apply in their normal order.
+                // http://docs.oasis-open.org/odata/odata-data-aggregation-ext/v4.0/cs02/odata-data-aggregation-ext-v4.0-cs02.html#_Toc435016590
+
+                // If a Where appears before an aggregation method (e.g. Average, Sum, etc) or GroupBy, 
+                // the conjuncts of the filter expression will be used to restrict the set of data to be aggregated. 
+                // They will not appear on the $filter query option. Instead, we use them to construct a filter transformation.
+                // E.g. /Sales?$apply=filter(Amount gt 1)/aggregate(Amount with average as AverageAmount)
+
+                // If a Where appears after an aggregation method or GroupBy, the conjuncts should appear
+                // on a $filter query option after the $apply.
+                // E.g. /Sales?$apply=groupby((Product/Color),aggregate(Amount with average as AverageAmount))&$filter=Product/Color eq 'Brown'


So there's support for having Where clauses both before and after an aggregation? In this case the generated query would contain both a filter transformation in the $apply clause as well as a $filter clause? Like /Sales?$apply=filter(Amount gt 1)/groupby((Product/Color),aggregae(Amount with average as Avg))$filter=Product/Color eq 'Brown'?
Is this a correct implication of what you've explained here? If so, maybe a test case could be added to verify that i's supported?

@habbes You understood it correctly. A $filter (as well as $order and $select) query option can appear after the $apply. When it does, it'll be applied to the result of the aggregation.

As a matter of fact, a filter transformation can also appear after groupby and aggregate expression. When it does, it'll be applied to the result of the aggregation, such that the following two expressions give you the same result:

/Sales?$apply=filter(Amount gt 1)/groupby((Product/Color),aggregate(Amount with average as AverageAmount))&$filter=Product/Color eq 'Brown'
We're dealing with 2 query params here - $apply and $filter - separated by a &. The $apply here has 2 transformations - filter and groupby - separated by a forward slash (/). The $filter will be applied to the aggregated result.

/Sales?$apply=filter(Amount gt 1)/groupby((Product/Color),aggregate(Amount with average as AverageAmount))/filter(Product/Color eq 'Brown')
We're dealing with a single query param here - $apply. It has 3 transformations separated by a forward slash (/). The filter transformation before groupby restricts the set of data to be aggregated while the one that appears after is applied to the aggregated result

That said, I deliberately decided to leave out support for filter transformation appearing after the groupby and aggregate transformation as well as system query options appearing after $apply. The effort required to make them play is not trivial. That work will tackled in a separate work item/pull request

odero · 2020-11-26T05:43:33Z

src/Microsoft.OData.Client/ALinq/ApplyQueryOptionExpression.cs

+	using System;
+	using System.Collections.Generic;
+    using System.Collections.ObjectModel;
+    using System.Linq.Expressions;
+	using Microsoft.OData.UriParser.Aggregation;


Fix indentation

odero · 2020-11-26T05:44:53Z

src/Microsoft.OData.Client/ALinq/ApplyQueryOptionExpression.cs

+		/// <summary>
+		/// The <see cref="ExpressionType"/> of the <see cref="Expression"/>.
+		public override ExpressionType NodeType


Suggested change

/// <summary>

/// The <see cref="ExpressionType"/> of the <see cref="Expression"/>.

public override ExpressionType NodeType

/// <summary>

/// The <see cref="ExpressionType"/> of the <see cref="Expression"/>.

/// </summary>

public override ExpressionType NodeType

odero · 2020-11-26T05:53:10Z

src/Microsoft.OData.Client/ALinq/ApplyQueryOptionExpression.cs

+				this.Expression = exp;
+				this.AggregationMethod = aggregationMethod;
+				this.AggregationAlias = string.Empty;


Suggested change

this.Expression = exp;

this.AggregationMethod = aggregationMethod;

this.AggregationAlias = string.Empty;

Aggregation(exp, aggregationMethod, string.Empty)

odero · 2020-11-26T06:14:27Z

src/Microsoft.OData.Client/ALinq/ResourceBinder.cs

+        private static Expression AnalyzeAggregation(MethodCallExpression methodCallExpr, AggregationMethod aggregationMethod)
+        {
+            Debug.Assert(methodCallExpr != null, "methodCallExpr != null");
+            if (methodCallExpr.Arguments.Count != 2)


Why must the number of args be == 2?

@odero Aggregation methods - Average, Sum, Min, Max) defined in Queryable and Enumerable classes have two sets of overloads for each aggregable type (double, double?, float, float?, int, int?, long, long?, decimal, decimal?)

The first overload takes one parameter. Examples: Average(IQueryable<Decimal>) or Average(IEnumerable<Decimal>).
To use this overload, you'd write the expression as follows:
context.Sales.Select(d1 => d1.Amount).Average(); // => Average(IEnumerable<Decimal>)

The second overload takes two parameters. Example: Average<TSource>(IQueryable<TSource>, Expression<Func<TSource,Decimal>>).
To use this overload, you'd write the expression as follows:
context.Sales.Average(d1 => d1.Amount);

I implemented support for the second overload in this pull request. That's the reason we're debug-asserting to verify that we're dealing with the second overload. It is succinct and requires the least amount of effort and almost no refactor to fit into the existing logic.

Implementing the first overload would be a lot of pain with little or no additional gain. Among other reasons, the Select() method is currently translated into $select expression. We'd need to do quite a bit of messy plumbing and refactor to handle Select() differently depending on whether or not its followed by an aggregation method, i.e.:

Translate Select(d1 => d1.Amount) into $select=Amount, and

Translate Select(d1 => d1.Amount).Average() into $apply=aggregate(Amount with average as AverageAmount)

The strategy would be to "look ahead" when doing the translation to determine which expression to translate into.
That support could be implemented at a later date. The effort would not be trivial so we'd need to have a strong justification.

We'd obviously need to document what is supported for the benefit of the users.

odero · 2020-11-26T06:17:50Z

src/Microsoft.OData.Client/ALinq/ResourceBinder.cs

@@ -2910,6 +2988,46 @@ internal static void ValidateExpandPath(Expression input, DataServiceContext con

                throw new NotSupportedException(Strings.ALinq_InvalidExpressionInNavigationPath(input));
            }
+
+            /// <summary>
+            /// Checks whether the specified <paramref name="expr"/> is a valid aggregate expression.


Have to admit I can't say I fully understand this line :)

An aggregate expression must evaluate to a single-valued property path to an aggregatable property

odero · 2020-11-26T07:11:52Z

src/Microsoft.OData.Client/ALinq/ResourceBinder.cs

+                MemberExpression memberExpr = StripTo<MemberExpression>(expr);
+
+                // ResourceBinder's VisitMemberAccess override transforms member access expressions 
+                // involving properties of known primitive types into their method method equivalent


Suggested change

// involving properties of known primitive types into their method method equivalent

// involving properties of known primitive types into their method equivalent

odero · 2020-11-26T07:23:25Z

src/Microsoft.OData.Client/ALinq/UriWriter.cs

+                return string.Empty;
+            }
+
+            return "filter(" + this.ExpressionToString(applyQueryOptionExpr.GetPredicate(), /*inPath*/ false) + ")"; ;


Suggested change

return "filter(" + this.ExpressionToString(applyQueryOptionExpr.GetPredicate(), /*inPath*/ false) + ")"; ;

return "filter(" + this.ExpressionToString(applyQueryOptionExpr.GetPredicate(), /*inPath*/ false) + ")";

odero · 2020-11-26T08:01:43Z

test/FunctionalTests/Microsoft.OData.Client.Tests/TestHttpWebRequestMessage.cs

+		this.getResponseStream = getResponseStream;
+	}
+
+#if (NETCOREAPP1_0 || NETCOREAPP2_0)


Is this check necessary? What about core 3+?

@odero Maybe not... I'll remove it and see how the tests behave. We're overriding GetResponse() for Core 3+ but looking at HttpWebRequestMessage class, I think the override should apply for all target versions

xuzhg · 2020-11-30T18:22:02Z

src/Microsoft.OData.Client/ALinq/ResourceBinder.cs

+                return methodCallExpr;
+            }
+
+            QueryableResourceExpression input;


input [](start = 40, length = 5)

maybe rename it as "output", it's weird to use "input" as output. maybe other name

xuzhg · 2020-11-30T18:30:11Z

src/Microsoft.OData.Client/DataServiceQueryOfT.cs

@@ -386,17 +384,10 @@ public virtual DataServiceQuery<TElement> AddQueryOption(string name, object val

        /// <summary>Executes the query and returns the results as a collection.</summary>
        /// <returns>A typed enumerator over the results in which TElement represents the type of the query results.</returns>
-#if !PORTABLELIB // Synchronous methods not available


#if !PORTABLELIB [](start = 0, length = 16)

thanks for removing this

xuzhg · 2020-11-30T18:31:48Z

src/Microsoft.OData.Client/DataServiceExtensions.cs

+        {
+            MethodCallExpression countMethodExpr;
+
+            if (source.Provider.GetType().Equals(typeof(DataServiceQueryProvider)))


source.Provider.GetType().Equals(typeof(DataServiceQueryProvider))) [](start = 16, length = 67)

Could the provider type be the sub type of "DataServiceQueryProvider"?

@xuzhg Thanks to your observation here. As a result of looking at it afresh I was able to significantly reduce the code in the extension method

xuzhg · 2020-11-30T18:33:50Z

src/Microsoft.OData.Client/DataServiceExtensions.cs

+        /// <summary>
+        /// Returns Distinct method defined in <paramref name="declaringType"/>.
+        /// </summary>
+        private static MethodInfo GetDistinctMethod(Type declaringType, Type sourceType)


GetDistinctMethod [](start = 34, length = 17)

should be static (cache) it?

@xuzhg Used SimpleLazy class to initialize a private static readonly field with the result of the method call. It should ensure that we only call the method once. Let me know if its a good fit

xuzhg · 2020-11-30T18:34:38Z

src/Microsoft.OData.Client/DataServiceExtensions.cs

+        /// <param name="source">A sequence of values of type <typeparamref name="TSource"/>.</param>
+        /// <param name="selector">A transform function to apply to each element.</param>
+        /// <returns>Distinct count of elements in a sequence after applying the projection function to each element.</returns>
+        public static int CountDistinct<TSource, TTarget>(this IEnumerable<TSource> source, Func<TSource, TTarget> selector)


IEnumerable source [](start = 63, length = 27)

should we check the null?

xuzhg · 2020-11-30T18:34:49Z

src/Microsoft.OData.Client/DataServiceExtensions.cs

+        {
+            MethodCallExpression countMethodExpr;
+
+            if (source.Provider.GetType().Equals(typeof(DataServiceQueryProvider)))


should we check the null?

xuzhg · 2020-11-30T18:35:23Z

test/FunctionalTests/Microsoft.OData.Client.Tests/TestHttpWebRequestMessage.cs

+namespace Microsoft.OData.Client.Tests
+{
+    public class TestHttpWebRequestMessage : HttpWebRequestMessage
+{


xuzhg · 2020-12-03T19:25:51Z

src/Microsoft.OData.Client/ALinq/UriHelper.cs

+        /// <summary>aggregate</summary>
+        internal const string AGGREGATE = "aggregate";
+
+        /// <summary>aggregate</summary>


xuzhg · 2020-12-03T19:26:38Z

src/Microsoft.OData.Client/ALinq/UriHelper.cs

+        internal const string COUNTDISTINCT = "countdistinct";
+
+        /// <summary>$count</summary>
+        internal const string VIRTUALPROPERTYCOUNT = "$count";


VIRTUALPROPERTYCOUNT [](start = 30, length = 20)

why has a name "virtual property count" equals "$count"?

@xuzhg I borrowed this one from ODL

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

Line 32 in 576c4ab

VirtualPropertyCount,

xuzhg · 2020-12-03T19:29:05Z

src/Microsoft.OData.Client/ALinq/UriWriter.cs

+            aggregateBuilder.Append(UriHelper.LEFTPAREN);
+            int i = 0;
+
+            while (true)


while (true) [](start = 11, length = 13)

why don't use foreach (...) ?

@xuzhg This is one scenario where traversing the collection using the indexer worked better. In a single statement, we're able increment the counter and check if its time to exit to avoid adding a trailing comma. The trailing comma would make the Uri invalid. Re:

if (++i == aggregations.Count) { break; } aggregateBuilder.Append(UriHelper.COMMA);

xuzhg

xuzhg · 2020-12-03T19:31:20Z

me too,

In reply to: 530944405 [](ancestors = 530944405)

…regated

gathogojr requested review from odero, marabooy, wandeg, paulodero, g2mula, wachugamaina, KenitoInc, habbes, mikepizzo, xuzhg, ElizabethOkerio and Sreejithpin November 11, 2020 13:41

gathogojr mentioned this pull request Nov 11, 2020

Support for $apply/groupby and $apply/aggregate #1810

Closed

2 tasks

gathogojr force-pushed the feature/596-support-dollar-apply-aggregate branch 2 times, most recently from 47393b6 to dec83ae Compare November 11, 2020 15:58

gathogojr added the Ready for review Use this label if a pull request is ready to be reviewed label Nov 11, 2020

marabooy reviewed Nov 13, 2020

View reviewed changes

src/Microsoft.OData.Client/ALinq/ApplyQueryOptionExpression.cs Outdated Show resolved Hide resolved

src/Microsoft.OData.Client/ALinq/UriWriter.cs Outdated Show resolved Hide resolved

src/Microsoft.OData.Client/ALinq/UriWriter.cs Outdated Show resolved Hide resolved

marabooy approved these changes Nov 13, 2020

View reviewed changes

habbes reviewed Nov 16, 2020

View reviewed changes

KenitoInc reviewed Nov 16, 2020

View reviewed changes

habbes approved these changes Nov 17, 2020

View reviewed changes

gathogojr force-pushed the feature/596-support-dollar-apply-aggregate branch from 73c9120 to 868a0b2 Compare November 25, 2020 07:23

habbes reviewed Nov 25, 2020

View reviewed changes

odero reviewed Nov 26, 2020

View reviewed changes

odero approved these changes Nov 26, 2020

View reviewed changes

gathogojr force-pushed the feature/596-support-dollar-apply-aggregate branch from 3a1166f to cd58440 Compare November 30, 2020 18:05

xuzhg reviewed Nov 30, 2020

View reviewed changes

xuzhg reviewed Dec 3, 2020

View reviewed changes

xuzhg approved these changes Dec 3, 2020

View reviewed changes

John Gathogo added 10 commits December 7, 2020 20:37

Split groupby and aggregate PR to make it easier to review

13541aa

Update Public Api baseline

3c3b4de

Remove unrequired method

b4cfd24

Improvement refactor

637fc3b

Improvements based on review

0e647b0

Fix formatting

d4f3eda

Address review comments

21ceaad

Implement filter transformation to restrict the set of data to be agg…

1b050f3

…regated

Implement changes requested in review

986acea

Remove unrequired framework check

47c8fcb

gathogojr force-pushed the feature/596-support-dollar-apply-aggregate branch from cd58440 to 47c8fcb Compare December 7, 2020 17:37

John Gathogo added 2 commits December 7, 2020 22:39

Address review feedback

23d6d52

Improvements from review feedback

e1fec7b

gathogojr merged commit 37689e1 into OData:master Dec 8, 2020

ElizabethOkerio mentioned this pull request Feb 12, 2021

The method 'Any' is not supported #1996

Closed

gathogojr deleted the feature/596-support-dollar-apply-aggregate branch June 14, 2022 11:17

	// involving properties of known primitive types into their method method equivalent
	// involving properties of known primitive types into their method equivalent

	return "filter(" + this.ExpressionToString(applyQueryOptionExpr.GetPredicate(), /inPath/ false) + ")"; ;
	return "filter(" + this.ExpressionToString(applyQueryOptionExpr.GetPredicate(), /inPath/ false) + ")";

Support for $apply/aggregate #1921

Support for $apply/aggregate #1921

Conversation

gathogojr commented Nov 11, 2020 • edited Loading

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

marabooy left a 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

gathogojr Nov 16, 2020 • edited Loading

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

gathogojr Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KenitoInc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Nov 16, 2020 • edited Loading

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

gathogojr commented Nov 16, 2020 • edited Loading

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

habbes Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes Nov 25, 2020 • edited Loading

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

gathogojr Nov 26, 2020 • edited Loading

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

gathogojr Dec 7, 2020 • edited Loading

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

xuzhg left a comment

Choose a reason for hiding this comment

xuzhg commented Dec 3, 2020

gathogojr commented Nov 11, 2020 •

edited

Loading

gathogojr Nov 16, 2020 •

edited

Loading

gathogojr Nov 26, 2020 •

edited

Loading

gathogojr Nov 16, 2020 •

edited

Loading

gathogojr commented Nov 16, 2020 •

edited

Loading

habbes Nov 25, 2020 •

edited

Loading

habbes Nov 25, 2020 •

edited

Loading

gathogojr Nov 26, 2020 •

edited

Loading

gathogojr Dec 7, 2020 •

edited

Loading