Feat(GraphQL): Add aggregate query at root level

[vmrajas]

Motivation:
the PR adds support for aggregation functions like Max, Min, Sum at root level in GraphQL.
Related RFC: https://discuss.dgraph.io/t/count-queries-in-graphql/10714/9
The PR contains the following changes:

1. Changes to query_rewriter.go to handle aggregate queries.
2. Refactor count queries rewriting to make it more aligned with other aggregate queries.
3. resolver.go . Changes to resolver.go to parse DQL result to GraphQL result for aggregate queries.

Testing:

1. Added tests to query_test.yaml and auth_query_test.yaml
2. e2e tests
   Fixes GRAPHQL-773

---

This change is 

[netlify]

Deploy preview for dgraph-docs ready!

Built with commit 2435f2f

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

[abhimanyusinghgaur]

Just some minor nit-picks, overall

Reviewed 14 of 54 files at r1.
Reviewable status: 14 of 54 files reviewed, 11 unresolved discussions (waiting on @MichaelJCompton, @pawanrawal, and @vmrajas)

---

graphql/e2e/auth/auth_test.go, line 1073 at r1 (raw file):

require.Nil(t, gqlResponse.Errors)

common.RequireNoGQLErrors(t, gqlResponse)
New best practice.

---

graphql/e2e/auth/auth_test.go, line 1188 at r1 (raw file):

require.Nil(t, gqlResponse.Errors)

common.RequireNoGQLErrors(t, gqlResponse)

---

graphql/e2e/auth/auth_test.go, line 1190 at r1 (raw file):

require.JSONEq(t, string(gqlResponse.Data), tcase.result)

require.JSONEq(t, tcase.result, string(gqlResponse.Data))

---

graphql/e2e/common/query.go, line 2855 at r1 (raw file):

"titleMin": "0.000000"

whoa! this is weird. Is this returned from Dgraph? If not, can we make it empty string, or actually better to make them null.
So, would it be better to have min/max/sum/avg as null in the case count is 0?

---

graphql/e2e/directives/test_data.json, line 89 at r1 (raw file):

1

Can we keep it zero? Zero being the minimum non-negative integer, used to serve as a special test case. Although it doesn't seem like it was being used anywhere. But, always good to have that case if need be.

---

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

interface{}

map[string]interface{}

---

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

interface{}

map[string]interface{}

so that we avoid explicit casting later when we already know it is a map

---

graphql/schema/wrappers.go, line 1504 at r1 (raw file):

ConstructedForDgraphPredicate()

typo?

DgraphPredicateForAggregateField()

---

graphql/schema/wrappers.go, line 1521 at r1 (raw file):

	if !isAggregateFunction {
		return f.DgraphPredicate()
	}

in essence, this function should not be called for any non-aggregate fields, so should we instead return an empty string from here so that one may recognize errors during development time itself?

---

graphql/schema/wrappers.go, line 1526 at r1 (raw file):

aggregateFieldName

aggregateTypeName

or maybe

aggregateResultTypeName

---

graphql/schema/wrappers.go, line 1531 at r1 (raw file):

return f.DgraphPredicate()

empty string from here as well?

[vmrajas]

Reviewable status: 14 of 54 files reviewed, 11 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, @pawanrawal, and @vmrajas)

---

graphql/e2e/auth/auth_test.go, line 1073 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

require.Nil(t, gqlResponse.Errors)

common.RequireNoGQLErrors(t, gqlResponse)
New best practice.

Done.

---

graphql/e2e/auth/auth_test.go, line 1188 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

require.Nil(t, gqlResponse.Errors)

common.RequireNoGQLErrors(t, gqlResponse)

Done.

---

graphql/e2e/auth/auth_test.go, line 1190 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

require.JSONEq(t, string(gqlResponse.Data), tcase.result)

require.JSONEq(t, tcase.result, string(gqlResponse.Data))

Done.

---

graphql/e2e/common/query.go, line 2855 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"titleMin": "0.000000"

whoa! this is weird. Is this returned from Dgraph? If not, can we make it empty string, or actually better to make them null.
So, would it be better to have min/max/sum/avg as null in the case count is 0?

Yes, it is returned from DQL. I have filed a JIRA and added a TODO to fix this from DQL side or use count

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

interface{}

map[string]interface{}

Done.

---

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

interface{}

map[string]interface{}

so that we avoid explicit casting later when we already know it is a map

Done.

---

graphql/schema/wrappers.go, line 1504 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

ConstructedForDgraphPredicate()

typo?

DgraphPredicateForAggregateField()

Done. Nice Catch !!

---

graphql/schema/wrappers.go, line 1521 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	if !isAggregateFunction {
		return f.DgraphPredicate()
	}

in essence, this function should not be called for any non-aggregate fields, so should we instead return an empty string from here so that one may recognize errors during development time itself?

It is currently true that this is called for aggregate fields only but it could change later on. The behaviour of this function is kept in accordance with the behaviour of constructedFor() function.

---

graphql/schema/wrappers.go, line 1526 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

aggregateFieldName

aggregateTypeName

or maybe

aggregateResultTypeName

Done.

---

graphql/schema/wrappers.go, line 1531 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

return f.DgraphPredicate()

empty string from here as well?

Keeping the behaviour in accordance with constructedFor() here as well.