-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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 extra column to tables and sql_metrics #10592
feat: add extra column to tables and sql_metrics #10592
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.
@etr2460 do you think it would be advisable to add a note to UPDATING.md
given this PR includes a migration?
3c6aa77
to
9d17782
Compare
Do you think it matters? It should be completely push safe. Unless you think there'll be performance issues |
@etr2460 reading through UPDATING.md I guess we only called out PRs that added columns which could be problematic like #8867. I guess the question is whether locking these tables will be problematic under production load. Note for MySQL adding a new column actually recreates the table and copies the data and the |
9d17782
to
c8925e5
Compare
Codecov Report
@@ Coverage Diff @@
## master #10592 +/- ##
==========================================
- Coverage 63.98% 59.26% -4.72%
==========================================
Files 768 356 -412
Lines 36359 22908 -13451
Branches 3431 0 -3431
==========================================
- Hits 23264 13577 -9687
+ Misses 12982 9331 -3651
+ Partials 113 0 -113
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -26,7 +26,7 @@ | |||
from superset import app | |||
|
|||
# Globals | |||
config = app.config # type: ignore | |||
config = app.config |
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.
needed to pass CI
SUMMARY
Adds the columns mentioned in #10591 for extra metadata around tables and metrics (with certification being the first use case)
I'm PRing out only the migration first to ensure push safety with future code that references these columns.
TEST PLAN
CI, run the migration and see it succeed
ADDITIONAL INFORMATION
to: @john-bodley @graceguo-supercat @ktmud