feat: CLI tool for visualizing plans #8
Conversation
package-lock.json
Outdated
| "version": "2.12.2", | ||
| "resolved": "https://registry.npmjs.org/@types/commander/-/commander-2.12.2.tgz", | ||
| "integrity": "sha512-0QEFiR8ljcHp9bAbWxecjVRuAMr16ivPiGOw6KFQBVrVd0RQIcM3xKdRisH2EDWgVWujiYtHwhSkSUoAAGzH7Q==", | ||
| "deprecated": "This is a stub types definition for commander (https://github.com/tj/commander.js). commander provides its own type definitions, so you don't need @types/commander installed!", |
There was a problem hiding this comment.
Since types/commander is listed above is it needed here?
There was a problem hiding this comment.
this is a generated file when we run npm install for installing the npm packages, but AFAIK this is required in the project to correctly have the dependencies.
sanjibansg
force-pushed
the
cli
branch
2 times, most recently
from
29b319e
to
cccb165
Compare
|
|
||
| const outputSvgContent = fs.readFileSync('plan.svg', 'utf-8'); | ||
| const expectedSvgContent = fs.readFileSync('test/expected.svg', 'utf-8'); | ||
| expect(outputSvgContent).toEqual(expectedSvgContent); |
There was a problem hiding this comment.
I am not sure if this should be the way to test whether the tool generates the correct SVG file. Is there any better way for this?
There was a problem hiding this comment.
An alternative would be to check that the file is there and then check its contents (which you could do with a string in a test file somewhere). But the input is coming from a separate test file. It seems appropriate to have a separate golden file to match. This could be changed to look for test files and compare them to golden ones (allowing it to check for multiple correct files) instead of being limited to a single file.
There was a problem hiding this comment.
okay, so what I understand is you suggest having multiple tests verifying the correctness of the tool instead of just limiting it to the one plan and its expected SVG. Is that correct?
There was a problem hiding this comment.
Long term that's the way to go. But for now this is a step in that direction.
| function renderGraph(edges: Link[]): Promise<string> { | ||
| const dom = new JSDOM(); | ||
| global.document = dom.window.document; | ||
| global.DOMParser = new JSDOM().window.DOMParser; | ||
|
|
||
| return instance().then((viz) => { | ||
| const dotString = `digraph { ${edges | ||
| .map((edge) => `${edge.source} -> ${edge.target}`) | ||
| .join("; ")} }`; | ||
|
|
||
| const svg = viz.renderSVGElement(dotString); | ||
| return svg.outerHTML; | ||
| }) as Promise<string>; | ||
| } |
There was a problem hiding this comment.
+1, This is definitely much simpler than the D3 approach.
There was a problem hiding this comment.
After my research, I had to remove d3 from the substrait-JS library, as d3 plots need a browser environment to render plots correctly and cannot do so on the server side with JSDOM, thus I had to move to graphviz. The fiddle can use the pre-processing functions defined here to render the d3 plot.
This PR introduces the CLI tool for visualizing substrait plans in substraitJS.