Skip to content
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 the response plan. #385

Merged
merged 24 commits into from
Jun 1, 2022

Conversation

shahzadlone
Copy link
Member

@shahzadlone shahzadlone commented Apr 29, 2022

Related Issues

Resolves #325,
Resolved #477 (as CommitSelectNode for now doesn't have any explainable attribute)
Closes #361, Closes #362 and also Closes #363 (Closing the TEXT structured ones because we have decided to only go with JSON approach to avoid the wonky looking client side formatting).

Fixes #486 in commit d940f7b2e6ee54d0ce8b4bd8f5330db60307630f

Description

This PR lays all the ground work needed to have the opt-ability for planner nodes to subscribe to the explain interface if they would like to be exposed to the explaining of the plan graph.

Even though top-level plan node name will be explained after this PR for all explainable nodes, this PR doesn't implement all the explain attributes for all explainable nodes. Here are the planNodes whose attributes can be explained after this PR:

  1. selectNode (attributes completed)
  2. selectTopNode (attributes completed / has no attributes)
  3. createNode (attributes completed)
  4. deleteNode (attributes completed)
  5. scanNode (some attributes completed)

Request:

query @explain {
 user {
   _key
   age
   name
 }
}

Response:

{
 "data": [
   {
     "explain": {
       "selectTopNode": {
         "selectNode": {
           "filter": null,
           "scanNode": {
             "collectionID": "1",
             "collectionName": "user",
             "filter": null
           }
         }
       }
     }
   }
 ]
}

The other attributes that need to be added are now tracked as sub-tasks / check-list items to #35. They are going to be their own PRs to keep this PR consumable and in order to also avoid having conflicts:

query/graphql/schema/manager.go Outdated Show resolved Hide resolved
query/graphql/schema/manager.go Outdated Show resolved Hide resolved
@shahzadlone shahzadlone self-assigned this Apr 30, 2022
@shahzadlone shahzadlone added action/no-benchmark Skips the action that runs the benchmark. feature New feature or request DO NOT MERGE labels Apr 30, 2022
@shahzadlone shahzadlone added this to the DefraDB v0.3 milestone Apr 30, 2022
This was linked to issues Apr 30, 2022
@shahzadlone shahzadlone force-pushed the lone/i-325/simple-explain-in-json branch from 5ea6914 to e858e5e Compare April 30, 2022 14:09
@sourcenetwork sourcenetwork deleted a comment from source-devs Apr 30, 2022
@shahzadlone shahzadlone force-pushed the lone/i-325/simple-explain-in-json branch from e858e5e to 88857c0 Compare April 30, 2022 14:47
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

Few thoughts, some just suggestions or comments. One notable structural change on the planNode interface.

db/container/container.go Outdated Show resolved Hide resolved
query/graphql/parser/query.go Show resolved Hide resolved
query/graphql/parser/query.go Outdated Show resolved Hide resolved
query/graphql/planner/planner.go Outdated Show resolved Hide resolved
query/graphql/planner/planner.go Show resolved Hide resolved
query/graphql/planner/planner.go Outdated Show resolved Hide resolved
query/graphql/planner/planner.go Outdated Show resolved Hide resolved
@shahzadlone shahzadlone force-pushed the lone/i-325/simple-explain-in-json branch 5 times, most recently from 0a295ed to d1101e1 Compare May 17, 2022 06:36
@sourcenetwork sourcenetwork deleted a comment from codecov bot May 17, 2022
query/graphql/parser/parser.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/planner.go Outdated Show resolved Hide resolved
@shahzadlone shahzadlone force-pushed the lone/i-325/simple-explain-in-json branch from bb8fdb7 to c5656b0 Compare May 17, 2022 15:32
@shahzadlone shahzadlone marked this pull request as ready for review May 17, 2022 15:36
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Partial review - will continue later

query/graphql/parser/query.go Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Outdated Show resolved Hide resolved
query/graphql/planner/planner.go Outdated Show resolved Hide resolved
query/graphql/planner/planner.go Show resolved Hide resolved
query/graphql/planner/planner.go Show resolved Hide resolved
@shahzadlone shahzadlone force-pushed the lone/i-325/simple-explain-in-json branch from ed129d3 to a2cb0fd Compare May 31, 2022 04:51
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

Looks great! Everying has mostly been taken care of.

Omitting my comments about the multiscanNode stuff since we discussed that on discord. (TLDR: multiscanNode embedding *scanNode caused issue :/)

Only notable issue is the assumed allocation of map[string]interface{} in Explain() methods. Basically, if a node is explainable, but doens't care to emit attributes, it should be fine to return a nil map. But currently, that will crash the DB since it assumes it returns a non-nil map.

query/graphql/parser/query.go Outdated Show resolved Hide resolved
query/graphql/parser/types/types.go Show resolved Hide resolved
query/graphql/planner/types/types.go Outdated Show resolved Hide resolved
query/graphql/planner/explain.go Show resolved Hide resolved
@shahzadlone shahzadlone requested a review from jsimnz June 1, 2022 07:49
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

LGTM

@shahzadlone shahzadlone merged commit 257bf09 into develop Jun 1, 2022
@shahzadlone shahzadlone deleted the lone/i-325/simple-explain-in-json branch June 1, 2022 07:58
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
- RELATED ISSUES:
Resolves sourcenetwork#325,
Closes sourcenetwork#361, Closes sourcenetwork#362 and also Closes sourcenetwork#363 (Closing the `TEXT` structured ones because we have decided to only go with JSON approach to avoid the wonky looking client side formatting).
Fixes sourcenetwork#486 in commit [d940f7b](sourcenetwork@d940f7b)

- DESCRIPTION:
This PR lays all the ground work needed to have the opt-ability for planner nodes to subscribe to the explain interface if they would like to be exposed to the explaining of the plan graph.

Even though top-level plan **node** name will be explained after this PR for all explainable nodes, this PR doesn't implement all the explain attributes for all explainable nodes. Here are the planNodes whose attributes can be explained after this PR:
1) `selectNode` (attributes completed)
2) `selectTopNode` (attributes completed / has no attributes)
3) `createNode` (attributes completed)
4) `deleteNode` (attributes completed)
5) `scanNode` (some attributes completed)

 Request:
 ```
query @Explain {
  user {
    _key
    age
    name
  }
}
```

 Response:
 ```
{
  "data": [
    {
      "explain": {
        "selectTopNode": {
          "selectNode": {
            "filter": null,
            "scanNode": {
              "collectionID": "1",
              "collectionName": "user",
              "filter": null
            }
          }
        }
      }
    }
  ]
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/parser Related to the parser components area/query Related to the query component area/schema Related to the schema system feature New feature or request
Projects
None yet
4 participants