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

Add DOT encoding support for internal/graph #70

Merged

Conversation

kevinmingtarja
Copy link
Contributor

Description

This PR adds support for encoding the dependency graph to DOT format, which is used by Graphviz, a popular graph visualization software. This will allow us to visualize our dependency graphs for debugging purposes.

Motivation

Closes #38

Testing

  • Added a unit test for the function
  • Locally, I tested it against some schemas from the test suite and temporarily outputting the encoded graph in resolveToSQLGraph(), then manually visualized it using http://magjac.com/graphviz-visual-editor/ to verify the format's correctness

Example output:

I have a fresh new postgres instance and my target schema is this:

CREATE TABLE foobar(
    id INT,
    foo VARCHAR(255),
    bar TEXT,
    fizz INT,
    PRIMARY KEY (foo, id)
) PARTITION BY LIST (foo);
CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN ('foo_1');
CREATE TABLE foobar_2 PARTITION OF foobar FOR VALUES IN ('foo_2');
CREATE TABLE foobar_3 PARTITION OF foobar FOR VALUES IN ('foo_3');

Based on the diffs and dependency graph, The DOT encoding will output:

digraph G {
fontname="Helvetica,Arial,sans-serif"
node [fontname="Helvetica,Arial,sans-serif"]
edge [fontname="Helvetica,Arial,sans-serif"]
n0 [label="ADDALTER_table_foobar"]
n1 [label="ADDALTER_table_foobar_2"]
n2 [label="DELETE_table_foobar_2"]
n3 [label="ADDALTER_table_foobar_3"]
n4 [label="DELETE_table_foobar_3"]
n5 [label="DELETE_table_foobar"]
n6 [label="ADDALTER_table_foobar_1"]
n7 [label="DELETE_table_foobar_1"]
n0 -> n1
n0 -> n3
n0 -> n6
n2 -> n1
n4 -> n3
n5 -> n0
n7 -> n6
}
Screen Shot 2023-09-06 at 17 54 51

@CLAassistant
Copy link

CLAassistant commented Sep 6, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@bplunkett-stripe bplunkett-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation here is good! I just have some small nits/tweaks.

Really cool set of changes!

internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/dotbuilder.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph_test.go Outdated Show resolved Hide resolved
@kevinmingtarja
Copy link
Contributor Author

Thanks for the feedback @bplunkett-stripe ! I just pushed my changes based on the review.

Copy link
Collaborator

@bplunkett-stripe bplunkett-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for making the changes! Only small change left is getting linting to pass.

Looks like linting isn't passing -- maybe I should allow anyone who posts a PR to run the workflows? In the interim, you can use make lint_fix (you will need to install few things) or you could use this docker container to identify linting errors

internal/graph/encode_dot_test.go Show resolved Hide resolved
@kevinmingtarja
Copy link
Contributor Author

I just did make lint_fix. It should be good to go now!

@bplunkett-stripe
Copy link
Collaborator

Nice! Let's hop back onto the ticket to figure out the structure of the CLI for this

@bplunkett-stripe bplunkett-stripe merged commit f26b44b into stripe:main Sep 7, 2023
5 checks passed
@kevinmingtarja kevinmingtarja deleted the kevinmingtarja/graph-dot-encoding branch September 8, 2023 03:55
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.

Create way to visualize dependency graph
3 participants