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

implement aggregations on nested fields #136

Merged
merged 39 commits into from
Dec 18, 2024

Conversation

hallettj
Copy link
Collaborator

@hallettj hallettj commented Dec 6, 2024

Turns on the nested field aggregation connector capability, and makes it work.

This also touches up the aggregation implementation generally. It uses the updated, unified system for referencing fields with paths. It also changes aggregation result types (besides count) to be nullable which avoids an error when aggregating over an empty document set.

Base automatically changed from jesse/recreate-supergraph-fixture to main December 10, 2024 22:32
) -> impl Iterator<Item = (AggregationFunction, Type)> {
let nullable_scalar_type = move || Type::Nullable {
underlying_type: Box::new(bson_to_named_type(scalar_type)),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not blocking, but is this useful in other areas as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It could be slightly useful to have a method on ndc_models::Type that does this. That would be a change in the ndc-spec repo. In the connector code we already have configuration::Type::make_nullable.

@hallettj hallettj merged commit a85094d into main Dec 18, 2024
1 check passed
@hallettj hallettj deleted the jessehallett/eng-1024-nested-field-aggregates branch December 18, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants