-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat: Add ability to explain sumNode
attribute(s).
#559
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving assuming you address the 2 points I've raised. Seems like you'll see those on all your explain PRs 😅.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of minor comments on the tests, otherwise looks good!
503e110
to
71aa2a5
Compare
Codecov Report
@@ Coverage Diff @@
## develop #559 +/- ##
===========================================
+ Coverage 56.58% 56.69% +0.10%
===========================================
Files 121 121
Lines 14082 14104 +22
===========================================
+ Hits 7969 7996 +27
+ Misses 5421 5416 -5
Partials 692 692
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, and thanks for the test expansion. Added a small thought RE one of the props, but is optional
if source.ChildTarget.HasValue { | ||
explainerMap["childFieldName"] = source.ChildTarget.Name | ||
} else { | ||
explainerMap["childFieldName"] = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought (non-blocking): I'm not sure this should be present - it should only reach here if summing an inline array, which doesn't have this prop available in the query syntax. Your call though, no strong feelings.
That is a reasonable thought, I raised the same question in my head too while implementing it. Thinking about it a bit, I was still unsure what the best route was so I decided to go with this approach (leaving the empty child as
|
71aa2a5
to
6b355fe
Compare
- Relevant issue(s): sourcenetwork#482. - Description Adds the attributes for `sumNode` to be included in the returned explain graph response. In this PR we are introducing 3 attributes: 1) `fieldName` 2) `childFieldName` 3) `filter` Example: - Request: ``` query @Explain { author { name _key TotalPages: _sum(books: {field: pages}) } } ``` - Response: ``` { "explain": { "selectTopNode": { "sumNode": { "sources": []{ { "fieldName": "books", "childFieldName": "pages", "filter": nil, } }, "selectNode": { "filter": null, "typeIndexJoin": { "joinType": "typeJoinMany", "rootName": "author", "root": { "scanNode": { "collectionID": "3", "collectionName": "author", "filter": null, "spans": []{ { "start": "/3", "end": "/4", } } } } "subTypeName": "books", "subType": { "selectTopNode": { "selectNode": { "filter": null, "scanNode": { "collectionID": "2", "collectionName": "book", "filter": null, "spans": []{ { "start": "/2", "end": "/3", } } } } } } } } } } } } ```
Relevant issue(s)
Resolves #482.
Description
Adds the attributes for
sumNode
to be included in the returned explain graph response.In this PR we are introducing 3 attributes:
fieldName
childFieldName
filter
Example:
Request:
Response:
Tasks
How has this been tested?
Integration tests locally and only CI. Specifically
make test
.Specify the platform(s) on which this was tested: