feat: Add `typeIndexJoin` explainable attributes. #499

shahzadlone · 2022-06-03T14:36:03Z

RELEVANT ISSUE(S)

Resolves #475

DESCRIPTION

Add the remaining attributes for typeIndexJoin node that we want to see in the explainable response.

Example:

Request:

query @explain {
  author {
    _key
    name
    contact {
      email
      address {
      city
      }
    }
  }
}

Response:

{
  "explain": {
    "selectTopNode": {
      "renderNode": {
        "selectNode": {
          "filter": nil
          "typeIndexJoin": {
            "joinType":  "typeJoinOne"
            "direction": "primary"
            "rootName":  "author"
            "root": {
              "scanNode": {
                "filter":         nil
                "collectionID":   "3"
                "collectionName": "author"
                "spans": []{
                  {
                    "start": "/3"
                    "end":   "/4"
                  }
                }
              }
            }
            "subTypeName": "contact"
            "subType": {
              "selectTopNode": {
                "selectNode": {
                  "filter": nil
                  "typeIndexJoin": {
                    "joinType":  "typeJoinOne"
                    "direction": "primary"
                    "rootName":  "contact"
                    "root": {
                      "scanNode": {
                        "filter":         nil
                        "collectionID":   "4"
                        "collectionName": "authorContact"
                        "spans": []{
                          {
                            "start": "/4"
                            "end":   "/5"
                          }
                        }
                      }
                    }
                    "subTypeName": "address"
                    "subType": {
                      "selectTopNode": {
                        "selectNode": {
                          "filter": nil
                          "scanNode": {
                            "filter":         nil
                            "collectionID":   "5"
                            "collectionName": "contactAddress"
                            "spans": []{
                              {
                                "start": "/5"
                                "end":   "/6"
                              }
                            }
                          }
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }
    }
  }
}

HOW HAS THIS BEEN TESTED?

Added integration tests.

ENVIRONMENT / OS THIS WAS TESTED ON?

Please specify which of the following was this tested on (remove or add your own):

Arch Linux

codecov · 2022-06-03T14:49:20Z

Codecov Report

Merging #499 (ac22d78) into develop (954d22d) will increase coverage by 0.11%.
The diff coverage is 68.51%.

@@             Coverage Diff             @@
##           develop     #499      +/-   ##
===========================================
+ Coverage    54.20%   54.32%   +0.11%     
===========================================
  Files           97       97              
  Lines        13109    13158      +49     
===========================================
+ Hits          7106     7148      +42     
- Misses        5328     5331       +3     
- Partials       675      679       +4

Impacted Files	Coverage Δ
query/graphql/planner/explain.go	`64.40% <53.84%> (-2.99%)`	⬇️
query/graphql/planner/type_join.go	`70.29% <72.50%> (+1.21%)`	⬆️
query/graphql/planner/select.go	`75.66% <100.00%> (ø)`
query/graphql/parser/query.go	`81.87% <0.00%> (+0.90%)`	⬆️
query/graphql/planner/render.go	`88.42% <0.00%> (+3.15%)`	⬆️

AndrewSisley

Approved with a couple of non-blocking comments that would be good to get replies on. Looks good.

query/graphql/planner/explain.go

tests/integration/query/explain/utils.go

jsimnz

Just need to add in the subType plan so both the root and the subtype scannodes show up.

query/graphql/planner/type_join.go

Also Restructure root and subType explain graphs.

jsimnz

Mostly looking great!

One weird thing I noticed in the typeIndexJoin attributes for the subType field. It's returning a selectTopNode without an innter selectNode, and instead goes straight into the next node, eg scanNode or another typeIndexJoin, etc.

I'm not sure if this is a problem in the explain code or the planner itself, but based on my review, the planner just called planner.SubSelect when making the subTypes, which just calls planner.Select but deletes the render step (since we do that at the end).

Let me know what you think after looking into it, and ping me if you need some insight/help.

jsimnz · 2022-06-09T09:28:03Z

query/graphql/planner/type_join.go

+		}
+
+		// Add the attribute(s).
+		explainerMap[joinRootLabel] = joinType.subTypeFieldName


unrelated to this PR, I guess we have some naming inconsistencies here w.r.t subTypeFieldNam actually coorepsonding to joinRootLabel.

shouldn't subTypeFieldName just be renamed to rootName? or am I missing something here?

I think so. Trying to remember why it wasn't. Either way independant of this PR, so we don't need to resolve it atm

jsimnz · 2022-06-09T09:33:12Z

tests/integration/query/explain/mix_test.go

+			},
+		},
+
+		Results: []dataMap{


praise: The effort that went into all these test cases is 👌 * chefs kiss *

Gracias amigo!

tests/integration/query/explain/mix_test.go

tests/integration/query/explain/type_join_test.go

…ld be skipped.

jsimnz

LGTM!

@Explain

- RELEVANT ISSUE(S) Resolves sourcenetwork#475 - DESCRIPTION Add the remaining attributes for `typeIndexJoin` node that we want to see in the explainable response. Example: Request: ``` query @Explain { author { _key name contact { email address { city } } } } ``` Response: ``` { "explain": { "selectTopNode": { "renderNode": { "selectNode": { "filter": nil "typeIndexJoin": { "joinType": "typeJoinOne" "direction": "primary" "rootName": "author" "root": { "scanNode": { "filter": nil "collectionID": "3" "collectionName": "author" "spans": []{ { "start": "/3" "end": "/4" } } } } "subTypeName": "contact" "subType": { "selectTopNode": { "selectNode": { "filter": nil "typeIndexJoin": { "joinType": "typeJoinOne" "direction": "primary" "rootName": "contact" "root": { "scanNode": { "filter": nil "collectionID": "4" "collectionName": "authorContact" "spans": []{ { "start": "/4" "end": "/5" } } } } "subTypeName": "address" "subType": { "selectTopNode": { "selectNode": { "filter": nil "scanNode": { "filter": nil "collectionID": "5" "collectionName": "contactAddress" "spans": []{ { "start": "/5" "end": "/6" } } } } } } } } } } } } } } } } ```

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 3, 2022

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

shahzadlone requested review from orpheuslummis, jsimnz, fredcarle and AndrewSisley June 3, 2022 14:36

shahzadlone self-assigned this Jun 3, 2022

AndrewSisley approved these changes Jun 3, 2022

View reviewed changes

query/graphql/planner/explain.go Outdated Show resolved Hide resolved

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

jsimnz requested changes Jun 6, 2022

View reviewed changes

query/graphql/planner/type_join.go Outdated Show resolved Hide resolved

shahzadlone force-pushed the lone/feat/explain-type-index-join-attributes branch 4 times, most recently from 9bd91b6 to 1dcd00e Compare June 9, 2022 00:16

shahzadlone requested a review from jsimnz June 9, 2022 00:17

shahzadlone force-pushed the lone/feat/explain-type-index-join-attributes branch from 1dcd00e to 01a0b0e Compare June 9, 2022 04:30

wip: Add attributes to typeIndexJoin explain

80241c8

Also Restructure root and subType explain graphs.

shahzadlone force-pushed the lone/feat/explain-type-index-join-attributes branch from 01a0b0e to 80241c8 Compare June 9, 2022 04:35

jsimnz requested changes Jun 9, 2022

View reviewed changes

wip: Fix the issue where the immediate node under selectTopNode wou…

ac22d78

…ld be skipped.

shahzadlone requested a review from jsimnz June 9, 2022 11:59

jsimnz approved these changes Jun 9, 2022

View reviewed changes

shahzadlone merged commit 8210564 into develop Jun 10, 2022

shahzadlone deleted the lone/feat/explain-type-index-join-attributes branch June 10, 2022 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `typeIndexJoin` explainable attributes. #499

feat: Add `typeIndexJoin` explainable attributes. #499

shahzadlone commented Jun 3, 2022 •

edited

Loading

codecov bot commented Jun 3, 2022 •

edited

Loading

AndrewSisley left a comment

jsimnz left a comment

jsimnz left a comment

jsimnz Jun 9, 2022

shahzadlone Jun 9, 2022

jsimnz Jun 9, 2022

jsimnz Jun 9, 2022

shahzadlone Jun 9, 2022

jsimnz left a comment

feat: Add typeIndexJoin explainable attributes. #499

feat: Add typeIndexJoin explainable attributes. #499

Conversation

shahzadlone commented Jun 3, 2022 • edited Loading

RELEVANT ISSUE(S)

DESCRIPTION

HOW HAS THIS BEEN TESTED?

ENVIRONMENT / OS THIS WAS TESTED ON?

codecov bot commented Jun 3, 2022 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

jsimnz Jun 9, 2022

Choose a reason for hiding this comment

shahzadlone Jun 9, 2022

Choose a reason for hiding this comment

jsimnz Jun 9, 2022

Choose a reason for hiding this comment

jsimnz Jun 9, 2022

Choose a reason for hiding this comment

shahzadlone Jun 9, 2022

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

feat: Add `typeIndexJoin` explainable attributes. #499

feat: Add `typeIndexJoin` explainable attributes. #499

shahzadlone commented Jun 3, 2022 •

edited

Loading

codecov bot commented Jun 3, 2022 •

edited

Loading