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

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

Merged
merged 3 commits into from
Mar 17, 2023

Conversation

jczhong84
Copy link
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

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
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
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.

querybook/server/logic/data_element.py Show resolved Hide resolved
}) => {
const { data: tableColumnStats } = useResource(
Copy link
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?

Copy link
Collaborator Author

@jczhong84 jczhong84 Mar 17, 2023

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
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

@@ -116,6 +119,9 @@ export interface IDataColumn {
name: string;
table_id: number;
type: string;
stats?: ITableColumnStats[];
Copy link
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