feat: Add ability to explain `sortNode` attribute(s). #558

shahzadlone · 2022-06-24T06:38:03Z

Relevant issue(s)

Resolves #481.
Fixes #584 ([2] case).

Description

Adds the attributes for sortNode to be included in the returned explain graph response.
So far, we are only introducing 1 attribute, which represents a list containing all the orderings of each field requested to be sorted, with its corresponding direction.

Example:

Request:

query @explain {
  author(order: {age: ASC}) {
    name
    age
    verified
  }
}

Response:

{
  "explain": {
    "selectTopNode": {
      "sortNode": {
        "selectNode": {
          "filter": null,
          "scanNode": {
            "filter":         null,
            "collectionID":   "3",
            "collectionName": "author",
            "spans": []{
              {
                "start": "/3",
                "end":   "/4",
              }
            }
          }
        }
        "orderings": []{
          {
            "direction": "ASC",
            "fields":     [ "age" ],
          }
        }
      }
    }
  }
}

Limitations:

Found sortNode.ordering is populated by empty elements #589

Tasks

I made sure the code is well commented, particularly in hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Integration tests locally and only CI. Specifically make test.

Specify the platform(s) on which this was tested:

Manjaro Flavor of Arch Linux Running in WSL2 on Windows 11

codecov · 2022-06-24T06:48:42Z

Codecov Report

Merging #558 (4857a7b) into develop (70c8363) will increase coverage by 0.13%.
The diff coverage is 90.47%.

@@             Coverage Diff             @@
##           develop     #558      +/-   ##
===========================================
+ Coverage    55.95%   56.08%   +0.13%     
===========================================
  Files          121      121              
  Lines        14191    14231      +40     
===========================================
+ Hits          7940     7982      +42     
+ Misses        5553     5549       -4     
- Partials       698      700       +2

Impacted Files	Coverage Δ
query/graphql/planner/planner.go	`73.58% <0.00%> (ø)`
query/graphql/planner/sort.go	`83.50% <86.95%> (+7.50%)`	⬆️
core/doc.go	`94.39% <100.00%> (+0.77%)`	⬆️
query/graphql/mapper/targetable.go	`58.62% <100.00%> (+8.62%)`	⬆️

fredcarle

Approving assuming you address the 2 formatting points I've raised.

tests/integration/query/explain/with_sort_test.go

AndrewSisley

Got a couple of comments on the tests but nothing too serious. Looks good!

tests/integration/query/explain/with_sort_test.go

todo: Still have to add the test where you can sort/order the parent by its child.

AndrewSisley

I think #584 should be solved here as it looks like an explain bug, but otherwise looks good

AndrewSisley · 2022-07-04T13:41:39Z

query/graphql/mapper/targetable.go

@@ -106,6 +106,13 @@ type OrderCondition struct {
 	Direction SortDirection
 }

+func (oc OrderCondition) IsEmpty() bool {
+	if oc.Direction == "" && len(oc.FieldIndexes) == 0 {


suggestion: I think oc.Direction == "" is not useful and this should only check len(oc.FieldIndexes). If it is impossible for only one of these to be true, then it is misleading and wasteful, if it is possible for direction to be empty and field indexes to be populated then that IMO is something that the sortNode could handle (could be a default direction for example).

I would like to check for Direction because if it is not empty then would like to have it dumped, this check was mainly put because I noticed that sortNode.ordering has a bunch of empty elements (i.e. elements in ordering having both Direction == "" and len(FieldIndexes) == 0). Can be handled inside sortNode for sure, I find more clarity having this IsEmpty function handle that but don't mind moving it into sortNode if you have a strong preference for it.

core/doc.go

tests/integration/query/explain/with_sort_test.go

function also look and ChildMappings to find the name of the index, Add the test that was failing before, also make `TryToFindNameFromIndex` now follow the boolean try pattern instead of returning error.

AndrewSisley

Looks good Shahzad. I do still disagree RE the IsEmpty stuff, but that is unimportant. I alse see tracking down why there are empties there as out of scope.

AndrewSisley · 2022-07-05T13:45:38Z

query/graphql/planner/planner.go

@@ -455,7 +455,7 @@ func (p *Planner) explainRequest(

 	explainGraph, err := buildExplainGraph(plan)
 	if err != nil {
-		return nil, err
+		return nil, multiErr(err, plan.Close())


:) This smells like the similar Close issue I saw a while ago. Good find

@Explain

) - Relevant issue(s): Resolves sourcenetwork#481. Fixes sourcenetwork#584 ([2] case). - Description Adds the attributes for `sortNode` to be included in the returned explain graph response. So far, we are only introducing 1 attribute, which represents a list containing all the `orderings` of each field requested to be sorted, with its corresponding direction. - Request: ``` query @Explain { author(order: {age: ASC}) { name age verified } } ``` - Response: ``` { "explain": { "selectTopNode": { "sortNode": { "selectNode": { "filter": null, "scanNode": { "filter": null, "collectionID": "3", "collectionName": "author", "spans": []{ { "start": "/3", "end": "/4", } } } } "orderings": []{ { "direction": "ASC", "fields": [ "age" ], } } } } } } ```

shahzadlone added feature New feature or request area/query Related to the query component action/no-benchmark Skips the action that runs the benchmark. labels Jun 24, 2022

shahzadlone added this to the DefraDB v0.3 milestone Jun 24, 2022

shahzadlone requested a review from a team June 24, 2022 06:38

shahzadlone self-assigned this Jun 24, 2022

shahzadlone force-pushed the lone/explain-sort-node-attributes branch 2 times, most recently from 4572a28 to 3512e0c Compare June 24, 2022 06:46

fredcarle approved these changes Jun 24, 2022

View reviewed changes

tests/integration/query/explain/with_sort_test.go Outdated Show resolved Hide resolved

tests/integration/query/explain/with_sort_test.go Outdated Show resolved Hide resolved

AndrewSisley requested changes Jun 24, 2022

View reviewed changes

shahzadlone force-pushed the lone/explain-sort-node-attributes branch 3 times, most recently from 3512e0c to 596f6fd Compare July 2, 2022 06:10

shahzadlone added 3 commits July 2, 2022 02:40

wip: Add the sortNode test that is to be satisfied.

2169f82

wip: Add implementation of explaining sortNode attribute(s).

9c4cbd4

wip: Find all the names using indexes of all the ordering fields.

31ba024

shahzadlone force-pushed the lone/explain-sort-node-attributes branch from 596f6fd to 31ba024 Compare July 2, 2022 06:41

shahzadlone mentioned this pull request Jul 2, 2022

bug: Panic upon explain request of parent being ordered by child #584

Closed

wip: Adhere to code review.

e669138

todo: Still have to add the test where you can sort/order the parent by its child.

shahzadlone force-pushed the lone/explain-sort-node-attributes branch from fb5e08f to e669138 Compare July 2, 2022 09:40

shahzadlone requested review from AndrewSisley, jsimnz and fredcarle July 2, 2022 09:40

AndrewSisley requested changes Jul 4, 2022

View reviewed changes

wip: Fix the Discard() bug, and make the TryToFindNameFromIndex

4857a7b

function also look and ChildMappings to find the name of the index, Add the test that was failing before, also make `TryToFindNameFromIndex` now follow the boolean try pattern instead of returning error.

shahzadlone force-pushed the lone/explain-sort-node-attributes branch from 556cd1c to 4857a7b Compare July 5, 2022 13:29

shahzadlone requested a review from AndrewSisley July 5, 2022 13:31

AndrewSisley approved these changes Jul 5, 2022

View reviewed changes

shahzadlone mentioned this pull request Jul 5, 2022

sortNode.ordering is populated by empty elements #589

Closed

shahzadlone merged commit c3c6495 into develop Jul 5, 2022

shahzadlone deleted the lone/explain-sort-node-attributes branch July 5, 2022 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ability to explain `sortNode` attribute(s). #558

feat: Add ability to explain `sortNode` attribute(s). #558

shahzadlone commented Jun 24, 2022 •

edited

Loading

codecov bot commented Jun 24, 2022 •

edited

Loading

fredcarle left a comment

AndrewSisley left a comment

AndrewSisley left a comment

AndrewSisley Jul 4, 2022

shahzadlone Jul 5, 2022 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Jul 5, 2022

feat: Add ability to explain sortNode attribute(s). #558

feat: Add ability to explain sortNode attribute(s). #558

Conversation

shahzadlone commented Jun 24, 2022 • edited Loading

Relevant issue(s)

Description

Example:

Request:

Response:

Limitations:

Tasks

How has this been tested?

codecov bot commented Jun 24, 2022 • edited Loading

Codecov Report

fredcarle left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley Jul 4, 2022

Choose a reason for hiding this comment

shahzadlone Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley Jul 5, 2022

Choose a reason for hiding this comment

feat: Add ability to explain `sortNode` attribute(s). #558

feat: Add ability to explain `sortNode` attribute(s). #558

shahzadlone commented Jun 24, 2022 •

edited

Loading

codecov bot commented Jun 24, 2022 •

edited

Loading

shahzadlone Jul 5, 2022 •

edited

Loading