You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/07/20 23:57:38 UTC
[GitHub] [superset] betodealmeida opened a new pull request #15807: fix: migration script can't drop constraint
betodealmeida opened a new pull request #15807:
URL: https://github.com/apache/superset/pull/15807
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
We're trying to run the `downgrade` from `49b5a32daba5_add_report_schedules.py`, but the migration errors because the unique constraint `uq_report_schedule_name` is not found. The `upgrade` function in the migration has the creation of the constraint inside a permissive `try/except` block, so I'm assuming the upgrade partially failed for some reason and now we can't downgrade.
I modified the migration to check if the constraint exists before deleting it. The change should be harmless, because in theory the constraint should always exist. But maybe we should adopt this as a more conservative approach for `downgrade`s in migrations.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
I was able to benchmark the migration:
```
Identifying models used in the migration:
- dbs (2 rows in table dbs)
- slices (114 rows in table slices)
- tables (27 rows in table tables)
Benchmarking migration
INFO [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO [alembic.runtime.migration] Will assume transactional DDL.
INFO [alembic.runtime.migration] Running upgrade f9a30386bd74 -> 620241d1153f, update time_grain_sqla
Migration on current DB took: 0.25 seconds
INFO [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO [alembic.runtime.migration] Will assume transactional DDL.
INFO [alembic.runtime.migration] Running downgrade 620241d1153f -> f9a30386bd74, update time_grain_sqla
Running with at least 10 entities of each model
- Adding 8 entities to the dbs model
Processing ████████████████████████████████ 100%
INFO [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO [alembic.runtime.migration] Will assume transactional DDL.
INFO [alembic.runtime.migration] Running upgrade f9a30386bd74 -> 620241d1153f, update time_grain_sqla
Migration for 10+ entities took: 0.39 seconds
INFO [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO [alembic.runtime.migration] Will assume transactional DDL.
INFO [alembic.runtime.migration] Running downgrade 620241d1153f -> f9a30386bd74, update time_grain_sqla
Running with at least 100 entities of each model
- Adding 90 entities to the dbs model
Processing ████████████████████████████████ 100%
- Adding 73 entities to the tables model
Processing ████████████████████████████████ 100%
INFO [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO [alembic.runtime.migration] Will assume transactional DDL.
INFO [alembic.runtime.migration] Running upgrade f9a30386bd74 -> 620241d1153f, update time_grain_sqla
Migration for 100+ entities took: 0.38 seconds
INFO [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO [alembic.runtime.migration] Will assume transactional DDL.
INFO [alembic.runtime.migration] Running downgrade 620241d1153f -> f9a30386bd74, update time_grain_sqla
Running with at least 1000 entities of each model
- Adding 900 entities to the dbs model
Processing ████████████████████████████████ 100%
- Adding 886 entities to the slices model
Processing ████████████████████████████████ 100%
- Adding 900 entities to the tables model
Processing ████████████████████████████████ 100%
INFO [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO [alembic.runtime.migration] Will assume transactional DDL.
INFO [alembic.runtime.migration] Running upgrade f9a30386bd74 -> 620241d1153f, update time_grain_sqla
Migration for 1000+ entities took: 0.31 seconds
Cleaning up DB
Revert DB to 620241d1153f? [y/N]: y
INFO [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO [alembic.runtime.migration] Will assume transactional DDL.
Reverted
Results:
Current: 0.25 s
10+: 0.39 s
100+: 0.38 s
1000+: 0.31 s
```
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15807:
URL: https://github.com/apache/superset/pull/15807#issuecomment-883786180
# [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15807](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d235aaa) into [master](https://codecov.io/gh/apache/superset/commit/cbd37801a0b6b2f53b39548dc84b9a247b4f5898?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cbd3780) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #15807 +/- ##
========================================
Coverage 76.91% 76.91%
========================================
Files 983 984 +1
Lines 51643 51776 +133
Branches 6991 6991
========================================
+ Hits 39720 39826 +106
- Misses 11699 11726 +27
Partials 224 224
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.81% <ø> (-0.02%)` | :arrow_down: |
| mysql | `81.55% <ø> (+<0.01%)` | :arrow_up: |
| postgres | `81.57% <ø> (+<0.01%)` | :arrow_up: |
| python | `81.66% <ø> (+<0.01%)` | :arrow_up: |
| sqlite | `81.18% <ø> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `72.25% <0.00%> (-1.74%)` | :arrow_down: |
| [superset/utils/retries.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvcmV0cmllcy5weQ==) | `100.00% <0.00%> (ø)` | |
| [superset/utils/webdriver.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvd2ViZHJpdmVyLnB5) | `80.00% <0.00%> (+0.51%)` | :arrow_up: |
| [superset/tasks/schedules.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `77.53% <0.00%> (+1.16%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [cbd3780...d235aaa](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #15807:
URL: https://github.com/apache/superset/pull/15807#issuecomment-883791955
This is not working in SQLite, but there's also a few other migrations that are not working because they drop columns without using `op.batch`:
- superset/migrations/versions/19e978e1b9c3_add_report_format_to_report_schedule_.py
- superset/migrations/versions/2e5a0ee25ed4_refractor_alerting.py
- superset/migrations/versions/301362411006_add_execution_id_to_report_execution_.py
- superset/migrations/versions/49b5a32daba5_add_report_schedules.py
- superset/migrations/versions/585b0b1a7b18_add_exec_info_to_saved_queries.py
- superset/migrations/versions/5daced1f0e76_reports_add_working_timeout_column.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15807:
URL: https://github.com/apache/superset/pull/15807#issuecomment-883786180
# [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15807](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d235aaa) into [master](https://codecov.io/gh/apache/superset/commit/cbd37801a0b6b2f53b39548dc84b9a247b4f5898?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cbd3780) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #15807 +/- ##
========================================
Coverage 76.91% 76.91%
========================================
Files 983 984 +1
Lines 51643 51776 +133
Branches 6991 6991
========================================
+ Hits 39720 39826 +106
- Misses 11699 11726 +27
Partials 224 224
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.81% <ø> (-0.02%)` | :arrow_down: |
| mysql | `81.55% <ø> (+<0.01%)` | :arrow_up: |
| postgres | `81.57% <ø> (ø)` | |
| python | `81.66% <ø> (+<0.01%)` | :arrow_up: |
| sqlite | `81.18% <ø> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `72.25% <0.00%> (-1.74%)` | :arrow_down: |
| [superset/utils/retries.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvcmV0cmllcy5weQ==) | `100.00% <0.00%> (ø)` | |
| [superset/utils/webdriver.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvd2ViZHJpdmVyLnB5) | `80.00% <0.00%> (+0.51%)` | :arrow_up: |
| [superset/tasks/schedules.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `77.53% <0.00%> (+1.16%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [cbd3780...d235aaa](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #15807:
URL: https://github.com/apache/superset/pull/15807#issuecomment-883786180
# [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15807](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d235aaa) into [master](https://codecov.io/gh/apache/superset/commit/cbd37801a0b6b2f53b39548dc84b9a247b4f5898?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cbd3780) will **decrease** coverage by `0.02%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #15807 +/- ##
==========================================
- Coverage 76.91% 76.88% -0.03%
==========================================
Files 983 983
Lines 51643 51643
Branches 6991 6991
==========================================
- Hits 39720 39708 -12
- Misses 11699 11711 +12
Partials 224 224
```
| Flag | Coverage Δ | |
|---|---|---|
| mysql | `?` | |
| postgres | `81.57% <ø> (ø)` | |
| python | `81.61% <ø> (-0.05%)` | :arrow_down: |
| sqlite | `81.18% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `94.04% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.83% <0.00%> (-0.78%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.58% <0.00%> (-0.44%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [cbd3780...d235aaa](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15807:
URL: https://github.com/apache/superset/pull/15807#discussion_r673571301
##########
File path: superset/migrations/versions/49b5a32daba5_add_report_schedules.py
##########
@@ -119,13 +120,17 @@ def upgrade():
)
+def has_unique_constraint(constraint_name: str, table_name: str) -> bool:
+ bind = op.get_bind()
+ inspector = Inspector.from_engine(bind)
+ unique_constraints = inspector.get_unique_constraints(table_name)
+ return constraint_name in {constraint["name"] for constraint in unique_constraints}
Review comment:
is constraint guaranteed to have a key of "name" for every engine?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15807:
URL: https://github.com/apache/superset/pull/15807#discussion_r673567666
##########
File path: scripts/benchmark_migration.py
##########
@@ -30,7 +30,7 @@
from flask_migrate import downgrade, upgrade
from graphlib import TopologicalSorter # pylint: disable=wrong-import-order
from progress.bar import ChargingBar
-from sqlalchemy import create_engine, inspect, Table
Review comment:
`Table` is not used anywhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15807:
URL: https://github.com/apache/superset/pull/15807#issuecomment-883786180
# [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15807](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d235aaa) into [master](https://codecov.io/gh/apache/superset/commit/cbd37801a0b6b2f53b39548dc84b9a247b4f5898?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cbd3780) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #15807 +/- ##
========================================
Coverage 76.91% 76.91%
========================================
Files 983 984 +1
Lines 51643 51776 +133
Branches 6991 6991
========================================
+ Hits 39720 39826 +106
- Misses 11699 11726 +27
Partials 224 224
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.81% <ø> (-0.02%)` | :arrow_down: |
| mysql | `81.54% <ø> (ø)` | |
| postgres | `81.57% <ø> (ø)` | |
| python | `81.66% <ø> (+<0.01%)` | :arrow_up: |
| sqlite | `81.18% <ø> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `72.25% <0.00%> (-1.74%)` | :arrow_down: |
| [superset/utils/retries.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvcmV0cmllcy5weQ==) | `100.00% <0.00%> (ø)` | |
| [superset/utils/webdriver.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvd2ViZHJpdmVyLnB5) | `80.00% <0.00%> (+0.51%)` | :arrow_up: |
| [superset/tasks/schedules.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `77.53% <0.00%> (+1.16%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [cbd3780...d235aaa](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida merged pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
betodealmeida merged pull request #15807:
URL: https://github.com/apache/superset/pull/15807
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15807:
URL: https://github.com/apache/superset/pull/15807#discussion_r673567924
##########
File path: superset/migrations/versions/49b5a32daba5_add_report_schedules.py
##########
@@ -119,13 +120,17 @@ def upgrade():
)
+def has_unique_constraint(constraint_name: str, table_name: str) -> bool:
+ bind = op.get_bind()
+ inspector = Inspector.from_engine(bind)
+ unique_constraints = inspector.get_unique_constraints(table_name)
+ return constraint_name in {constraint["name"] for constraint in unique_constraints}
Review comment:
I wonder if we should move this to `superset/utils/` and use it in all our new migrations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a change in pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15807:
URL: https://github.com/apache/superset/pull/15807#discussion_r674172137
##########
File path: superset/migrations/versions/49b5a32daba5_add_report_schedules.py
##########
@@ -119,13 +120,17 @@ def upgrade():
)
+def has_unique_constraint(constraint_name: str, table_name: str) -> bool:
+ bind = op.get_bind()
+ inspector = Inspector.from_engine(bind)
+ unique_constraints = inspector.get_unique_constraints(table_name)
+ return constraint_name in {constraint["name"] for constraint in unique_constraints}
Review comment:
@betodealmeida it seems like we have similar logic like this [elsewhere](https://github.com/apache/superset/blob/62a8f2e193da3bdf1f43be085b5814be3dd476fa/superset/utils/core.py#L675-L684).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15807:
URL: https://github.com/apache/superset/pull/15807#discussion_r673571751
##########
File path: superset/migrations/versions/49b5a32daba5_add_report_schedules.py
##########
@@ -119,13 +120,17 @@ def upgrade():
)
+def has_unique_constraint(constraint_name: str, table_name: str) -> bool:
+ bind = op.get_bind()
+ inspector = Inspector.from_engine(bind)
+ unique_constraints = inspector.get_unique_constraints(table_name)
+ return constraint_name in {constraint["name"] for constraint in unique_constraints}
Review comment:
I just looked it up, that looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15807:
URL: https://github.com/apache/superset/pull/15807#issuecomment-883786180
# [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15807](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d235aaa) into [master](https://codecov.io/gh/apache/superset/commit/cbd37801a0b6b2f53b39548dc84b9a247b4f5898?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cbd3780) will **increase** coverage by `0.01%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #15807 +/- ##
==========================================
+ Coverage 76.91% 76.92% +0.01%
==========================================
Files 983 984 +1
Lines 51643 51776 +133
Branches 6991 6991
==========================================
+ Hits 39720 39829 +109
- Misses 11699 11723 +24
Partials 224 224
```
| Flag | Coverage Δ | |
|---|---|---|
| mysql | `81.54% <ø> (ø)` | |
| postgres | `81.57% <ø> (ø)` | |
| python | `81.66% <ø> (+<0.01%)` | :arrow_up: |
| sqlite | `81.18% <ø> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/utils/retries.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvcmV0cmllcy5weQ==) | `100.00% <0.00%> (ø)` | |
| [superset/utils/webdriver.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvd2ViZHJpdmVyLnB5) | `80.00% <0.00%> (+0.51%)` | :arrow_up: |
| [superset/tasks/schedules.py](https://codecov.io/gh/apache/superset/pull/15807/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `77.53% <0.00%> (+1.16%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [cbd3780...d235aaa](https://codecov.io/gh/apache/superset/pull/15807?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15807: fix: migration script can't drop constraint
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15807:
URL: https://github.com/apache/superset/pull/15807#discussion_r673567764
##########
File path: scripts/benchmark_migration.py
##########
@@ -160,7 +160,6 @@ def main(
rows = session.query(model).count()
print(f"- {model.__name__} ({rows} rows in table {model.__tablename__})")
model_rows[model] = rows
- session.close()
Review comment:
Not sure why we were closing the session here, but it's not needed and actually could cause problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org