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] reduce table metadata fetch for latest_partition check #9685
[fix] reduce table metadata fetch for latest_partition check #9685
Conversation
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.
one change requested, otherwise lgtm
); | ||
}) | ||
.catch(error => { | ||
console.log('fetch extra_table_metadata:', error.statusText); |
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.
can you add an eslint disable comment here to prevent the warning?
and maybe this should be a console.error
?
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.
fixed. console.error is better.
@@ -581,6 +581,7 @@ def data(self) -> Dict: | |||
d["main_dttm_col"] = self.main_dttm_col | |||
d["fetch_values_predicate"] = self.fetch_values_predicate | |||
d["template_params"] = self.template_params | |||
d["is_sqllab_view"] = self.is_sqllab_view |
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.
@graceguo-supercat does this impact the cache key in anyway? If so we should probably add a note in UPDATING.md
to inform others.
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.
@john-bodley It adds extra attribute in the datasource object. I am not sure how cache key use datasource. Is this a relevant cache key code?
https://github.com/apache/incubator-superset/blob/a6cedaaa879348aca49a520793bb20e63d152a1f/superset/common/query_object.py#L199
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.
@graceguo-supercat I'm not exactly sure where this is used but I think it's unrelated to the cache. Sorry for the false alarm.
CATEGORY
Choose one
SUMMARY
In #9637 I added an API call to
/superset/extra_table_metadata
to check datasource table's metadata, if it has partition then show extra adhoc filter operator. But virtual table (with is_sqllab_view flag is True) doesn't have real table exists, so above metadata check will return server side error.This PR is to add is_sqllab_view check before fetching table metadata.
TEST PLAN
Manual test
ADDITIONAL INFORMATION
REVIEWERS
@etr2460