Skip to content

fix: some bug fix related to tag and data element - #1199

Merged
czgu merged 3 commits into
pinterest:masterfrom
jczhong84:fix/tags
Mar 17, 2023
Merged

czgu merged 3 commits into
pinterest:masterfrom
jczhong84:fix/tags

Conversation

@jczhong84

Copy link
Copy Markdown
Collaborator

some bug fixes

  • can't add tags whose name contains space and underscore
  • too many api calls on the column tab in table view. Combined the tag, data element and column stats into one api call
  • unnecessary wrap of the tag name on the left sidebar mini table view. Change it to nowrap and also hide tag type in the mini view.

Also removed the expand/collapse toggle button and move the column name before the column type
image

Comment thread querybook/server/logic/data_element.py Outdated
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

don't serialize it here. Let it be serialized automatically when returning

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

api json encoder wont do automatic serialization for nested objects.

Comment thread querybook/server/logic/data_element.py
columnId,
stats,
}) => {
const { data: tableColumnStats } = useResource(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

since we don't get tags, data elements, and stats individually anymore, can you remove the api and resource?

@jczhong84 jczhong84 Mar 17, 2023 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

actually there are 3 /column/stats apis, two of them are also not used today. should we remove all of them?

@register("/column/stats/<int:column_id>/", methods=["GET"])
@register("/column/stats/<metastore_name>/", methods=["POST"])
@register("/column/stats/", methods=["POST"])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

oh yeah lets remove all of them then

name: string;
table_id: number;
type: string;
stats?: ITableColumnStats[];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

instead of stats?, can you separate the type out to be IDetailedDataColumn extends IDataColumn?

czgu
czgu previously approved these changes Mar 17, 2023
@czgu
czgu merged commit 2bb8873 into pinterest:master Mar 17, 2023
@jczhong84
jczhong84 deleted the fix/tags branch April 7, 2023 00:41
rohan-sh1 pushed a commit to CAI-TECHNOLOGIES/cai-ext-db-explorer that referenced this pull request Apr 11, 2023
* fix: some tag ui bugs

* comments

* remove column stats api
aidenprice pushed a commit to arrowtail-precision/querybook that referenced this pull request Jan 3, 2024
* fix: some tag ui bugs

* comments

* remove column stats api
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.

2 participants