Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#626 The diagram view includes 'Parallel' for parallel-aware nodes #638

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

dkarneman
Copy link
Contributor

Per #626, the Diagram doesn't include Parallel for parallel-aware nodes (Seq Scan, Append, Hash, etc). The 'Parallel' modifier is stripped during parsing and then added back in the graph view.

This PR re-uses the nodeName function to make the two match.

Render of src/services/__tests__/12-line-wrapped-plans/05-plan
Screenshot_2024-03-21_at_11_31_22

I considered removing the line that strips out 'Parallel'. But that would alter the NODE_TYPE and it's not clear to me whether nodes of the same type are handled separately (i.e. do all 'Seq Scans' need to be treated alike, regardless of whether they're parallel?)

Test Plan

Existing tests pass, and lints cleanly (except for one lint that is not modified by this PR.)

I'd love to write a test for this change, but I don't see existing examples that test the view, just the parser. Please point me in the right direction if there's a good way to test the view.

@pgiraud
Copy link
Member

pgiraud commented Mar 26, 2024

I considered removing the line that strips out 'Parallel'. But that would alter the NODE_TYPE and it's not clear to me whether nodes of the same type are handled separately (i.e. do all 'Seq Scans' need to be treated alike, regardless of whether they're parallel?)

The reason why the Parallel is removed from NODE TYPE is to match the JSON format. When parsing a plan in the text format, we try to convert it to a JS object that ressemble the explain JSON format as much as possible.
See https://github.com/dalibo/pev2/blob/master/example/src/samples.ts#L5790-L5792 for an example of a plan in JSON format with parallel nodes.

Existing tests pass, and lints cleanly (except for one lint that is not modified by this PR.)

Thanks for the report. I'll fix that.

I'd love to write a test for this change, but I don't see existing examples that test the view, just the parser. Please point me in the right direction if there's a good way to test the view.

There's unfortunately no tests for the rendering. I really need to add some.

@pgiraud pgiraud merged commit bcd9996 into dalibo:master Mar 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants