Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
partiql-visualize-dot: add #438
Changes from 2 commits
257fa0b
b9a8885
ef9fd7c
8cb7965
2e527b8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 likevisualize-dot
?There was a problem hiding this comment.
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
tofeatures
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 indot
format, usingdot-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?There was a problem hiding this comment.
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
'sserde
feature:partiql-ast
feature forserde
:partiql-lang-rust/partiql-ast/Cargo.toml
Lines 31 to 36 in b9a8885
cfg
expression to conditionally include code whenserde
feature is enabled:partiql-lang-rust/partiql-ast/src/ast.rs
Line 24 in b9a8885
With how we had defined the
serde
feature, the above code would only be compiled and built when theserde
feature is specified (or--all-features
). We would like something similar forpartiql-extension-visualize
.There was a problem hiding this comment.
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.