Fix(GraphQL): Fix Aggregate queries on empty data #7119

vmrajas · 2020-12-11T10:40:44Z

Motivation:
Recently support has also been added for aggregation functions in GraphQL with #6985 .
In case there does not exists any string say name in all nodes and max(name) is queried in DQL, it returns "0.0000" . Similar behaviour is also observed with min function. In case of sum and avg, if no data exists, 0 is returned.
This PR fixes this behaviour from GraphQL side and in case count of data is 0, null is returned for aggregate field.

Testing:

Added tests to query_test.yaml
Added e2e tests.

Fixes GRAPHQL-887

This change is

pawanrawal

Reviewed 7 of 7 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @abhimanyusinghgaur and @MichaelJCompton)

- Some rewording and linking added - Added information on how queries against null data behave, per #7119 and reviewer feedback

* Create aggregate.md * Update aggregate.md * Update aggregate.md * Merge branch 'master' into aaroncarey/aggregate-GQL-890 * Address most of the feedback recieved so far - repeated paragraphs/bullets refactored into a shorter repeated tip. - Adjusted example descriptions and example query syntax to fix errors, per reviewer feedback Not included: combining with the "Count Queries" content, or resolving a comment that I'm not sure how to address yet. * Combining Count Queries and Aggregate Queries, editing "weight" metadata to have non-duplicate values across files in /query * Minor edits * Update aggregate.md * Add example with filter * Update wiki/content/graphql/queries/aggregate.md Co-authored-by: Damián Parrino <bucanero@users.noreply.github.com> * Update wiki/content/graphql/queries/aggregate.md Co-authored-by: Damián Parrino <bucanero@users.noreply.github.com> * Incorporate reviewer feedback - Some rewording and linking added - Added information on how queries against null data behave, per #7119 and reviewer feedback * update example syntax * Adding a corrected version of the first advanced aggregate query example, previously removed * Update aggregate.md

…s (GRAPHQL-936) (#7240) Continued from: * #7230 * #7237 * #7238 This PR makes aggregate queries at root and child levels work with the new JSON changes. It removes the default behaviour of rewriting the count field to DQL query everytime (introduced in #7119). Now, rewritten DQL will have count field only if it was requested in GraphQL.

@Custom

…ding (GraphQL-730) (#7371) * perf(GraphQL): JSON Part-1: JSON is generated by Dgraph for simple queries (GRAPHQL-877) (#7230) This PR moves the GraphQL JSON generation for simple queries inside Dgraph. Previously, the GraphQL layer would get a JSON response from Dgraph in the form expected for DQL. That JSON response had to go through the following pipeline: Unmarshal Run GraphQL completion Marshal After this pipeline was executed, then the output JSON would be in the form as expected by GraphQL. Now, the response from Dgraph would already be in the form expected by GraphQL. So, we no more need to run the above pipeline. Hence, saving valuable time and memory. * perf(GraphQL): JSON Part-2: JSON is generated by Dgraph for simple mutations (GRAPHQL-877) (#7237) This PR is a continuation of #7230. It makes standard GraphQL mutations work with the JSON changes. Each mutation now returns the GraphQL JSON required for that mutation as a byte array from the MutationResolver itself, so no further processing of the JSON is required. Delete mutations now query the `queryField` explicitly before executing the mutation, to make it work with the JSON changes. * perf(GraphQL): JSON Part-3: Geo queries (GRAPHQL-879) (#7238) Continued from: * #7230 * #7237 This PR makes Geo queries/mutations work with the new JSON changes. * perf(GraphQL): JSON Part-4: Aggregate queries at root and child levels (GRAPHQL-936) (#7240) Continued from: * #7230 * #7237 * #7238 This PR makes aggregate queries at root and child levels work with the new JSON changes. It removes the default behaviour of rewriting the count field to DQL query everytime (introduced in #7119). Now, rewritten DQL will have count field only if it was requested in GraphQL. * review comments from merged PRs * perf(GraphQL): JSON Part-5: Change query rewriting for scalar/object fields asked multiple times at same level (GRAPHQL-938) (#7283) Previously, for a GraphQL query like this: ``` query { queryThing { i1: id i2: id name n: name n1: name } } ``` GraphQL layer used to generate following DQL query: ``` query { queryThing(func: type(Thing)) { id : uid name : Thing.name } } ``` This can’t work with the new way of JSON encoding for GraphQL because it expects that everything asked for in GraphQL query is present in DQL response. So, Now, the GraphQL layer generates the following DQL to make it work: ``` query { queryThing(func: type(Thing)) { Thing.i1 : uid Thing.i2: uid Thing.name : Thing.name Thing.n : Thing.name Thing.n1 : Thing.name } } ``` Basically, the `DgraphAlias()` is now generated using the `TypeCondition.GraphqlAlias` format. That works even with fragments as they have their specific type conditions. Here are related thoughts: ``` query { queryHuman { name n: name } # DQL: use field.name as DgAlias() # queryHuman(func: type(Human)) { # name: Human.name # name: Human.name # } # => just using field.name doesn't work => use field.alias # queryHuman(func: type(Human)) { # name: Human.name # n: Human.name # } # but there are interfaces and fragments queryCharacter { ... on Human { n: name } ... on Droid { n: bio } } # DQL: use field.alias as DgAlias() # queryCharacter(func: type(Character)) { # n: Character.name # n: Character.bio # } # => just using field.alias doesn't work => use field.name + field.alias # queryCharacter(func: type(Character)) { # name.n: Character.name # bio.n: Character.bio # } # but the implementing types may have fields with same name not inherited from the interface queryThing { ... on ThingOne { c: color } ... on ThingTwo { c: color } } # DQL: use field.name + field.alias as DgAlias() # queryThing(func: type(Thing)) { # color.c: ThingOne.color # color.c: ThingTwo.color # } # => even that doesn't work => use Typename + field.name + field.alias # queryThing(func: type(Thing)) { # ThingOne.color.c: ThingOne.color # ThingTwo.color.c: ThingTwo.color # } # nice! but then different frags explicitly ask for an inherited field with same name & alias queryCharacter { ... on Human { n: name } ... on Droid { n: name } } # DQL: use Typename + field.name + field.alias as DgAlias() # queryCharacter(func: type(Character)) { # Character.name.n: Character.name # Character.name.n: Character.name # } # => doesn't work => use typeCond + Typename + field.name + field.alias # queryCharacter(func: type(Character)) { # Human.Character.name.n: Character.name # Droid.Character.name.n: Character.name # } # but wait, wouldn't just typeCond + field.alias work? # queryCharacter(func: type(Character)) { # Human.n: Character.name # Droid.n: Character.name # } # yeah! that works even for all the above cases. # # OR # # just appending the count when encountering duplicate alias at the same level also works. # But, we need to maintain the count map every time we need DgAlias(). # Also, this requires query pre-processing and the generated aliases are non-deterministic. # # So, we are going with the typeCond + field.alias approach. That will work with @Custom(dql:...) too. } ``` * perf(GraphQL): JSON Part-6: Admin Server and custom HTTP query/mutation are now resolved separately (GRAPHQL-937) (#7317) This PR: * makes the admin server on `/admin` and `@custom(http: {...})` Query/Mutation work through a common pipeline separate from the queries/mutations resolved through Dgraph * adds scalar coercion to queries resolved through Dgraph * and, removes completion as a step from resolvers, as it is not needed anymore. * perf(GraphQL): JSON Part-7: JSON is generated by Dgraph for custom HTTP fields (GRAPHQL-878) (#7361) This PR moves the resolution and GraphQL JSON generation for `@custom(http: {...})` fields inside Dgraph. It also fixes following existing bugs with `@custom` field resolution: * https://discuss.dgraph.io/t/slash-graphql-lambda-bug/12233 (GRAPHQL-967) * race condition in custom logic with repeated fields in implementing types (GRAPHQL-860) * GRAPHQL-639 * perf(GraphQL): JSON part-8: Cleanup and comments (GRAPHQL-947) (#7376) This is a cleanup PR for the JSON changes. It is mostly about nit-picks, making some old tests work, skipping some tests to be ported later to e2e and making the CI green overall. It also fixes a [Discuss Issue](https://discuss.dgraph.io/t/custom-field-resolvers-are-not-always-called/12489) (GRAPHQL-989). * fix merge * review comments * fix tests * fix more tests

Fix aggregate queries on empty data

5909f07

vmrajas requested review from pawanrawal and abhimanyusinghgaur December 11, 2020 10:40

vmrajas requested a review from MichaelJCompton as a code owner December 11, 2020 10:40

github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Dec 11, 2020

pawanrawal approved these changes Dec 11, 2020

View reviewed changes

Empty commit to make the CLA pass

4b666aa

vmrajas merged commit 8b8b0ad into master Dec 11, 2020

vmrajas deleted the GRAPHQL-887 branch December 11, 2020 11:26

aaroncarey added a commit that referenced this pull request Dec 11, 2020

Incorporate reviewer feedback

34adb92

- Some rewording and linking added - Added information on how queries against null data behave, per #7119 and reviewer feedback

abhimanyusinghgaur mentioned this pull request Jan 4, 2021

perf(GraphQL): JSON Part-4: Aggregate queries at root and child levels (GRAPHQL-936) #7240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(GraphQL): Fix Aggregate queries on empty data #7119

Fix(GraphQL): Fix Aggregate queries on empty data #7119

vmrajas commented Dec 11, 2020 •

edited by manishrjain

Loading

pawanrawal left a comment

Fix(GraphQL): Fix Aggregate queries on empty data #7119

Fix(GraphQL): Fix Aggregate queries on empty data #7119

Conversation

vmrajas commented Dec 11, 2020 • edited by manishrjain Loading

pawanrawal left a comment

Choose a reason for hiding this comment

vmrajas commented Dec 11, 2020 •

edited by manishrjain

Loading