ui: Align db console logical plan with sql shell

[xinhaoz]

Resolves: #67328

Release note (ui change): The 'Logical Plan' tab in db console
has been renamed 'Explain Plan' and the plan format displayed
has been updated to match the output of the EXPLAIN command in
the SQL shell. Global EXPLAIN properties have been added to
the logical plan in db-console which were previously missing.

The EXPLAIN format shown below should now be be reflected in db console:

  distribution: full
  vectorized: true

  • hash join
  │ estimated row count: 503
  │ equality: (rider_id) = (id)
  │
  ├── • scan
  │     estimated row count: 513 (100% of the table; stats collected 9 seconds ago)
  │     table: rides@primary
  │     spans: FULL SCAN
  │
  └── • scan
        estimated row count: 50 (100% of the table; stats collected 1 minute ago)
        table: users@primary
        spans: FULL SCAN

---

previous db-console view:

updated db-console Explain Tab of above:

[cockroach-teamcity]

This change is 

[maryliag]

Great work on this! I just have a few comments :)

Reviewed 5 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

---

pkg/ui/cluster-ui/src/core/typography.module.scss, line 21 at r1 (raw file):

$font-family--bold: SourceSansPro-Bold;
$font-family--semi-bold: SourceSansPro-SemiBold;
$font-family--monospace: RobotoMono-Regular;

is this being used only on the details page? Have you checked if this didn't affect the visual of other pages?

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 126 at r1 (raw file):

// flattenTreeAttributes takes a tree representation of a logical
// plan (IExplainTreePlanNode) and flattens the attributes in each node.
// (see flattenAttributes)

nit: can you add the example of a flatten tree attributes, like we had previously?

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.module.scss, line 107 at r1 (raw file):

  }

  div[id="plan-view-inner-container"] > ul > li {

nit: add a new class to the component, instead of using a very specific id. With ids it gets harder to maintain and also we can't re-use

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.spec.tsx, line 71 at r1 (raw file):

];

