feat: Add ability to input simple explain type arg #1039

shahzadlone · 2023-01-17T01:29:21Z

Resolves #972

Description

This PR lays the groundwork that was missing to push the work that implements the other remaining explain types.

Usage:

@explain
@explain(type: simple)

Future Usage(not in this PR):

@explain(type: <execute|predict|debug|simple>)

PR Highlights:

Moves simple tests to default directory.
Added test for simple explain, ensure plan graph is proper.
Add documentation test for handling incorrect type argument value.
Move the plan.Close up one scope to avoid handling at every exit point.
Some minor renaming for more readability.
Add directive structure information to the request rather than just a boolean (explain or not).
Isolate parsing of directives.
Parsing of directives to happen one level up in the stack.
Update schema to accept simple value to type argument.

For Reviewers:

Apology for it being a bit harder to review as some commits aren't clean, as some definition/implementation changes span across multiple commits. Reviewing commit by commit might be more confusing that looking at the entire diff.

TODO:

Need to fix the newly introduced errors for packages that already have an errors.go file.

Tasks

I made sure the code is well commented, particularly 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?

make test

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

WSL2 Manjaro

codecov · 2023-01-17T01:36:57Z

Codecov Report

Merging #1039 (bf96071) into develop (465ca1d) will decrease coverage by 0.05%.
The diff coverage is 62.09%.

@@             Coverage Diff             @@
##           develop    #1039      +/-   ##
===========================================
- Coverage    68.16%   68.12%   -0.05%     
===========================================
  Files          171      171              
  Lines        16211    16250      +39     
===========================================
+ Hits         11050    11070      +20     
- Misses        4237     4249      +12     
- Partials       924      931       +7

Impacted Files	Coverage Δ
planner/errors.go	`0.00% <0.00%> (ø)`
query/graphql/parser/mutation.go	`79.77% <ø> (-0.45%)`	⬇️
query/graphql/parser/subscription.go	`73.33% <ø> (-1.14%)`	⬇️
query/graphql/schema/types/types.go	`100.00% <ø> (ø)`
planner/planner.go	`76.43% <55.55%> (-2.23%)`	⬇️
query/graphql/parser/query.go	`74.61% <63.29%> (-4.08%)`	⬇️
planner/explain.go	`63.79% <100.00%> (ø)`
planner/type_join.go	`72.91% <100.00%> (-0.08%)`	⬇️
... and 3 more

AndrewSisley

I've left some early feedback, I like the changes to the test structure and the planner code but have some other concerns. I'll go over the code again before approval. Will probably chat in standup regarding the exposure of stuff that is not implemented.

client/request/explain.go

client/request/request.go

planner/planner.go

tests/integration/explain/debug/basic_test.go

fredcarle · 2023-01-19T19:07:29Z

Looks good Shahzad! I'll let Andy approve once you've made the changes as he was the one who requested the changes.

AndrewSisley

Looks good Shahzad :)

client/request/request.go

query/graphql/parser/query.go

tests/integration/explain/default/basic_test.go

tests/integration/explain/default/utils.go

directory, as that is more accurate description for them and add the `invalid_type_arg_test.go` test to assert invalid explain types are caught.

store directive and their corressponding data (specific to a request).

have 4 types of explains.

where it is made, this makes it cleaner and not have us having to handle the `plan.Close()` on every exit of the function. Also remove `multiErr` function, don't need no more. Also factory handle other types of explain requests, to adhere to our tests we have added already.

…ific function and in one place.

Adhere to PR suggestion

islamaliev · 2023-04-03T15:09:02Z

Tested this one. Works fine. explain directive allows passing different types as expected

@Explain

- Resolves #972 - Description: This PR lays the groundwork that was missing to push the work that implements the other remaining explain types. - Usage: ``` @Explain @Explain(type: simple) ```

@Explain

- Resolves sourcenetwork#972 - Description: This PR lays the groundwork that was missing to push the work that implements the other remaining explain types. - Usage: ``` @Explain @Explain(type: simple) ```

shahzadlone added area/query Related to the query component area/schema Related to the schema system area/parser Related to the parser components area/testing Related to any test or testing suite area/planner Related to the planner system labels Jan 17, 2023

