sql: add missing specs to plan diagrams #76748
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
There was a problem hiding this comment.
Although I wonder whether we should add a test that verifies somehow that whenever a new processor is introduced, its spec implements diagramCellType interface. If I'm not mistaken, at the moment (before your change) we have 29 implementations for 33 processors, so it leaves two more processors without the proper implementation which would crash the server whenever EXPLAIN (DISTSQL) is executed on a query with those two processors.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:
rharding6373
force-pushed
the
log_diagram
branch
from
6d75521
to
d1e9cd1
Compare
There was a problem hiding this comment.
That's a good point. Added the rest of the missing specs and a compliance test. PTAL.
Reviewable status:
rharding6373
changed the title
There was a problem hiding this comment.
Nice!
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:
pkg/sql/execinfrapb/flow_diagram.go, line 516 at r1 (raw file):
} // summary implements the diagramCellType interface.
nit: let's keep the comment.
rharding6373
force-pushed
the
log_diagram
branch
from
d1e9cd1
to
04f2a20
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/execinfrapb/flow_diagram.go, line 516 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: let's keep the comment.
oops, thanks for the catch!
|
bors r+ |
|
Build failed (retrying...): |
|
bors r- |
|
Canceled. |
This change allows missing specs (e.g., RestoreDataSpec and SplitAndScatterSpec) to be shown in plan diagrams. Before this change a plan involving these types would result in an error generating the diagrams. Also added a test to make sure future specs implement the `diagramCellType` interface, which is required to generate diagrams. Release note: None
rharding6373
force-pushed
the
log_diagram
branch
from
04f2a20
to
607034a
Compare
|
bors r+ |
|
Build succeeded: |
This change allows missing specs (e.g., RestoreDataSpec and
SplitAndScatterSpec) to be shown in plan diagrams. Before this change a
plan involving these types would result in an error generating the
diagrams. Also added a test to make sure future specs implement the
diagramCellTypeinterface, which is required to generate diagrams.Release note: None