Skip to content

Feat(GraphQL): Add Aggregation Queries at Child Level #7022

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

Merged
merged 2 commits into from
Dec 1, 2020

Conversation

vmrajas
Copy link
Contributor

@vmrajas vmrajas commented Dec 1, 2020

Motivation:
This PR adds Aggregation Queries at child level. Aggregation Queries handle Avg,Max,Min,Sum functions for scalar fields.
Related RFC: https://discuss.dgraph.io/t/count-queries-in-graphql/10714/9
This PR contains the following changes:

  1. Support for aggregation queries at child levels in query_rewriter.go
  2. Changes to resolver.go to handle response of aggregation queries at child level.
  3. Tests to query_test.yaml and auth_query_test.yaml
  4. e2e tests for aggregation queries at child level.
    Fixes GRAPHQL-773

This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Dec 1, 2020
@netlify
Copy link

netlify bot commented Dec 1, 2020

Deploy preview for dgraph-docs ready!

Built with commit 2afcbf5

https://deploy-preview-7022--dgraph-docs.netlify.app

@vmrajas vmrajas changed the base branch from master to GRAPHQL-773 December 1, 2020 05:26
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

Really nice PR
:lgtm:

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MichaelJCompton, @pawanrawal, and @vmrajas)


graphql/resolve/query_rewriter.go, line 1118 at r1 (raw file):

is added.

is not added


graphql/resolve/query_test.yaml, line 2935 at r1 (raw file):

dgraph.uid : uid

can we find a way to not add this for aggregate children?


graphql/resolve/resolver.go, line 1397 at r1 (raw file):

				fieldAlias := generateUniqueDgraphAlias(f, fieldSeenCount)
				aggregateFieldName := aggregateField.Name()
				aggregateVal[aggregateFieldName] = res[aggregateFieldName+"_"+fieldAlias]

could just be:

aggregateFieldName := aggregateField.Name()
aggregateVal[aggregateFieldName] = res[aggregateFieldName+"_"+uniqueDgraphAlias]

Base automatically changed from GRAPHQL-773 to master December 1, 2020 11:58
Copy link
Contributor Author

@vmrajas vmrajas left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 54 files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)


graphql/resolve/query_rewriter.go, line 1118 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
is added.

is not added

Done.


graphql/resolve/query_test.yaml, line 2935 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
dgraph.uid : uid

can we find a way to not add this for aggregate children?

No. This is done by rewriteAsQuery. I believe it is rather harmless to add it. The question of whether adding dgraph.uid to fields is relevant or not is much larger and beyond the scope of aggregation queries.


graphql/resolve/resolver.go, line 1397 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
				fieldAlias := generateUniqueDgraphAlias(f, fieldSeenCount)
				aggregateFieldName := aggregateField.Name()
				aggregateVal[aggregateFieldName] = res[aggregateFieldName+"_"+fieldAlias]

could just be:

aggregateFieldName := aggregateField.Name()
aggregateVal[aggregateFieldName] = res[aggregateFieldName+"_"+uniqueDgraphAlias]

Done.

@vmrajas vmrajas merged commit 9f027e0 into master Dec 1, 2020
@vmrajas vmrajas deleted the GRAPHQL-773-child branch December 1, 2020 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

2 participants