feat: Add ability to explain-debug all nodes #1563

shahzadlone · 2023-06-08T16:14:02Z

Resolves #948

Description

Implements a new type of explain called debug which dumps all plan nodes (even non-explainable nodes) as a graph (has no attributes).

Usage is similar to other explain types, but would just do @explain(type: debug) instead of @explain(type: simple) or @explain(type: execute).

Example Request:

query @explain(type: debug) {
	Author (groupBy: [age]) {
		age
		_group {
			name
		}
	}
}

Response:

{
	"data": [
		"explain": {
			"selectTopNode": {
				"groupNode": {
					"selectNode": {
						"pipeNode": {
							"scanNode": {}
						}
					}
				}
			}
		}
	]
}

For reviewers

2nd commit is the one that needs to be thoroughly reviewed, rest is all tests.

How has this been tested?

Integration tests

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

WSL2 (Manjaro)

codecov · 2023-06-08T16:23:37Z

Codecov Report

Patch coverage: 66.67% and project coverage change: -0.01 ⚠️

Comparison is base (ac5d7b8) 72.29% compared to head (1e186cc) 72.28%.

@@             Coverage Diff             @@
##           develop    #1563      +/-   ##
===========================================
- Coverage    72.29%   72.28%   -0.01%     
===========================================
  Files          185      185              
  Lines        18389    18477      +88     
===========================================
+ Hits         13294    13355      +61     
- Misses        4055     4074      +19     
- Partials      1040     1048       +8

