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.

Sign up for GitHub

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

Jump to bottom

Fix mermaid diagram subgraph generation #6

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Fix mermaid diagram subgraph generation #6

merged 1 commit into from
Nov 9, 2023

Conversation

LeonardoNatale
Copy link
Contributor

Issue

  • Current mermaid diagram are not correctly generated, resulting in empty subgraph.
  • See the jaffle_shop example ⬇️
    image

Solution

  • Remove extra sort on the schema_nodes -> it looks like its exhausting the iterator (not 100% sure)
  • handle case where node length > 2
    • the inner for loop will fail for a node represented by the tuple (analytics, finance, kpis)
  • see diagram generated following the changes ⬇️
    image

MaxHalford
MaxHalford approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@MaxHalford MaxHalford left a comment

Choose a reason for hiding this comment

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

Nice catch @LeonardoNatale! And congrats on your first PR, much appreciated :)

lea/views/dag.py Show resolved Hide resolved
@MaxHalford MaxHalford merged commit 717c88a into carbonfact:main Nov 9, 2023
2 of 3 checks passed
@MaxHalford
Copy link
Member

I just added some doctests for good measure :)

@LeonardoNatale LeonardoNatale deleted the fix/mermaid-diagram-generation branch November 13, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxHalford MaxHalford MaxHalford approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LeonardoNatale @MaxHalford