shahzadlone added this to the DefraDB v0.5 milestone Jan 17, 2023

shahzadlone requested a review from a team January 17, 2023 01:29

shahzadlone self-assigned this Jan 17, 2023

shahzadlone added the action/no-benchmark Skips the action that runs the benchmark. label Jan 17, 2023

shahzadlone force-pushed the lone/feat/parse-explain-argument-types branch from 59b864b to 75196aa Compare January 17, 2023 01:30

shahzadlone changed the title ~~PR - Move all our existing tests into the default explain test~~ feat: Add ability to input explain type argument Jan 17, 2023

shahzadlone added the feature New feature or request label Jan 17, 2023

shahzadlone force-pushed the lone/feat/parse-explain-argument-types branch from 75196aa to ebc32c7 Compare January 17, 2023 01:52

AndrewSisley requested changes Jan 17, 2023

View reviewed changes

shahzadlone force-pushed the lone/feat/parse-explain-argument-types branch from ebc32c7 to d7efd22 Compare January 20, 2023 17:20

AndrewSisley approved these changes Jan 20, 2023

View reviewed changes

client/request/request.go Show resolved Hide resolved

query/graphql/parser/query.go Outdated Show resolved Hide resolved

tests/integration/explain/default/basic_test.go Outdated Show resolved Hide resolved

tests/integration/explain/default/utils.go Show resolved Hide resolved

shahzadlone force-pushed the lone/feat/parse-explain-argument-types branch from d7efd22 to 83387a6 Compare January 21, 2023 02:01

shahzadlone changed the title ~~feat: Add ability to input explain type argument~~ feat: Add ability to input simple explain type arg Jan 21, 2023

shahzadlone added 10 commits January 20, 2023 21:02

PR - Move all our existing tests into the default explain test

f618ae1

directory, as that is more accurate description for them and add the `invalid_type_arg_test.go` test to assert invalid explain types are caught.

PR: Test - Add simple explain type argument test.

e4e49d1

PR: Test - Add execute explain type argument test.

0b689f1

PR: Test - Add debug explain type argument test.

2f718f8

PR: Test - Add predict explain type argument test.

27ce805

PR: Store different type of explain request types and add the ability to

bb3f64b

store directive and their corressponding data (specific to a request).

PR: Rename buildExplainGraph to buildSimpleExplainGraph since now we

1392913

have 4 types of explains.

PR: Make linter happy (some changes for compiling in following commits).

6089413

PR: Add schema directive enum like type argument.

c89432f

shahzadlone added 4 commits January 20, 2023 21:02

PR: Rework the parsing logic of directives out of each operation spec…

35d7a83

…ific function and in one place.

PR: Adhere - Remove unimplemented explain types.

53afcbc

PR: Docs - Fix Documentation and remove a block

3aa7a50

Adhere to PR suggestion

PR: Errors - Move errors to errors.go file.

bf96071

shahzadlone force-pushed the lone/feat/parse-explain-argument-types branch from 83387a6 to bf96071 Compare January 21, 2023 02:02

shahzadlone requested review from AndrewSisley and fredcarle January 21, 2023 02:03

shahzadlone merged commit 8dd7f84 into develop Jan 21, 2023

shahzadlone deleted the lone/feat/parse-explain-argument-types branch January 21, 2023 02:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ability to input simple explain type arg #1039

feat: Add ability to input simple explain type arg #1039

shahzadlone commented Jan 17, 2023 •

edited

codecov bot commented Jan 17, 2023 •

edited

AndrewSisley left a comment

fredcarle commented Jan 19, 2023

AndrewSisley left a comment

islamaliev commented Apr 3, 2023

feat: Add ability to input simple explain type arg #1039

feat: Add ability to input simple explain type arg #1039

Conversation

shahzadlone commented Jan 17, 2023 • edited

Description

For Reviewers:

TODO:

Tasks

How has this been tested?

codecov bot commented Jan 17, 2023 • edited

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

fredcarle commented Jan 19, 2023

AndrewSisley left a comment

Choose a reason for hiding this comment

islamaliev commented Apr 3, 2023

shahzadlone commented Jan 17, 2023 •

edited

codecov bot commented Jan 17, 2023 •

edited