-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: add certification info to table selector #11785
feat: add certification info to table selector #11785
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11785 +/- ##
==========================================
- Coverage 67.20% 63.35% -3.85%
==========================================
Files 903 905 +2
Lines 43719 43867 +148
Branches 4199 4205 +6
==========================================
- Hits 29380 27791 -1589
- Misses 14235 15896 +1661
- Partials 104 180 +76
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
a533ead
to
233f226
Compare
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.
Lgtm with one small comment.
superset/connectors/sqla/models.py
Outdated
return json.loads(self.extra) | ||
except (TypeError, json.JSONDecodeError): | ||
return {} | ||
|
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.
Wdyt of making this be a @property
?
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.
seems reasonable
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.
done
233f226
to
a9891d0
Compare
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.
LGTM
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.
post-hoc LGTM
SUMMARY
The current table selector only gets table names from the database engine itself, and leaves out any additional metadata a user may have provided to a physical dataset. This merges the tables list returned from the database with any extra metadata from the datasets so we can display certification status in table selectors throughout the product
AFTER SCREENSHOTS
TEST PLAN
ADDITIONAL INFORMATION
to: @ktmud @graceguo-supercat @bkyryliuk @john-bodley