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

partiql-visualize-dot: add #438

Merged
merged 5 commits into from
Nov 8, 2023
Merged

Conversation

rajivr
Copy link
Contributor

@rajivr rajivr commented Oct 31, 2023

Extract code from partiql-rust-cli/src/visualize

Issue #, if available:

Description of changes:

As mentioned here, I have tried to extract code from partiql-rust-cli/src/visualize, so we can easily use it in our tests and other places.

One way of using it might be as follows.

In Cargo.toml

[dev-dependencies]
# ...
partiql-visualize-dot = { path = "../partiql-visualize-dot", version = "0.5.*" }

And then within a test we can do

use partiql_visualize_dot::{PlanToDot, ToDotGraph};

#[test]
fn test {
    // create `logical` which is a value of `LogicalPlan<BindingsOp>`
    let plan_to_dot = PlanToDot::default();
    println!("{}", plan_to_dot.to_graph(&logical));
}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Extract code from `partiql-rust-cli/src/visualize`

Signed-off-by: Rajiv Ranganath <[email protected]>
@rajivr rajivr marked this pull request as ready for review October 31, 2023 05:10
@rajivr
Copy link
Contributor Author

rajivr commented Oct 31, 2023

To add further context, here is how I was able to visualize tests::custom_ion_scan_text which uses a custom catalog.

graph-ast

graph-logical

graph-physical

Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Thanks for the addition! The PartiQL Rust team is ok w/ moving the partiql-rust-cli dot visualization over. Since this visualization code was part of the experimental partiql-rust-cli, we would prefer if it was under the extension directory and also be behind a features flag.

Could you also add

  • An entry to the CHANGELOG for this addition
  • A brief README for this new crate, containing some of the verbiage from the partiql-rust-cli about this crate being experimental, subject to change, etc.

Once those are added and the other comments are addressed, we should be good to approve your PR.

Cargo.toml Outdated Show resolved Hide resolved
[dependencies]
partiql-ast = { path = "../partiql-ast", version = "0.5.*" }
partiql-logical = { path = "../partiql-logical", version = "0.5.*" }

Copy link
Member

Choose a reason for hiding this comment

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

Could you put a features section for dot visualization and call it something like visualize-dot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not entirely clear how adding visualize-dot to features would be helpful. Please allow me to explain and let me know your thoughts.

From what I can make out, the code in partiql-rust-cli/src/visualize creates an output in dot format, using dot-writer crate.

It then uses other crates to support rendering in different formats, such as png, svg, etc.,

In the current PR, I've only extracted the code to just create dot output. I am guessing when we add support for png, svg output, those can be behind feature flags in order to minimize the dependency graph?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry perhaps I should have added more detail in my initial comment. Since the dot visualization of the logical plan is still experimental / a PoC, we'd like to put the new functions and structures behind a feature similar to what was done for partiql-ast's serde feature:

With how we had defined the serde feature, the above code would only be compiled and built when the serde feature is specified (or --all-features). We would like something similar for partiql-extension-visualize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added visualize-dot feature. Please review.

partiql-visualize-dot/src/ast_to_dot.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Attention: 615 lines in your changes are missing coverage. Please review.

Comparison is base (6ef3177) 81.91% compared to head (2e527b8) 79.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #438      +/-   ##
==========================================
- Coverage   81.91%   79.00%   -2.91%     
==========================================
  Files          63       65       +2     
  Lines       16718    17333     +615     
  Branches    16718    17333     +615     
==========================================
  Hits        13694    13694              
- Misses       2460     3075     +615     
  Partials      564      564              
Files Coverage Δ
...ion/partiql-extension-visualize/src/plan_to_dot.rs 0.00% <0.00%> (ø)
...sion/partiql-extension-visualize/src/ast_to_dot.rs 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rajivr
Copy link
Contributor Author

rajivr commented Nov 6, 2023

Could you also add

* An entry to the CHANGELOG for this addition

* A brief README for this new crate, containing some of the verbiage from the partiql-rust-cli about this crate being experimental, subject to change, etc.

CHANGELOG and README has been added.

Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Thanks for applying the comments. Changes since first review look good (and thanks for merging/rebasing off of main!). I left a followup to the features comment from the prior review. Once that's resolved, think this is ready to merge.

Signed-off-by: Rajiv Ranganath <[email protected]>
Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the PR!

@alancai98
Copy link
Member

Ignoring coverage CI failure since this is experimental/PoC.

@alancai98 alancai98 merged commit febf454 into partiql:main Nov 8, 2023
9 of 10 checks passed
@rajivr rajivr deleted the partiql-visualize-dot branch November 9, 2023 03:11
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