-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(lineage) Add column-level impact analysis feature #6272
feat(lineage) Add column-level impact analysis feature #6272
Conversation
Urn resourceUrn = Urn.createFromString(schemaFieldUrn.getEntityKey().get(0)); | ||
result.setParent(UrnToEntityMapper.map(resourceUrn)); | ||
} catch (Exception e) { | ||
log.error("Error converting schemaField parent urn string to Urn", e); |
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.
There's no issue if we can't convert right? It's still safe to return the mapped object without this field?
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.
ah that's a good catch - SchemaFieldEntity
is expecting parent
to not be null here. so I think our options are to either make parent
nullable or raise an error if this situation occurs (which it shouldn't). I don't think it really makes sense to have parent
be null since the schemaField should always have a valid parent reference urn in its urn.. so i'm leaning towards raising an error. what do you think?
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.
talked IRL and agreed on throwing an exception here
@@ -2343,21 +2343,31 @@ type KeyValueSchema { | |||
Standalone schema field entity. Differs from the SchemaField struct because it is not directly nested inside a | |||
schema field | |||
""" | |||
type SchemaFieldEntity { | |||
type SchemaFieldEntity implements Entity { |
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.
Nice
@@ -55,4 +55,29 @@ describe("impact analysis", () => { | |||
cy.contains("User Creations").should("not.exist"); | |||
cy.contains("User Deletions"); | |||
}); | |||
|
|||
it("can view column level impact analysis and turn it off", () => { |
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.
Amazing, thank you!
Adds the ability to view impact analysis specifically at the column level. You can now select a column to view impact analysis on from the schema tab with a new menu or select from a dropdown on the lineage tab. You'll see the impacted columns and you can even click on the column name in order to see the whole path for how you get between the two columns.
This involved some changes to the how we index upstream lineage. We now check if the upstream lineage aspect that we're indexing has fineGrainedLineage, and if so, we draw edges manually between the two
schemaField
s. That way we can query to get edges based on aschemaField
urn.Unforunately this means that for those users that already have fineGrainedLineage ingested, they will need to restore their indices in order to get old fineGrainedLineage data to work with impact analysis
Here's some screenshots for how this looks in action:
The new column menu
Viewing impact analysis for the
email
columnThe paths modal to see the full path between entities and their columns (all of these happen to be named
email
)Checklist