From 8afac36b6b242c81f08f7566c51ed76c88425868 Mon Sep 17 00:00:00 2001 From: "J.C. Zhong" Date: Fri, 17 Mar 2023 01:12:54 +0000 Subject: [PATCH 1/3] fix: some tag ui bugs --- querybook/server/datasources/data_element.py | 33 ----- querybook/server/datasources/metastore.py | 18 ++- querybook/server/logic/data_element.py | 31 ++++- querybook/server/logic/tag.py | 2 +- .../DataTableStats/DataTableColumnStats.tsx | 16 +-- .../DataTableTags/DataTableTags.tsx | 7 +- .../DataTableTags/TableTagSelect.tsx | 2 +- .../DataTableColumnCard.scss | 28 ++--- .../DataTableColumnCard.tsx | 118 +++++++----------- .../DataTableViewMini/TablePanelView.tsx | 2 +- querybook/webapp/const/metastore.ts | 6 + querybook/webapp/resource/table.ts | 6 +- querybook/webapp/ui/Tag/HoverIconTag.tsx | 90 +++++++------ querybook/webapp/ui/Tag/Tag.scss | 1 + 14 files changed, 173 insertions(+), 187 deletions(-) diff --git a/querybook/server/datasources/data_element.py b/querybook/server/datasources/data_element.py index bc8af9580..85a1da596 100644 --- a/querybook/server/datasources/data_element.py +++ b/querybook/server/datasources/data_element.py @@ -1,39 +1,6 @@ -from app.auth.permission import verify_data_column_permission from app.datasource import register -from lib.logger import get_logger from lib.metastore import get_metastore_loader from logic import data_element as logic -from const.data_element import DataElementAssociationDict - -LOG = get_logger(__file__) - - -@register( - "/column//data_element/", - methods=["GET"], -) -def get_data_element_by_column_id(column_id: int) -> DataElementAssociationDict: - verify_data_column_permission(column_id) - associations = logic.get_data_element_associations_by_column_id(column_id=column_id) - - if not associations: - return None - - # check if there are more than 1 association type - association_types = set([r.type for r in associations]) - if len(association_types) > 1: - LOG.error( - f"Column {column_id} has more than one data element associated with it" - ) - return None - - data_element = {} - for row in associations: - data_element["type"] = row.type.value - data_element[row.property_name] = ( - row.data_element.to_dict() if row.data_element else row.primitive_type - ) - return data_element @register("/data_element//metastore_link/", methods=["GET"]) diff --git a/querybook/server/datasources/metastore.py b/querybook/server/datasources/metastore.py index 681617e1b..cd74473db 100644 --- a/querybook/server/datasources/metastore.py +++ b/querybook/server/datasources/metastore.py @@ -23,6 +23,8 @@ from lib.utils import mysql_cache from logic import metastore as logic from logic import admin as admin_logic +from logic import data_element as data_element_logic +from logic import tag as tag_logic from models.metastore import ( DataTableWarning, DataTableStatistics, @@ -374,12 +376,16 @@ def get_column_by_table(table_id, column_name, with_table=False): @register("/column//", methods=["GET"]) def get_column(column_id, with_table=False): - with DBSession() as session: - column = logic.get_column_by_id(column_id, session=session) - verify_data_table_permission(column.table_id, session=session) - column_dict = column.to_dict(with_table) - - return column_dict + column = logic.get_column_by_id(column_id) + verify_data_table_permission(column.table_id) + column_dict = column.to_dict(with_table) + + column_dict["stats"] = DataTableColumnStatistics.get_all(column_id=column_id) + column_dict["tags"] = tag_logic.get_tags_by_column_id(column_id=column_id) + column_dict[ + "data_element_association" + ] = data_element_logic.get_data_element_association_by_column_id(column_id) + return column_dict @register("/column//", methods=["PUT"]) diff --git a/querybook/server/logic/data_element.py b/querybook/server/logic/data_element.py index e5fb0cdb8..d0f0cdf9a 100644 --- a/querybook/server/logic/data_element.py +++ b/querybook/server/logic/data_element.py @@ -1,6 +1,7 @@ from typing import Union from app.db import with_session from const.data_element import ( + DataElementAssociationDict, DataElementAssociationProperty, DataElementAssociationTuple, DataElementAssociationType, @@ -24,12 +25,32 @@ def get_data_element_by_name(name: str, session=None): @with_session -def get_data_element_associations_by_column_id(column_id: int, session=None) -> dict: - return ( +def get_data_element_association_by_column_id( + column_id: int, session=None +) -> DataElementAssociationDict: + associations = ( session.query(DataElementAssociation) .filter(DataElementAssociation.column_id == column_id) .all() ) + if not associations: + return None + + # check if there are more than 1 association type + association_types = set([r.type for r in associations]) + if len(association_types) > 1: + LOG.error( + f"Column {column_id} has more than one data element associated with it" + ) + return None + + data_element = {} + for row in associations: + data_element["type"] = row.type.value + data_element[row.property_name] = ( + row.data_element.to_dict() if row.data_element else row.primitive_type + ) + return data_element @with_session @@ -75,7 +96,11 @@ def create_data_element_association( primitive_type: str = None, session=None, ): - if data_element_tuple is None: + if ( + data_element_tuple is None + or not data_element_tuple.name + or not data_element_tuple.type + ): return None if type(data_element_tuple) is str: diff --git a/querybook/server/logic/tag.py b/querybook/server/logic/tag.py index 59cf7b09a..a9d96fc7f 100644 --- a/querybook/server/logic/tag.py +++ b/querybook/server/logic/tag.py @@ -54,7 +54,7 @@ def create_or_update_tag(tag_name, meta={}, commit=True, session=None): else: tag = Tag.update( id=tag.id, - fields={"count": tag.count + 1, "meta": meta}, + fields={"count": tag.count + 1, "meta": {**tag.meta, **meta}}, skip_if_value_none=True, commit=commit, session=session, diff --git a/querybook/webapp/components/DataTableStats/DataTableColumnStats.tsx b/querybook/webapp/components/DataTableStats/DataTableColumnStats.tsx index e40849e27..63cbeab79 100644 --- a/querybook/webapp/components/DataTableStats/DataTableColumnStats.tsx +++ b/querybook/webapp/components/DataTableStats/DataTableColumnStats.tsx @@ -1,27 +1,19 @@ import * as React from 'react'; -import { useResource } from 'hooks/useResource'; +import { ITableColumnStats } from 'const/metastore'; import { isNumeric } from 'lib/utils/number'; -import { TableColumnResource } from 'resource/table'; import { KeyContentDisplay } from 'ui/KeyContentDisplay/KeyContentDisplay'; import { TableStats } from './DataTableStatsCommon'; interface IProps { - columnId: number; + stats: ITableColumnStats[]; } export const DataTableColumnStats: React.FunctionComponent = ({ - columnId, + stats, }) => { - const { data: tableColumnStats } = useResource( - React.useCallback( - () => TableColumnResource.getStats(columnId), - [columnId] - ) - ); - - const statsDOM = (tableColumnStats || []).map((tableColumnStat) => ( + const statsDOM = (stats || []).map((tableColumnStat) => ( = ({ tableId, readonly = false, mini = false, + showType = true, }) => { const isUserAdmin = useSelector( (state: IStoreState) => state.user.myUserInfo.isAdmin @@ -62,6 +64,7 @@ export const DataTableTags: React.FunctionComponent = ({ key={tag.id} isUserAdmin={isUserAdmin} mini={mini} + showType={showType} /> )); @@ -82,7 +85,8 @@ export const TableTag: React.FC<{ readonly?: boolean; deleteTag?: (tagName: string) => void; mini?: boolean; -}> = ({ tag, readonly, deleteTag, isUserAdmin, mini }) => { + showType?: boolean; +}> = ({ tag, readonly, deleteTag, isUserAdmin, mini, showType = true }) => { const tagMeta = tag.meta ?? {}; const tagRef = React.useRef(); const [showConfigModal, setShowConfigModal] = React.useState(false); @@ -151,6 +155,7 @@ export const TableTag: React.FC<{ onClick={handleTagClick} ref={tagRef} mini={mini} + showType={showType} /> ); diff --git a/querybook/webapp/components/DataTableTags/TableTagSelect.tsx b/querybook/webapp/components/DataTableTags/TableTagSelect.tsx index d4b6cc47a..d3185cafd 100644 --- a/querybook/webapp/components/DataTableTags/TableTagSelect.tsx +++ b/querybook/webapp/components/DataTableTags/TableTagSelect.tsx @@ -20,7 +20,7 @@ interface IProps { const tagReactSelectStyle = makeReactSelectStyle(true, miniReactSelectStyles); function isTagValid(val: string, existingTags: string[]) { - const regex = /^[a-z0-9]{1,255}$/i; + const regex = /^[a-z0-9_ ]{1,255}$/i; const match = val.match(regex); return Boolean(match && !existingTags.includes(val)); } diff --git a/querybook/webapp/components/DataTableViewColumn/DataTableColumnCard.scss b/querybook/webapp/components/DataTableViewColumn/DataTableColumnCard.scss index 6109876c0..84c98ae1b 100644 --- a/querybook/webapp/components/DataTableViewColumn/DataTableColumnCard.scss +++ b/querybook/webapp/components/DataTableViewColumn/DataTableColumnCard.scss @@ -4,24 +4,19 @@ .Card { margin: 16px 0px; - .DataTableColumnCard-top { - cursor: pointer; - .DataTableColumnCard-left { - display: flex; - flex-direction: row; - align-items: center; - max-width: 95%; - - .column-type { - min-width: 80px; - word-break: break-all; - } + .DataTableColumnCard-header { + display: flex; + flex-direction: row; + align-items: center; + .column-name { + width: 180px; + margin-right: 12px; } - } - .DataTableColumnCard-preview { - @include one-line-ellipsis(); - width: 95%; + .column-type { + width: 100%; + word-break: break-all; + } } .EditableTextField .editor-wrapper { @@ -31,7 +26,6 @@ } .DataTableColumnCardNestedType { - .column-type { min-width: 80px; word-break: break-all; diff --git a/querybook/webapp/components/DataTableViewColumn/DataTableColumnCard.tsx b/querybook/webapp/components/DataTableViewColumn/DataTableColumnCard.tsx index c293f603d..fad65a065 100644 --- a/querybook/webapp/components/DataTableViewColumn/DataTableColumnCard.tsx +++ b/querybook/webapp/components/DataTableViewColumn/DataTableColumnCard.tsx @@ -7,13 +7,11 @@ import { DataTableColumnStats } from 'components/DataTableStats/DataTableColumnS import { TableTag } from 'components/DataTableTags/DataTableTags'; import { IDataColumn } from 'const/metastore'; import { useResource } from 'hooks/useResource'; -import { useToggleState } from 'hooks/useToggleState'; import { Nullable } from 'lib/typescript'; import { parseType } from 'lib/utils/complex-types'; -import { DataElementResource, TableColumnResource } from 'resource/table'; +import { TableColumnResource } from 'resource/table'; import { Card } from 'ui/Card/Card'; import { EditableTextField } from 'ui/EditableTextField/EditableTextField'; -import { Icon } from 'ui/Icon/Icon'; import { KeyContentDisplay } from 'ui/KeyContentDisplay/KeyContentDisplay'; import { AccentText, StyledText } from 'ui/StyledText/StyledText'; @@ -35,31 +33,23 @@ export const DataTableColumnCard: React.FunctionComponent = ({ onEditColumnDescriptionRedirect, updateDataColumnDescription, }) => { - const { data: columnTags } = useResource( - React.useCallback( - () => TableColumnResource.getTags(column.id), - [column.id] - ) + const { data: detailedColumn } = useResource( + React.useCallback(() => TableColumnResource.get(column.id), [column.id]) ); - const { data: dataElementAssociation } = useResource( - React.useCallback( - () => DataElementResource.getDataElementByColumnId(column.id), - [column.id] - ) - ); - const [expanded, , toggleExpanded] = useToggleState(true); const parsedType = useMemo(() => parseType('', column.type), [column.type]); - const tagsDOM = (columnTags || []).map((tag) => ( + const tagsDOM = (detailedColumn?.tags || []).map((tag) => ( )); const descriptionContent = (
- {dataElementAssociation && + {detailedColumn?.data_element_association && !(column.description as ContentState).hasText() && ( )} = ({ return (
-
toggleExpanded()} - aria-label={ - expanded ? 'click to collapse' : 'click to expand' - } - data-balloon-pos="down-right" - > -
- - {column.type} - - {column.name} -
- +
+ + {column.name} + + + {column.type} +
- {expanded ? ( -
- {parsedType.children && ( - - - - )} - {tagsDOM.length > 0 && ( - -
- {tagsDOM} -
- - )} - {dataElementAssociation && ( - - - - )} - {column.comment && ( - - {column.comment} - - )} - - {descriptionContent} +
+ {parsedType.children && ( + + - -
- ) : ( -
- {(column.description as ContentState).getPlainText()} -
- )} + )} + {tagsDOM.length > 0 && ( + +
+ {tagsDOM} +
+ + )} + {detailedColumn?.data_element_association && ( + + + + )} + {column.comment && ( + + {column.comment} + + )} + + {descriptionContent} + + +
); diff --git a/querybook/webapp/components/DataTableViewMini/TablePanelView.tsx b/querybook/webapp/components/DataTableViewMini/TablePanelView.tsx index d90509fd8..f4dc5addf 100644 --- a/querybook/webapp/components/DataTableViewMini/TablePanelView.tsx +++ b/querybook/webapp/components/DataTableViewMini/TablePanelView.tsx @@ -44,7 +44,7 @@ export const TablePanelView: React.FunctionComponent = ({ ? (table.description as ContentState).getPlainText() : ''} - + ); diff --git a/querybook/webapp/const/metastore.ts b/querybook/webapp/const/metastore.ts index 2826abb11..feff7d724 100644 --- a/querybook/webapp/const/metastore.ts +++ b/querybook/webapp/const/metastore.ts @@ -1,5 +1,8 @@ import type { ContentState } from 'draft-js'; +import { IDataElementAssociation } from './dataElement'; +import { ITag } from './tag'; + // Keep it in sync with MetadataType in server/const/metastore.py export enum MetadataType { TABLE_DESCRIPTION = 'table_description', @@ -116,6 +119,9 @@ export interface IDataColumn { name: string; table_id: number; type: string; + stats?: ITableColumnStats[]; + tags?: ITag[]; + data_element_association?: IDataElementAssociation; } export interface ILineage { diff --git a/querybook/webapp/resource/table.ts b/querybook/webapp/resource/table.ts index ce8a0256e..4cd068980 100644 --- a/querybook/webapp/resource/table.ts +++ b/querybook/webapp/resource/table.ts @@ -1,7 +1,6 @@ import type { ContentState } from 'draft-js'; import JSONBig from 'json-bigint'; -import { IDataElementAssociation } from 'const/dataElement'; import type { DataTableWarningSeverity, IDataColumn, @@ -169,6 +168,8 @@ export const TableResource = { }; export const TableColumnResource = { + get: (columnId: number) => ds.fetch(`/column/${columnId}/`), + getStats: (columnId: number) => ds.fetch(`/column/stats/${columnId}/`), update: (columnId: number, description: ContentState) => { @@ -240,9 +241,6 @@ export const TableTagResource = { }; export const DataElementResource = { - getDataElementByColumnId: (columnId: number) => - ds.fetch(`/column/${columnId}/data_element/`), - getMetastoreLink: (dataElementId: number) => ds.fetch(`/data_element/${dataElementId}/metastore_link/`), }; diff --git a/querybook/webapp/ui/Tag/HoverIconTag.tsx b/querybook/webapp/ui/Tag/HoverIconTag.tsx index 75788664f..7b264b8ee 100644 --- a/querybook/webapp/ui/Tag/HoverIconTag.tsx +++ b/querybook/webapp/ui/Tag/HoverIconTag.tsx @@ -11,52 +11,66 @@ export interface IHoverIconTagProps extends Omit { type?: string; icon?: string; iconOnHover?: AllLucideIconNames; + showType?: boolean; onIconHoverClick?: (e?: React.MouseEvent) => any; } export const HoverIconTag = React.forwardRef< HTMLSpanElement, IHoverIconTagProps ->(({ name, type, icon, iconOnHover, onIconHoverClick, ...tagProps }, ref) => { - const hoverDOM = iconOnHover ? ( -
- -
- ) : null; +>( + ( + { + name, + type, + icon, + iconOnHover, + showType = true, + onIconHoverClick, + ...tagProps + }, + ref + ) => { + const hoverDOM = iconOnHover ? ( +
+ +
+ ) : null; - const className = clsx(tagProps['className'], 'HoverIconTag'); + const className = clsx(tagProps['className'], 'HoverIconTag'); - const iconDOM = icon && ( - - ); + const iconDOM = icon && ( + + ); + + if (type && showType) { + const { tooltip, tooltipPos, color, mini, onClick, ...extraProps } = + tagProps; + return ( + + + {iconDOM} + {type} + + + {name} + {hoverDOM} + + + ); + } - if (type) { - const { tooltip, tooltipPos, color, mini, onClick, ...extraProps } = - tagProps; return ( - - - {iconDOM} - {type} - - - {name} - {hoverDOM} - - + + {iconDOM} + {name} + {hoverDOM} + ); } - - return ( - - {iconDOM} - {name} - {hoverDOM} - - ); -}); +); diff --git a/querybook/webapp/ui/Tag/Tag.scss b/querybook/webapp/ui/Tag/Tag.scss index 68def6334..d1485bea5 100644 --- a/querybook/webapp/ui/Tag/Tag.scss +++ b/querybook/webapp/ui/Tag/Tag.scss @@ -26,6 +26,7 @@ font-size: var(--small-text-size); font-family: var(--font-accent); max-width: 100%; + white-space: nowrap; span { @include one-line-ellipsis(); From b15be801eac7f734a63df92668540abd8a56d236 Mon Sep 17 00:00:00 2001 From: "J.C. Zhong" Date: Fri, 17 Mar 2023 02:51:18 +0000 Subject: [PATCH 2/3] comments --- querybook/server/logic/data_element.py | 2 +- querybook/webapp/const/metastore.ts | 2 ++ querybook/webapp/resource/table.ts | 6 +++--- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/querybook/server/logic/data_element.py b/querybook/server/logic/data_element.py index d0f0cdf9a..da91bce27 100644 --- a/querybook/server/logic/data_element.py +++ b/querybook/server/logic/data_element.py @@ -97,7 +97,7 @@ def create_data_element_association( session=None, ): if ( - data_element_tuple is None + not data_element_tuple or not data_element_tuple.name or not data_element_tuple.type ): diff --git a/querybook/webapp/const/metastore.ts b/querybook/webapp/const/metastore.ts index feff7d724..abe4f2135 100644 --- a/querybook/webapp/const/metastore.ts +++ b/querybook/webapp/const/metastore.ts @@ -119,6 +119,8 @@ export interface IDataColumn { name: string; table_id: number; type: string; +} +export interface IDetailedDataColumn extends IDataColumn { stats?: ITableColumnStats[]; tags?: ITag[]; data_element_association?: IDataElementAssociation; diff --git a/querybook/webapp/resource/table.ts b/querybook/webapp/resource/table.ts index 4cd068980..ccb8b79f1 100644 --- a/querybook/webapp/resource/table.ts +++ b/querybook/webapp/resource/table.ts @@ -10,6 +10,7 @@ import type { IDataTableSamples, IDataTableWarning, IDataTableWarningUpdateFields, + IDetailedDataColumn, ILineage, IPaginatedQuerySampleFilters, IQueryMetastore, @@ -168,10 +169,9 @@ export const TableResource = { }; export const TableColumnResource = { - get: (columnId: number) => ds.fetch(`/column/${columnId}/`), + get: (columnId: number) => + ds.fetch(`/column/${columnId}/`), - getStats: (columnId: number) => - ds.fetch(`/column/stats/${columnId}/`), update: (columnId: number, description: ContentState) => { const params = { description: convertContentStateToHTML(description), From 2e3201fbb6ab2e52df0a30c662553cf00b48937d Mon Sep 17 00:00:00 2001 From: "J.C. Zhong" Date: Fri, 17 Mar 2023 18:07:59 +0000 Subject: [PATCH 3/3] remove column stats api --- querybook/server/datasources/metastore.py | 62 ----------------------- querybook/server/logic/data_element.py | 2 +- 2 files changed, 1 insertion(+), 63 deletions(-) diff --git a/querybook/server/datasources/metastore.py b/querybook/server/datasources/metastore.py index cd74473db..5ee384b52 100644 --- a/querybook/server/datasources/metastore.py +++ b/querybook/server/datasources/metastore.py @@ -562,68 +562,6 @@ def create_table_stats(data): return -@register("/column/stats//", methods=["GET"]) -def get_table_column_stats(column_id): - """Get all table stats column by id""" - with DBSession() as session: - column = logic.get_column_by_id(column_id, session=session) - verify_data_table_permission(column.table_id, session=session) - return DataTableColumnStatistics.get_all(column_id=column_id, session=session) - - -@register("/column/stats//", methods=["POST"]) -def create_table_column_stats_by_name(metastore_name, data): - """Batch add/update table column stats""" - # TODO: verify user is a service account - with DBSession() as session: - metastore = admin_logic.get_query_metastore_by_name( - metastore_name, session=session - ) - api_assert(metastore, "Invalid metastore") - verify_metastore_permission(metastore.id, session=session) - - with DataTableFinder(metastore.id) as t_finder: - for d in data: - column = t_finder.get_table_column_by_name( - schema_name=d["schema_name"], - table_name=d["table_name"], - column_name=d["column_name"], - session=session, - ) - - if column is not None: - for s in d["stats"]: - logic.upsert_table_column_stat( - column_id=column.id, - key=s["key"], - value=s["value"], - uid=current_user.id, - session=session, - ) - return - - -@register("/column/stats/", methods=["POST"]) -def create_table_column_stats(data): - """Batch add/update table column stats""" - # TODO: verify user is a service account - with DBSession() as session: - - for d in data: - column = logic.get_column_by_id(d["column_id"], session=session) - if column: - verify_data_table_permission(column.table_id, session=session) - for s in d["stats"]: - logic.upsert_table_column_stat( - column_id=d["column_id"], - key=s["key"], - value=s["value"], - uid=current_user.id, - session=session, - ) - return - - @register("/lineage/", methods=["POST"]) @admin_only def add_lineage(table_id, parent_table_id, job_metadata_id): diff --git a/querybook/server/logic/data_element.py b/querybook/server/logic/data_element.py index da91bce27..5a1b23726 100644 --- a/querybook/server/logic/data_element.py +++ b/querybook/server/logic/data_element.py @@ -48,7 +48,7 @@ def get_data_element_association_by_column_id( for row in associations: data_element["type"] = row.type.value data_element[row.property_name] = ( - row.data_element.to_dict() if row.data_element else row.primitive_type + row.data_element if row.data_element else row.primitive_type ) return data_element