-
Notifications
You must be signed in to change notification settings - Fork 961
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: Show Nested Table Columns #1489
Conversation
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
Signed-off-by: Daniel Won <dwon@lyft.com>
@@ -236,7 +236,7 @@ const Table: React.FC<TableProps> = ({ | |||
ref={index === preExpandRow ? expandRowRef : null} | |||
> | |||
<> | |||
{expandRow ? ( | |||
{expandRow && item.isExpandable ? ( |
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.
Probably need to reverse this field or something.
frontend/amundsen_application/static/js/ducks/tableMetadata/api/helpers.ts
Outdated
Show resolved
Hide resolved
frontend/amundsen_application/static/js/ducks/tableMetadata/api/helpers.ts
Outdated
Show resolved
Hide resolved
frontend/amundsen_application/static/js/features/ColumnList/ColumnType/parser.ts
Show resolved
Hide resolved
frontend/amundsen_application/static/js/features/ColumnList/index.tsx
Outdated
Show resolved
Hide resolved
frontend/amundsen_application/static/js/features/ColumnList/styles.scss
Outdated
Show resolved
Hide resolved
* | ||
* @param columns | ||
*/ | ||
export function parseNestedColumns(columns: TableColumn[]) { |
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.
Let's test all these helper functions thoroughly.
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>
@@ -280,7 +283,7 @@ const Table: React.FC<TableProps> = ({ | |||
})} | |||
</> | |||
</tr> | |||
{expandRow ? ( | |||
{expandRow? ( |
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.
{expandRow? ( | |
{expandRow ? ( |
const nestedType = parseNestedType(column.col_type); | ||
return { |
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.
Nit:
const nestedType = parseNestedType(column.col_type); | |
return { | |
const nestedType = parseNestedType(column.col_type); | |
return { |
stats: [], | ||
}, | ||
]; | ||
const result = convertNestedTypeToColumns(nestedType); |
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.
const result = convertNestedTypeToColumns(nestedType); | |
const result = Parser.convertNestedTypeToColumns(nestedType); |
let arrow; | ||
if (nestedLevel > 0) { | ||
arrow = ( | ||
<> | ||
<div className={`nesting-arrow-spacer spacer-${nestedLevel}`} /> | ||
<NestingArrow /> | ||
</> | ||
); | ||
} | ||
return ( | ||
<> | ||
{arrow} | ||
<div className="column-name-container"> | ||
<h3 className="column-name">{title}</h3> | ||
<p className="column-desc truncated">{description}</p> | ||
</div> | ||
</> | ||
); |
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 would just do:
let arrow; | |
if (nestedLevel > 0) { | |
arrow = ( | |
<> | |
<div className={`nesting-arrow-spacer spacer-${nestedLevel}`} /> | |
<NestingArrow /> | |
</> | |
); | |
} | |
return ( | |
<> | |
{arrow} | |
<div className="column-name-container"> | |
<h3 className="column-name">{title}</h3> | |
<p className="column-desc truncated">{description}</p> | |
</div> | |
</> | |
); | |
return ( | |
<> | |
{nestedLevel > 0 && (<> | |
<div className={`nesting-arrow-spacer spacer-${nestedLevel}`} /> | |
<NestingArrow /> | |
</>)} | |
<div className="column-name-container"> | |
<h3 className="column-name">{title}</h3> | |
<p className="column-desc truncated">{description}</p> | |
</div> | |
</> | |
); |
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.
Looking good!
There seems to be an issue with your prettier or your pre-commit hooks, as there are some formatting issues.
Signed-off-by: Daniel Won <dwon@lyft.com>
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.
LGTM
This is awesome, do you have a screenshot here that you could share? |
I have one request, Can we have sample data as well ? because we are getting such a good features but can't get visual or understanding better without having sample data. |
Hi @danwom. This change significantly derailed the frontend of my organizations Amundsen deployment. Some of the databuilder extractors (namely Delta Lake & BigQuery) scrape for nested columns during the extraction process. They do this because previously there was no nice way to render nested columns on the UI so they just got created as columns. With this UI change these columns are now rendered twice. Same columns on same Table Detail Page: My ask here is can we roll these types of change out behind a configuration value? This way folks have to deliberately turhn it on. Or if people prefer this way then it can be turned on by default but at least there is a way for me to turn it off. I would still like to have my extractor extract the nested the columns because it is easier to read with the full nested path (especially where there are many levels of nesting) and I can add descriptions to nested columns that way. cc @markgrover @feng-tao if you have any input. |
- Parses complex column types and treats the nested columns as children - Fixes bugs with sorting columns (description, type not updating) Signed-off-by: Amom Mendes <amommendes@hotmail.com>
- Parses complex column types and treats the nested columns as children - Fixes bugs with sorting columns (description, type not updating) Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
- Parses complex column types and treats the nested columns as children - Fixes bugs with sorting columns (description, type not updating)
- Parses complex column types and treats the nested columns as children - Fixes bugs with sorting columns (description, type not updating)
Summary of Changes
In addition to showing nested columns as strings in a modal, we now support showing nested columns as individual rows in the list of columns. Also fixes some bugs where sorting columns would mess up descriptions and nested types.
Tests
Documentation
CheckList
Make sure you have checked all steps below to ensure a timely review.