implement group by for single-column aggregates #144
Conversation
| accumulators: accumulators_for_aggregates(&grouping.aggregates)?, | ||
| }; | ||
|
|
||
| // TODO: ENG-1562 This implementation does not fully implement the |
There was a problem hiding this comment.
Do we need to do this work at a specific time or this is behind a capability?
There was a problem hiding this comment.
Oh right, that's behind the query.aggregates.group_by.order capability
| .collect() | ||
| } | ||
|
|
||
| // TODO: This function can be infallible once counts are implemented |
There was a problem hiding this comment.
Is there follow up work we need here?
There was a problem hiding this comment.
I've followed up on this in #145
| .collect() | ||
| } | ||
|
|
||
| // TODO: This function can be infallible once counts are implemented |
There was a problem hiding this comment.
Same question as above?
| let aggregates = serialize_aggregates(mode, path, &grouping.aggregates, doc.into())?; | ||
|
|
||
| // TODO: This conversion step can be removed when the aggregates map key type is | ||
| // changed from String to FieldName |
There was a problem hiding this comment.
Is there a follow up ticket for this one?
There was a problem hiding this comment.
Effectively yes. That change is in the latest ndc-spec, and I have a ticket for updating the spec https://linear.app/hasura/issue/ENG-1573/[mongodb]-update-to-latest-ndc-spec-revision
| ) -> Result<plan::GroupComparisonTarget<T>> { | ||
| let plan_target = match target { | ||
| // TODO: Do we expect the target aggregate to correspond to one of the grouping aggregates? | ||
| // Or can it be independent? |
There was a problem hiding this comment.
This seems like a question? Do we know the answer and can remove this?
There was a problem hiding this comment.
Good catch - I found the answer but forgot to remove the comment
| } | ||
| ndc::GroupOrderByTarget::Aggregate { aggregate } => { | ||
| // TODO: Do we expect the target aggregate to correspond to one of the grouping aggregates? | ||
| // Or can it be independent? |
There was a problem hiding this comment.
This seems like a question? Do we know the answer and can remove this?
| /// The observed type may be `None` if the type of a variable use could not be inferred. | ||
| pub variable_types: VariableTypes<T::ScalarType>, | ||
|
|
||
| // TODO: type for unrelated collection |
There was a problem hiding this comment.
Do we need a ticket here to track this?
There was a problem hiding this comment.
Maybe - I don't remember where this would come up
This completes the basic functionality for group-by started in #144 by implementing all forms of count aggregations.
Implements most of the functionality for the capability
query.aggregates.group_by. There are still a couple of things to follow up on.Counts are not implemented for group by queries yet. I'll follow up on those in ENG-1568. (Counts are implemented in #145 which can be merged after this PR is merged.)
There is a bug involving multiple references to the same relationship that should be resolved. I'll follow up in ENG-1569.
While working on this I removed the custom "count" aggregation - it is redundant, and I've been meaning to do that for a while. Users can use the standard count aggregations instead.
There is a change in here that explicitly converts aggregate result values for "average" and "sum" aggregations to the result types declared in the schema. This is necessary to avoid errors in response serialization for groups when aggregating over 128-bit decimal values. I applied the same type conversion for group and for root aggregates for consistency. This does mean there will be some loss of precision in those cases. But it also means we won't get back a JSON string in some cases, and a JSON number in others.