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

feat: Table and Column Lineage Lists #969

Merged
merged 17 commits into from
Apr 9, 2021
Merged

feat: Table and Column Lineage Lists #969

merged 17 commits into from
Apr 9, 2021

Conversation

danwom
Copy link

@danwom danwom commented Apr 7, 2021

Summary of Changes

Added support for configurable table and column level lineage. Table lineage appears as upstream and downstream tabs in the table details page. Column lineage appears as a small list when columns details are expanded.

Tests

Tests are WIP. Will be added to this PR soon.

Documentation

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes, including screenshots of any UI changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public python functions and the classes in the PR contain docstrings that explain what it does
  • PR passes all tests documented in the developer guide

Comment on lines 441 to 442
const columnLineage = (<GetColumnLineageResponse>action).payload.lineage;
const { columnName } = (<GetColumnLineageResponse>action).payload;
Copy link
Member

Choose a reason for hiding this comment

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

You might combine these two in one?

Suggested change
const columnLineage = (<GetColumnLineageResponse>action).payload.lineage;
const { columnName } = (<GetColumnLineageResponse>action).payload;
const { columnName, columnLineage: lineage } = (<GetColumnLineageResponse>action).payload;

Copy link
Author

Choose a reason for hiding this comment

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

Haven't used this before. Thanks

Comment on lines 4 to 12
import { connect } from 'react-redux';

import { GlobalState } from 'ducks/rootReducer';
import * as React from 'react';
import { emptyLineage } from 'ducks/tableMetadata/reducer';

import { getColumnLineageLink } from 'config/config-utils';
import './styles.scss';
import { Lineage, TableMetadata } from 'interfaces/TableMetadata';
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would order it differntly:

Suggested change
import { connect } from 'react-redux';
import { GlobalState } from 'ducks/rootReducer';
import * as React from 'react';
import { emptyLineage } from 'ducks/tableMetadata/reducer';
import { getColumnLineageLink } from 'config/config-utils';
import './styles.scss';
import { Lineage, TableMetadata } from 'interfaces/TableMetadata';
import * as React from 'react';
import { connect } from 'react-redux';
import { GlobalState } from 'ducks/rootReducer';
import { emptyLineage } from 'ducks/tableMetadata/reducer';
import { getColumnLineageLink } from 'config/config-utils';
import { Lineage, TableMetadata } from 'interfaces/TableMetadata';
import './styles.scss';

Copy link
Author

Choose a reason for hiding this comment

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

One of the artifacts of using "alt+enter" to import a dep is that it seems to place it somewhat randomly. Will clean up.


export class ColumnLineageList extends React.Component<Props> {
renderLineageLinks(entity, index) {
if (index >= 5) {
Copy link
Member

Choose a reason for hiding this comment

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

Why 5? Could we make this number a constant with a name that explains itself?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I forgot to put this into a constant.

Comment on lines 64 to 67
<span className="title-3">Top 5 Downstream Columns&nbsp;</span>
<a href={externalLink} rel="noreferrer" target="_blank">
See More
</a>
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as above.
This might signal a common component? Something like

>
<div className="resource-info">
<div className="resource-info-text my-auto">
<div className="resource-name title-2">
Copy link
Member

Choose a reason for hiding this comment

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

We would use @extend %text-title-w2; in the styles file instead

{!!table.badges && table.badges.length > 0 && (
<div>
<div className="body-secondary-3">
<BadgeList badges={badges} />
Copy link
Member

Choose a reason for hiding this comment

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

Maybe @extend %text-body-w3;

</div>
</div>
)}
<img className="icon icon-right" alt="" />
Copy link
Member

Choose a reason for hiding this comment

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

We are moving out of these icons too, I guess RightIcon is the way to go now

Copy link
Author

Choose a reason for hiding this comment

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

Didn't notice those before. Btw how do we style a ? Here we only want to show this arrow when hovered, but the component doesn't have a className on the root element like className="right-icon"

Copy link
Member

@Golodhros Golodhros left a comment

Choose a reason for hiding this comment

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

Looking good overall!

I usually prefer to see the tests at the same time, but this will do!

@danwom
Copy link
Author

danwom commented Apr 9, 2021

@Golodhros @verdan Feel free to take another look when you have time.

@danwom danwom changed the title Feat - Table and Column Lineage Lists feat - Table and Column Lineage Lists Apr 9, 2021
@@ -349,7 +349,14 @@ body {

.body-link {
color: $brand-color-4;
font-size: 14px;
font-size: 16px;
Copy link
Member

Choose a reason for hiding this comment

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

Let's use $font-size-large instead

Copy link
Member

@Golodhros Golodhros 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!!

Daniel Won added 14 commits April 9, 2021 14:40
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
- refactored links to use 'body-link' class

Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
@danwom danwom changed the title feat - Table and Column Lineage Lists feat: Table and Column Lineage Lists Apr 9, 2021
Daniel Won added 3 commits April 9, 2021 15:00
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
@danwom danwom dismissed verdan’s stale review April 9, 2021 22:52

Requested changes have been addressed

@allisonsuarez
Copy link
Contributor

👍

@danwom danwom merged commit df9532a into master Apr 9, 2021
@feng-tao
Copy link
Member

feng-tao commented Apr 9, 2021

@danwom cool cool cool!

@danwom
Copy link
Author

danwom commented Apr 9, 2021

cool cool cool

🤣

@dorianj
Copy link
Contributor

dorianj commented Apr 10, 2021

Nice!

@dorianj dorianj deleted the lineage-config branch April 10, 2021 17:49
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.

6 participants