-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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(sqllab): Adds refresh button to table metadata in SQL Lab #29974
feat(sqllab): Adds refresh button to table metadata in SQL Lab #29974
Conversation
When table metadata changes (e.g. due to queries launched in Superset or other external changes) the user has no convenient way to force a refresh of the data. The metadata is persisted even across sessions and will never update if the user stays on the current tab. This feature is a first step to make this experience better by adding a refresh button for each table that allows a manual refresh. Future improvements: - Automatically refresh when query is likely to change the metadata - Refresh metadata from time to time
4aecbb6
to
9c1b366
Compare
Rebased on |
providesTags: result => | ||
result | ||
? [ | ||
{ type: 'TableMetadatas', id: result.name }, |
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.
Having a guaranteed unique id would be better. From what I can tell we would need to return it from the API though, so I just went with the name for now.
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.
what is result.name
in this context? Is it a fully-qualified table name? Regardless I think at this time changing anything in the table selector flushes the metadata section, so even just a table name should be safe for now. If we were to support tables from multiple schemas to remain accessible in this metadata section, this could in theory create some collision.
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.
It's just the table name, no extra qualifier like schema. And I see it the same way regarding collisions, so I'm good leaving it as is it.
@@ -107,7 +108,7 @@ const TableElement = ({ table, ...props }: TableElementProps) => { | |||
const { | |||
currentData: tableMetadata, | |||
isSuccess: isMetadataSuccess, | |||
isLoading: isMetadataLoading, | |||
isFetching: isMetadataLoading, |
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.
haven't dug deep, but I'd assume switching the name here we'd also change some other reference to it somewhere else in the method?
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.
We are destructuring isFetching
(which is already part of the return of useTableMetadataQuery
) into isMetadataLoading
.
(isLoading
is only true on the first load while isFetching
is also true on subsequent fetches.)
I renamed the local variable isMetadataLoading
to isMetadataFetching
to be in sync with the naming convention.
(cherry picked from commit 9d5268a)
SUMMARY
When table metadata changes (e.g. due to queries launched in Superset or other external changes) the user has no convenient way to force a refresh of the data for their
TabState
. The metadata is persisted even across sessions and will never update if the user stays on the current tab.This feature is a first step to make this experience better by adding a refresh button for each table that allows a manual refresh.
Future improvements:
TableSchema
and instead use a shared cache with well defined expiryBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
TableSchema
was properly updated)ADDITIONAL INFORMATION