Flag	Coverage Δ
all-tests	`72.28% <66.67%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
planner/type_join.go	`74.87% <ø> (-0.07%)`	⬇️
request/graphql/schema/types/types.go	`100.00% <ø> (ø)`
planner/explain.go	`58.99% <65.91%> (+2.97%)`	⬆️
request/graphql/parser/request.go	`73.12% <100.00%> (+0.34%)`	⬆️

... and 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac5d7b8...1e186cc. Read the comment docs.

AndrewSisley · 2023-06-09T16:48:51Z

thought: It feels quite odd to me that debug explain outputs less stuff than simple explain. Why would we not want to output the attributes for the nodes that have them?

shahzadlone · 2023-06-09T17:46:29Z

thought: It feels quite odd to me that debug explain outputs less stuff than simple explain. Why would we not want to output the attributes for the nodes that have them?

We already have access to the attributes with the simple explain call. The debug explain (atleast the first iteration of it) should visualize the full plan graph nodes. i.e. the pipeNode and multiScanNode.

However this makes me think that similar to how we were gunna add an option to execute explain like showSimpleAttributes to enable showing simple explain attributes for execute explain nodes, we can in future do the same thing for debug as well.

AndrewSisley · 2023-06-09T21:24:54Z

thought: It feels quite odd to me that debug explain outputs less stuff than simple explain. Why would we not want to output the attributes for the nodes that have them?

We already have access to the attributes with the simple explain call. The debug explain (atleast the first iteration of it) should visualize the full plan graph nodes. i.e. the pipeNode and multiScanNode.

However this makes me think that similar to how we were gunna add an option to execute explain like showSimpleAttributes to enable showing simple explain attributes for execute explain nodes, we can in future do the same thing for debug as well.

I'm not sure you are understanding me. Typically debug provides the same information as simple/info/default/etc, and more, additional info (never losing the simple info in the debug). It would also annoy me as a user if I couldnt get the full graph with attributes from a single explain call.

shahzadlone · 2023-06-12T07:58:55Z

Typically debug provides the same information as simple/info/default/etc, and more, additional info (never losing the simple info in the debug).

Is "simple" an actual log level? I have never seen that, if it is then I can see the confusion. The different explain types aren't log/verbosity levels, they do different things. For example execute explain will return the execution information, simple will return the attributes and only the explain nodes. Upon thinking further I would be okay to name this to something else, maybe type: internal, this explain should be very rarely used by non-internal-devs as the graph includes nodes that are too into the weeds of defradb.

It would also annoy me as a user if I couldnt get the full graph with attributes from a single explain call.

That will be possible though for both execute and this explain with showSimpleAttributes.

AndrewSisley · 2023-06-12T13:35:01Z

Is "simple" an actual log level?

Simple explain is pretty analogous to info log levels. The split in both serves exactly the same purpose 99% of the time.

That will be possible though for both execute and this explain with showSimpleAttributes.

What is the point of multiple types of explain, if you have to decorate them all with attributes to make them useful? Why not have one type of explain with all the attributes?

shahzadlone · 2023-06-12T21:10:17Z

@AndrewSisley made a ticket to reflect our standup discussion: #1570

for default explain that was missing. This commit can be safely ignored as it is not related to this PR.

Debug explain has no attributes, so we just build it externally using the `Kind()` calls. Note: This explain also includes non-explainable nodes.

AndrewSisley

Is a mad amount of tests that you have managed to write for this, I skimmed through them and they all appeared sensible to have - thanks a bunch for adding them all :)

Prod code looked good too, minus the feature-change requested RE type that we discussed in the standup. Just added an out of scope suggestion for the future that I spotted whilst reading.

AndrewSisley · 2023-06-12T22:52:37Z

planner/explain.go

 	idsLabel            = "ids"
+	joinRootLabel       = "root"
+	joinSubTypeLabel    = "subType"
+	keysLabel           = "_keys"
 	limitLabel          = "limit"
 	offsetLabel         = "offset"
 	sourcesLabel        = "sources"
 	spansLabel          = "spans"


suggestion (out of scope): These are all public, and I think they should live in client/request/explain.go - might be worth adding a ticket if you agree.

Same also goes for the attribute tags and node names (if they are in planner atm)

I appreciate this suggestion! Would be much nicer. Will open a ticket.

ticket: #1571

islamaliev · 2023-07-19T11:50:03Z

bug bash result:
ran local defradb node, create different collection with relationships.
Used Altair GraphQL client to query many different types of queries and mutations (filters, groups, orders, limits and others).
For all queries got a corresponding explain graph

@Explain

Resolves sourcenetwork#948 ## Description Implements a new type of explain called `debug` which dumps all plan nodes (even non-explainable nodes) as a graph (has no attributes). Usage is similar to other explain types, but would just do `@explain(type: debug)` instead of `@explain(type: simple)` or `@explain(type: execute)`. Example Request: ``` query @Explain(type: debug) { Author (groupBy: [age]) { age _group { name } } } ``` Response: ``` { "data": [ "explain": { "selectTopNode": { "groupNode": { "selectNode": { "pipeNode": { "scanNode": {} } } } } } ] } ```

shahzadlone added feature New feature or request code quality Related to improving code quality area/planner Related to the planner system labels Jun 8, 2023

shahzadlone added this to the DefraDB v0.6 milestone Jun 8, 2023

shahzadlone requested a review from a team June 8, 2023 16:14

shahzadlone self-assigned this Jun 8, 2023

shahzadlone changed the title ~~feat: Add ability to explain all nodes (debug)~~ feat: Add ability to explain-debug all nodes Jun 8, 2023

shahzadlone added 14 commits June 12, 2023 17:11

PR(UNRELATED): Add ExpectedPatterns check in test

c3cd77e

for default explain that was missing. This commit can be safely ignored as it is not related to this PR.

PR(MAIN): Implement debug explain

1b848f8

Debug explain has no attributes, so we just build it externally using the `Kind()` calls. Note: This explain also includes non-explainable nodes.

PR(TEST): Add basic test + fixture

105b596

PR(TEST): Add type join tests

6080924

PR(TEST): Add mutation tests

4a4ece1

PR(TEST): Add dagscan tests

2a3761b

PR(TEST): Add filter tests

6a7b5ed

PR(TEST): Add order tests

b995280

PR(TEST): Add limit tests

dba5559

PR(TEST): Add count tests

5a1baf0

PR(TEST): Add sum tests

2d11954

PR(TEST): Add average tests

6b4111b

PR(TEST): Add top level tests

8cb30b5

PR(TEST): Add group tests

1e186cc

shahzadlone force-pushed the feat/debug-explain branch from 10bbbbb to 1e186cc Compare June 12, 2023 21:11

AndrewSisley approved these changes Jun 12, 2023

View reviewed changes

shahzadlone mentioned this pull request Jun 13, 2023

Move public explain stuff to client/request/explain.go #1571

Open

shahzadlone merged commit c9e4355 into sourcenetwork:develop Jun 13, 2023

shahzadlone deleted the feat/debug-explain branch June 13, 2023 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ability to explain-debug all nodes #1563

feat: Add ability to explain-debug all nodes #1563

shahzadlone commented Jun 8, 2023 •

edited

Loading

codecov bot commented Jun 8, 2023 •

edited

Loading

AndrewSisley commented Jun 9, 2023

shahzadlone commented Jun 9, 2023

AndrewSisley commented Jun 9, 2023

shahzadlone commented Jun 12, 2023

AndrewSisley commented Jun 12, 2023 •

edited

Loading

shahzadlone commented Jun 12, 2023

AndrewSisley left a comment

AndrewSisley Jun 12, 2023

shahzadlone Jun 13, 2023

shahzadlone Jun 13, 2023

islamaliev commented Jul 19, 2023

feat: Add ability to explain-debug all nodes #1563

feat: Add ability to explain-debug all nodes #1563

Conversation

shahzadlone commented Jun 8, 2023 • edited Loading

Description

For reviewers

How has this been tested?

codecov bot commented Jun 8, 2023 • edited Loading

Codecov Report

AndrewSisley commented Jun 9, 2023

shahzadlone commented Jun 9, 2023

AndrewSisley commented Jun 9, 2023

shahzadlone commented Jun 12, 2023

AndrewSisley commented Jun 12, 2023 • edited Loading

shahzadlone commented Jun 12, 2023

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley Jun 12, 2023

Choose a reason for hiding this comment

shahzadlone Jun 13, 2023

Choose a reason for hiding this comment

shahzadlone Jun 13, 2023

Choose a reason for hiding this comment

islamaliev commented Jul 19, 2023

shahzadlone commented Jun 8, 2023 •

edited

Loading

codecov bot commented Jun 8, 2023 •

edited

Loading

AndrewSisley commented Jun 12, 2023 •

edited

Loading