const treePlanWithSingleChildPaths: IExplainTreePlanNode = {

since you removed the flatterTree function, but created a new one (flattenTreeAttributes), you can update this values to test your new function

[Azhng]

Do you mind posting the screenshot of the DB console logical plan from before this change too as well ?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 150 at r1 (raw file):

/* ************************* PLAN NODES ************************* */

function NodeAttribute({ attribute }: { attribute: FlatPlanNodeAttribute }) {

nit: annotate it with return type? also this seems like a new syntax that I haven't seen before 🤔 . What does func_name( { param }: { param: type } syntax mean?

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 183 at r1 (raw file):

        <div className={cx("nodeAttributes")}>
          {node.attrs.map((attr, idx) => (
            <NodeAttribute key={idx} attribute={attr} />

i'm a bit confused, key param is not defined in NodeAttribute function earlier? How does this work?

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 213 at r1 (raw file):

}

function PlanNode({ node }: PlanNodeProps) {

ditto: return type annotation

[xinhaoz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

---

pkg/ui/cluster-ui/src/core/typography.module.scss, line 21 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

is this being used only on the details page? Have you checked if this didn't affect the visual of other pages?

Oh yes, I didn't notice it was being used in a few other places. Added the RobotoMono-Regular font as a separate var.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 126 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: can you add the example of a flatten tree attributes, like we had previously?

Hmm, I'm not sure how useful that would be, since this function just applies flattenAttributes to all nodes in the tree (there is san example of what flattenAttributes does at its declaration). This function no longer structurally changes the tree so should I still include an example in the comments? It would just show that each node's attributes are flattened like in flattenAttributes comment.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 150 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: annotate it with return type? also this seems like a new syntax that I haven't seen before 🤔 . What does func_name( { param }: { param: type } syntax mean?

It's just some destructuring syntax, similar to if you were to do const {myProp} = props; in the function body. The second half is type annotation. I can lift that out into an object type for clarity, eg.

type NodeAttributeProps = { param: type ; };
func_name( { param }: NodeAttributeProps)

Let me know which you think is preferred, (I inlined i there since it was just the one prop).

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 183 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

i'm a bit confused, key param is not defined in NodeAttribute function earlier? How does this work?

I think the key prop is included in React elements since it's needed for situations like this when each element in the list needs a unique key for some react DOM magic.. not too sure how that works, though.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 213 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

ditto: return type annotation

Done.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.spec.tsx, line 71 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

since you removed the flatterTree function, but created a new one (flattenTreeAttributes), you can update this values to test your new function

Done.

[Azhng]

Reviewed 3 of 6 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 150 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

It's just some destructuring syntax, similar to if you were to do const {myProp} = props; in the function body. The second half is type annotation. I can lift that out into an object type for clarity, eg.

type NodeAttributeProps = { param: type ; };
func_name( { param }: NodeAttributeProps)

Let me know which you think is preferred, (I inlined i there since it was just the one prop).

I see. My personal preference would be always spells out the this.props, but it would be up to you.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 183 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

I think the key prop is included in React elements since it's needed for situations like this when each element in the list needs a unique key for some react DOM magic.. not too sure how that works, though.

Is it possible you can add key props to the params of the NodeAttribute? Or that's disallowed by React?

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 113 at r2 (raw file):

  if (dist.numerator === 0) return "local";

  return dist.numerator === dist.denominator ? "full" : "partial";

Perhaps add comment here detailing what does full and partial mean here exactly? The backend API only returns a boolean on whether or not a plan is distributed or not. It's not immediately clear to me how we went from a boolean value to inferring whether a plan is fully/partially distributed ?

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 121 at r2 (raw file):

  }

  return (vec.numerator > 0).toString();

nit: This is more clear to me compared to the distributionToString function, but would be nice to also include some comments here.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 226 at r2 (raw file):

      <PlanNodeDetails node={node} />
      {node.children &&
        node.children.map((child, idx) => <PlanNodes node={child} key={idx} />)}

Hmm. this feels a bit strange to me, PlanNode and PlanNodes recursively calls each other, which means it will generates DOM that has alternating <ul> and <li> tags where each unordered list would always only contain 1 element.

<ul>
  <li>
     <ul>
       <li>
       </li>
     <ul>
  </li>
</ul>

I'm not sure if this is a common React practice in this case?

Since now that PlanNodes can only contain 1 element, why not just remove PlanNodes directly and have PlanNode render both the <ul> tag as well as <li> tag.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 282 at r2 (raw file):

        key: "distribution",
        values: [distributionToString(globalProperties.distribution)],
        warn: false,

is this value always hard coded ?

[maryliag]

Reviewed 3 of 6 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)

---

pkg/ui/cluster-ui/src/core/typography.module.scss, line 21 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Oh yes, I didn't notice it was being used in a few other places. Added the RobotoMono-Regular font as a separate var.

you forgot to add it back the @include font-face("RobotoMono-Medium");

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 126 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Hmm, I'm not sure how useful that would be, since this function just applies flattenAttributes to all nodes in the tree (there is san example of what flattenAttributes does at its declaration). This function no longer structurally changes the tree so should I still include an example in the comments? It would just show that each node's attributes are flattened like in flattenAttributes comment.

At first I though it was changing the format, since it doesn't, no need to add comments :)

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.spec.tsx, line 119 at r2 (raw file):

  });
  describe("when there are nodes with multiple attributes with the same key", () => {
    it("flattens attributes for each node with", () => {

with...?

[xinhaoz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @xinhaoz)

---

pkg/ui/cluster-ui/src/core/typography.module.scss, line 21 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you forgot to add it back the @include font-face("RobotoMono-Medium");

Done.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 183 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Is it possible you can add key props to the params of the NodeAttribute? Or that's disallowed by React?

AFAIK this isn't typically done unless key will be used in the function body, so I'm going to leave it out.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 113 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Perhaps add comment here detailing what does full and partial mean here exactly? The backend API only returns a boolean on whether or not a plan is distributed or not. It's not immediately clear to me how we went from a boolean value to inferring whether a plan is fully/partially distributed ?

I took another look and it seems like the fraction value here represents the number of times the statement was executed as distributed and vectorized respectively over the statement's total execution times. "partial" here probably doesn't make much sense so I'll remove that.

I'm not sure if we want to display 'true' as long as there was at least one distributed or vectorized execution, if we only want to display these values as true when fraction = 1 and thus all execs had the respective property, or if we want to display the exact number of times it was distributed/vectorized when we don't have 0 or 1 (as in statement details).

This is what we currently do in statementDetails for distSQL and vectorized, for more context:

cockroach/pkg/ui/cluster-ui/src/statementDetails/statementDetails.tsx

Line 203 in 26398d3

function renderBools(fraction: Fraction) {

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 226 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm. this feels a bit strange to me, PlanNode and PlanNodes recursively calls each other, which means it will generates DOM that has alternating <ul> and <li> tags where each unordered list would always only contain 1 element.

<ul>
  <li>
     <ul>
       <li>
       </li>
     <ul>
  </li>
</ul>

I'm not sure if this is a common React practice in this case?

Since now that PlanNodes can only contain 1 element, why not just remove PlanNodes directly and have PlanNode render both the <ul> tag as well as <li> tag.

Yeah this does feel a bit strange... I've consolidated the two functions and wrapped children under the

instead. I think this makes a bit more sense now.

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 282 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

is this value always hard coded ?

Yesah, we don't warn for distribution and vectorized, I just wanted the attributes in this format to send to the component for rendering. I put a comment now for clarity, let me know if that works!

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.spec.tsx, line 119 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

with...?

Done.

[Azhng]

Code looks good! Maybe @maryliag can take another look at the scss stuff? I'm not too confident in my css skills.

Also left an open question for @kevin-v-ngo.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @kevin-v-ngo, and @maryliag)

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 113 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

I took another look and it seems like the fraction value here represents the number of times the statement was executed as distributed and vectorized respectively over the statement's total execution times. "partial" here probably doesn't make much sense so I'll remove that.

I'm not sure if we want to display 'true' as long as there was at least one distributed or vectorized execution, if we only want to display these values as true when fraction = 1 and thus all execs had the respective property, or if we want to display the exact number of times it was distributed/vectorized when we don't have 0 or 1 (as in statement details).

This is what we currently do in statementDetails for distSQL and vectorized, for more context:

cockroach/pkg/ui/cluster-ui/src/statementDetails/statementDetails.tsx

Line 203 in 26398d3

function renderBools(fraction: Fraction) {

I see. This seems to be an early attempt at doing some form of plan change tracking.

@kevin-v-ngo what do you think we should be doing in this case? For context, suppose we have the following scenario:

There is a stmt fingerprint S, and we have a cluster of 3 nodes (n1, n2, n3). Now suppose S is executed on n1 5 times and n26 times. Suppose, for n1, 5 / 5 executions are executed with vec: true, and for n2, the first 5/6 executions are executed with vec: true, however, due to some reason the last execution we had vec = false.

Without plan hash, the backend API will send back 2 payload:

• n1 sends back { count: 5, vec: true }
• n2 sends back { count: 6, vec: false }

So in the frontend, we originally, it seems like we would display something like plan is vec 5 of 11 times. We have few issues here:

1. The information is just wrong. Correct result should be plan is vec 10 of 11 times.
2. Should we show the plan as vec: true or vec: false? Neither of the option really reflects the real picture.
3. As far as I can tell, introducing plan hash is the only long term solution on addressing this problem. But the full fledged plan hash support won't be ready until 22.1. How can we address this in the short term?

[maryliag]

Reviewed 1 of 4 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @kevin-v-ngo, and @maryliag)

[kevin-v-ngo]

Code looks good! Maybe @maryliag can take another look at the scss stuff? I'm not too confident in my css skills.

Also left an open question for @kevin-v-ngo.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @kevin-v-ngo, and @maryliag)

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 113 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…
I see. This seems to be an early attempt at doing some form of plan change tracking.

@kevin-v-ngo what do you think we should be doing in this case? For context, suppose we have the following scenario:

There is a stmt fingerprint S, and we have a cluster of 3 nodes (n1, n2, n3). Now suppose S is executed on n1 5 times and n26 times. Suppose, for n1, 5 / 5 executions are executed with vec: true, and for n2, the first 5/6 executions are executed with vec: true, however, due to some reason the last execution we had vec = false.

Without plan hash, the backend API will send back 2 payload:

• n1 sends back { count: 5, vec: true }
• n2 sends back { count: 6, vec: false }

So in the frontend, we originally, it seems like we would display something like plan is vec 5 of 11 times. We have few issues here:

1. The information is just wrong. Correct result should be plan is vec 10 of 11 times.
2. Should we show the plan as vec: true or vec: false? Neither of the option really reflects the real picture.
3. As far as I can tell, introducing plan hash is the only long term solution on addressing this problem. But the full fledged plan hash support won't be ready until 22.1. How can we address this in the short term?

Hi @Azhng, in general, the sampled plan doesn't really reflect the full picture today anyways given that there could've been multiple different plans for that fingerprint. This should also be an edge case anyways since vectorized is on by default. All 'interesting' queries you'd run EXPLAIN for should have vectorized on unless the user explicitly sets the vectorize session variable to 'off'. For now, i'd just go with the latest sampled plan we have and use its vectorized value (most likely true).

Let's chat offline on the long term solution. We should even consider removing this from EXPLAIN.

[Azhng]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Azhng, @kevin-v-ngo, @maryliag, and @xinhaoz)

---

pkg/ui/cluster-ui/src/statementDetails/planView/planView.tsx, line 113 at r2 (raw file):

Previously, kevin-v-ngo wrote…

Hi @Azhng, in general, the sampled plan doesn't really reflect the full picture today anyways given that there could've been multiple different plans for that fingerprint. This should also be an edge case anyways since vectorized is on by default. All 'interesting' queries you'd run EXPLAIN for should have vectorized on unless the user explicitly sets the vectorize session variable to 'off'. For now, i'd just go with the latest sampled plan we have and use its vectorized value (most likely true).

Let's chat offline on the long term solution. We should even consider removing this from EXPLAIN.

👍

[xinhaoz]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)