hasura / ndc-spec

NDC Specification and Reference Implementation
http://hasura.github.io/ndc-spec
20 stars 5 forks source link

Remove path from GroupOrderByTarget::Aggregate #184

Closed daniel-chambers closed 1 month ago

daniel-chambers commented 1 month ago

This PR removes the path field from GroupOrderByTarget::Aggregate.

path should not exist on GroupOrderByTarget::Aggregate. I think path was copied from OrderByTarget::Aggregate (ie from normal row ordering) by mistake. Group aggregate ordering is not the same as row aggregate ordering. When we do group ordering, we are aggregating the group of rows and then ordering the groups by the aggregate result. When we do aggregations in normal row ordering, we are computing an aggregate per row and then ordering the rows by the aggregate result.

This is why path exists in row ordering. We can traverse a relationship from the row (probably an array relationship) in order to get an array of related rows to aggregate. (eg order Invoices by the count of each Invoice's InvoiceLines) However, that is not valid for group ordering. We are aggregating the rows in the group, it makes no sense to traverse anything before doing that.

I'd also add that when we compute aggregates for groups to return (rather than to order with), we cannot specify a path, we can only specify an Aggregate. This is another demonstration of why path should not exist for group aggregate ordering.

pub struct Grouping {
    /// Dimensions along which to partition the data
    pub dimensions: Vec<Dimension>,
    /// Aggregates to compute in each group
    pub aggregates: IndexMap<String, Aggregate>,
    ...
}