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.

Sign up for GitHub

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

Jump to bottom

fix: explain query plan formatting #147

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: explain query plan formatting #147

wants to merge 4 commits into from

Conversation

douglasmdev
Copy link
Contributor

@douglasmdev douglasmdev commented Aug 14, 2023 •
edited
Loading

Description

Adds an ASCII tree representation to the output of EXPLAIN QUERY PLAN ...

Related Issues

Visual reference

Here's an example

→ CREATE TABLE t1 (t TEXT);
→ CREATE INDEX i1 ON t1 (t);
→ CREATE TABLE t2 (v text);
→ CREATE INDEX i2 ON t2 (v);
→  SELECT 1; EXPLAIN QUERY PLAN SELECT t FROM t1 UNION SELECT v FROM t2; SELECT 2;
1 
1     
QUERY PLAN
COMPOUND QUERY
  LEFT-MOST SUBQUERY
    SCAN t1
  UNION USING TEMP B-TREE
    SCAN t2
2 
2

@douglasmdev
feat(statement-result): add query field
675c70d 
- in order to keep record of which query generated each result
@douglasmdev douglasmdev force-pushed the fix/explain-query-plan-formatting branch from 474ea15 to 6da726b Compare August 14, 2023 18:26
@douglasmdev douglasmdev force-pushed the fix/explain-query-plan-formatting branch 3 times, most recently from 63b2ca9 to 57ea872 Compare August 14, 2023 20:45
luisfvieirasilva
luisfvieirasilva reviewed Aug 15, 2023
View reviewed changes
internal/db/db.go Outdated
Comment on lines 228 to 232
for _, query := range queries {
if shouldContinue := readQueryResultSet(queryRows, statementResultCh, query); !shouldContinue {
return false
}

hasResultSetToRead = queryRows.NextResultSet()
hasResultSetToRead = queryRows.NextResultSet()
}
Copy link

Choose a reason for hiding this comment

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

You don't wanna to iterate twice here

internal/db/utils.go Outdated
var explainQueryPlanColumnNames = []string{"id", "parent", "notused", "detail"}

func queryContainsExplainQueryPlanStatement(query string) bool {
return strings.Contains(
Copy link

Choose a reason for hiding this comment

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

Maybe a StartsWith is safer now, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the docs: https://www.sqlite.org/lang_explain.html
EXPLAIN QUERY PLAN always appears in the beggining of statements, bug I agree that having some extra safety woud be nice.

internal/db/utils.go Outdated
return false
}
for _, colName := range explainQueryPlanColumnNames {
if !slices.Contains(colNames, colName) {
Copy link

Choose a reason for hiding this comment

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

I think these ones we can match exactly, not with contains

@douglasmdev douglasmdev force-pushed the fix/explain-query-plan-formatting branch from 57ea872 to 853980b Compare August 15, 2023 11:54
@douglasmdev douglasmdev force-pushed the fix/explain-query-plan-formatting branch from 853980b to 3eff2e6 Compare August 15, 2023 14:55
@douglasmdev douglasmdev force-pushed the fix/explain-query-plan-formatting branch from 3eff2e6 to 8b0663d Compare August 15, 2023 22:18
@douglasmdev douglasmdev marked this pull request as ready for review August 15, 2023 22:27
WilsonNet
WilsonNet reviewed Oct 4, 2023
View reviewed changes
test/db_root_command_shell_test.go
_, _, err := s.tc.Execute("CREATE TABLE fake_explain (ID INTEGER PRIMARY KEY, PARENT INTEGER, NOTUSED INTEGER, DETAIL TEXT);")
s.tc.Assert(err, qt.IsNil)

outS, errS, err := s.tc.ExecuteShell([]string{"SELECT * FROM fake_explain;"})
Copy link
Contributor

@WilsonNet WilsonNet Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

shouldn't you be testing the EXPLAI QUERY PLAN command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is testing that a table with the same column names as the EQP table will still be treated as a normal table (which it should).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luisfvieirasilva luisfvieirasilva luisfvieirasilva left review comments

@WilsonNet WilsonNet WilsonNet left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve the output of EXPLAIN QUERY PLAN
3 participants
@douglasmdev @luisfvieirasilva @WilsonNet