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 2020/09/29 03:20:25 UTC
[GitHub] [incubator-superset] betodealmeida opened a new pull request #11098: Add UUID column to ImportMixin
betodealmeida opened a new pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR is a simple rewrite of https://github.com/apache/incubator-superset/pull/7829, adding a UUID column to the `ImportMixin`. This initial work will be used to improve the import/export functionality in Superset by producing artifacts that are not dependent on the primary keys of a particular database.
<!-- ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF -->
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
```bash
$ superset db upgrade
$ superset db downgrade e5ef6828ac4e
```
Confirmed that columns are created and populated.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [X] Requires DB Migration.
- [X] Confirm DB Migration upgrade and downgrade tested.
- [ ] 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.
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] [incubator-superset] betodealmeida edited a comment on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida edited a comment on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-705230232
For some reason this PR broke master (https://github.com/apache/incubator-superset/commit/9785667a0dc970fd7b7033f172c2760ee0c22a6e errored), working on a fix.
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-700422449
> One problem with db migrations is that they cannot be cherry-picked out of order. Or they can if they both reference the same parent and ultimately need a converging migration. All this is fairly confusing. Less migrations is probably better. Either way I'd advise release managers to avoid cherry-picking anything with a migration.
Right. The case I was thinking of was when you want to cherry-pick feature `B`, and it has a database migration `Mb` that comes after another migration `Ma` that also needs to be cherry-picked. If the PR implementing `Ma` and the feature `A` is big it's harder to cherry-pick it, but you're forced to do it because of the Alembic DAG.
If instead you separate the feature `A` into one PR, and the migration `Ma` into another, someone interested in cherry-picking feature `B` can just cherry-pick the actual migration `Ma` and skip the PR implementing `A`, assuming that the DB migration is non-disruptive — eg, adding a column like we're doing here.
But in this case it doesn't matter, because the migration changing `position_json` is disruptive, and can't be separated from the changes in the logic to read the new schema. So I'll go ahead and implement the migration of `position_json` in this PR to consolidate the migrations.
Thanks, Max!
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on a change in pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r496363499
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin_py.py
##########
@@ -0,0 +1,111 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin.py
+
+Revision ID: b56500de1855
+Revises: e5ef6828ac4e
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "e5ef6828ac4e"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=False), primary_key=False, default=uuid.uuid4)
+
+
+table_names = [
+ # Core models
+ "dbs",
+ "dashboards",
+ "slices",
+ # SQLAlchemy connectors
+ "tables",
+ "table_columns",
+ "sql_metrics",
+ # Druid connector
+ "clusters",
+ "datasources",
+ "columns",
+ "metrics",
+]
+models = [
+ type(table_name, (Base, ImportMixin), {"__tablename__": table_name})
+ for table_name in table_names
+]
+
+
+def batch_commit(objects, session, batch_size=100):
+ count = len(objects)
+ for i, object_ in enumerate(objects):
+ object_.uuid = uuid.uuid4()
+ session.merge(object_)
+ if (i + 1) % batch_size == 0:
+ session.commit()
+ print(f"uuid assigned to {i + 1} out of {count}")
+ session.commit()
+ print(f"Done! Assigned {count} uuids")
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for model in models:
+ # add column with NULLs
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "uuid",
+ UUIDType(binary=False),
+ primary_key=False,
+ default=uuid.uuid4,
+ )
+ )
+
+ # populate column
+ objects = session.query(model).all()
+ batch_commit(objects, session)
+
+ # add uniqueness constraint
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.create_unique_constraint("uq_uuid", ["uuid"])
+
+
+def downgrade():
+ for model in models:
+ try:
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.drop_column("uuid")
+ batch_op.drop_constraint("uq_uuid")
+ except Exception:
+ pass
Review comment:
I'd at least `logging.warning` here
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-704501773
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@94d4d55`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `14.81%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #11098 +/- ##
=========================================
Coverage ? 61.60%
=========================================
Files ? 829
Lines ? 39195
Branches ? 3688
=========================================
Hits ? 24148
Misses ? 14866
Partials ? 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `62.30% <ø> (?)` | |
| #python | `61.19% <14.81%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ns/b56500de1855\_add\_uuid\_column\_to\_import\_mixin.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9iNTY1MDBkZTE4NTVfYWRkX3V1aWRfY29sdW1uX3RvX2ltcG9ydF9taXhpbi5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/models/slice.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `88.02% <ø> (ø)` | |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.26% <0.00%> (ø)` | |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `94.38% <100.00%> (ø)` | |
| [superset/datasets/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvc2NoZW1hcy5weQ==) | `94.28% <100.00%> (ø)` | |
| [superset/models/helpers.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | `87.44% <100.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `89.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=footer). Last update [94d4d55...aad4f87](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-701021326
```
qlite> SELECT position_json FROM dashboards WHERE id=8;
{
"CHART-Hkx6154FEm": {
"children": [],
"id": "CHART-Hkx6154FEm",
"meta": {
"chartId": 82,
"height": 30,
"sliceName": "slice 1",
"width": 4
},
"type": "CHART"
},
"GRID_ID": {
"children": [
"ROW-SyT19EFEQ"
],
"id": "GRID_ID",
"type": "GRID"
},
"ROOT_ID": {
"children": [
"GRID_ID"
],
"id": "ROOT_ID",
"type": "ROOT"
},
"ROW-SyT19EFEQ": {
"children": [
"CHART-Hkx6154FEm"
],
"id": "ROW-SyT19EFEQ",
"meta": {
"background": "BACKGROUND_TRANSPARENT"
},
"type": "ROW"
},
"DASHBOARD_VERSION_KEY": "v2"
}
sqlite> SELECT position_json FROM dashboards WHERE id=8;
{
"CHART-Hkx6154FEm": {
"children": [],
"id": "CHART-Hkx6154FEm",
"meta": {
"chartId": 82,
"height": 30,
"sliceName": "slice 1",
"width": 4,
"uuid": "706c8c3c-175b-4606-9016-4ef7e2ebff09"
},
"type": "CHART"
},
"GRID_ID": {
"children": [
"ROW-SyT19EFEQ"
],
"id": "GRID_ID",
"type": "GRID"
},
"ROOT_ID": {
"children": [
"GRID_ID"
],
"id": "ROOT_ID",
"type": "ROOT"
},
"ROW-SyT19EFEQ": {
"children": [
"CHART-Hkx6154FEm"
],
"id": "ROW-SyT19EFEQ",
"meta": {
"background": "BACKGROUND_TRANSPARENT"
},
"type": "ROW"
},
"DASHBOARD_VERSION_KEY": "v2"
}
sqlite> SELECT * FROM slices WHERE uuid=REPLACE('706c8c3c-175b-4606-9016-4ef7e2ebff09', '-', '');
2020-09-23 12:21:27.651468|2020-09-23 12:43:27.098505|82|Unicode Cloud|table||word_cloud|{"granularity_sqla": "dttm", "groupby": [], "limit": "100", "metric": {"aggregate": "SUM", "column": {"column_name": "value"}, "expressionType": "SIMPLE", "label": "Value"}, "rotation": "square", "row_limit": 50000, "series": "short_phrase", "since": "100 years ago", "size_from": "10", "size_to": "70", "until": "now", "viz_type": "word_cloud", "remote_id": 33, "datasource_name": "unicode_test", "schema": null, "database_name": "examples", "import_time": 1600890207}|2|2|||[examples].[unicode_test](id:4)|4||706c8c3c175b460690164ef7e2ebff09
sqlite>
```
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-704501773
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@94d4d55`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `14.81%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #11098 +/- ##
=========================================
Coverage ? 61.60%
=========================================
Files ? 829
Lines ? 39195
Branches ? 3688
=========================================
Hits ? 24145
Misses ? 14869
Partials ? 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `62.30% <ø> (?)` | |
| #python | `61.18% <14.81%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ns/b56500de1855\_add\_uuid\_column\_to\_import\_mixin.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9iNTY1MDBkZTE4NTVfYWRkX3V1aWRfY29sdW1uX3RvX2ltcG9ydF9taXhpbi5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/models/slice.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `88.02% <ø> (ø)` | |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.26% <0.00%> (ø)` | |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `94.38% <100.00%> (ø)` | |
| [superset/datasets/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvc2NoZW1hcy5weQ==) | `94.28% <100.00%> (ø)` | |
| [superset/models/helpers.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | `87.44% <100.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `89.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=footer). Last update [94d4d55...921960a](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] eschutho commented on a change in pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r497085884
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin_py.py
##########
@@ -0,0 +1,113 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin.py
+
+Revision ID: b56500de1855
+Revises: e5ef6828ac4e
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "e5ef6828ac4e"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=False), primary_key=False, default=uuid.uuid4)
+
+
+table_names = [
+ # Core models
+ "dbs",
+ "dashboards",
+ "slices",
+ # SQLAlchemy connectors
+ "tables",
+ "table_columns",
+ "sql_metrics",
+ # Druid connector
+ "clusters",
+ "datasources",
+ "columns",
+ "metrics",
+]
+models = [
+ type(table_name, (Base, ImportMixin), {"__tablename__": table_name})
+ for table_name in table_names
+]
+
+
+def batch_commit(objects, session, batch_size=100):
+ count = len(objects)
+ for i, object_ in enumerate(objects):
+ object_.uuid = uuid.uuid4()
+ session.merge(object_)
+ if (i + 1) % batch_size == 0:
+ session.commit()
+ print(f"uuid assigned to {i + 1} out of {count}")
+ session.commit()
+ print(f"Done! Assigned {count} uuids")
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for model in models:
+ # add column with NULLs
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "uuid",
+ UUIDType(binary=False),
+ primary_key=False,
+ default=uuid.uuid4,
+ )
+ )
+
+ # populate column
+ objects = session.query(model).all()
+ batch_commit(objects, session)
+
+ # add uniqueness constraint
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.create_unique_constraint("uq_uuid", ["uuid"])
Review comment:
are we planning to do any lookups by uuid? Should we add an index on those columns if so?
----------------------------------------------------------------
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.
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] [incubator-superset] bkyryliuk commented on a change in pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r501853903
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin.py
##########
@@ -0,0 +1,154 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin
+
+Revision ID: b56500de1855
+Revises: 18532d70ab98
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import json
+import logging
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+from superset.utils import core as utils
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "18532d70ab98"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
Review comment:
@betodealmeida this migration is very slow, it is worth to mention in the changelog e.g. for our staging env it took ~30 min and often it means extra downtime for the service
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on a change in pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r496363394
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin_py.py
##########
@@ -0,0 +1,111 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin.py
+
+Revision ID: b56500de1855
+Revises: e5ef6828ac4e
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "e5ef6828ac4e"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=False), primary_key=False, default=uuid.uuid4)
+
+
+table_names = [
+ # Core models
+ "dbs",
+ "dashboards",
+ "slices",
+ # SQLAlchemy connectors
+ "tables",
+ "table_columns",
+ "sql_metrics",
+ # Druid connector
+ "clusters",
+ "datasources",
+ "columns",
+ "metrics",
+]
+models = [
+ type(table_name, (Base, ImportMixin), {"__tablename__": table_name})
+ for table_name in table_names
+]
+
+
+def batch_commit(objects, session, batch_size=100):
+ count = len(objects)
+ for i, object_ in enumerate(objects):
+ object_.uuid = uuid.uuid4()
+ session.merge(object_)
+ if (i + 1) % batch_size == 0:
+ session.commit()
+ print(f"uuid assigned to {i + 1} out of {count}")
+ session.commit()
+ print(f"Done! Assigned {count} uuids")
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for model in models:
+ # add column with NULLs
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "uuid",
+ UUIDType(binary=False),
+ primary_key=False,
+ default=uuid.uuid4,
+ )
+ )
+
+ # populate column
+ objects = session.query(model).all()
+ batch_commit(objects, session)
+
+ # add uniqueness constraint
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.create_unique_constraint("uq_uuid", ["uuid"])
+
+
+def downgrade():
+ for model in models:
+ try:
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.drop_column("uuid")
Review comment:
I'm not sure if you tested the downgrade, but I think it's more likely to work if you drop the constraint first.
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io commented on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-704501773
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@94d4d55`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `14.81%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #11098 +/- ##
=========================================
Coverage ? 60.41%
=========================================
Files ? 392
Lines ? 24562
Branches ? 0
=========================================
Hits ? 14838
Misses ? 9724
Partials ? 0
```
| Flag | Coverage Δ | |
|---|---|---|
| #python | `60.41% <14.81%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ns/b56500de1855\_add\_uuid\_column\_to\_import\_mixin.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9iNTY1MDBkZTE4NTVfYWRkX3V1aWRfY29sdW1uX3RvX2ltcG9ydF9taXhpbi5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/models/slice.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `88.02% <ø> (ø)` | |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.01% <0.00%> (ø)` | |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `94.38% <100.00%> (ø)` | |
| [superset/datasets/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvc2NoZW1hcy5weQ==) | `94.28% <100.00%> (ø)` | |
| [superset/models/helpers.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | `87.44% <100.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `89.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=footer). Last update [94d4d55...aad4f87](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-704501773
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@94d4d55`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `14.81%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #11098 +/- ##
=========================================
Coverage ? 61.00%
=========================================
Files ? 392
Lines ? 24564
Branches ? 0
=========================================
Hits ? 14986
Misses ? 9578
Partials ? 0
```
| Flag | Coverage Δ | |
|---|---|---|
| #python | `61.00% <14.81%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ns/b56500de1855\_add\_uuid\_column\_to\_import\_mixin.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9iNTY1MDBkZTE4NTVfYWRkX3V1aWRfY29sdW1uX3RvX2ltcG9ydF9taXhpbi5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/models/slice.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `88.02% <ø> (ø)` | |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.26% <0.00%> (ø)` | |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `94.38% <100.00%> (ø)` | |
| [superset/datasets/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvc2NoZW1hcy5weQ==) | `94.28% <100.00%> (ø)` | |
| [superset/models/helpers.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | `87.44% <100.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `89.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=footer). Last update [94d4d55...aad4f87](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] bkyryliuk commented on a change in pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r501853903
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin.py
##########
@@ -0,0 +1,154 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin
+
+Revision ID: b56500de1855
+Revises: 18532d70ab98
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import json
+import logging
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+from superset.utils import core as utils
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "18532d70ab98"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
Review comment:
@betodealmeida this migration is very slow, it is worth to mention in the changelog e.g. for our staging env it took ~30 min and often it means extra downtime for the service
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on a change in pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r502057119
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin.py
##########
@@ -0,0 +1,154 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin
+
+Revision ID: b56500de1855
+Revises: 18532d70ab98
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import json
+import logging
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+from superset.utils import core as utils
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "18532d70ab98"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
Review comment:
Good point, @bkyryliuk! I'll add it today.
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-700415440
> Overall looks simpler than I thought it would be. In the interest of minimizing the number of database migrations, do we want to alter `Dashboard.json_metadata` in the same migration?
I'm not sure... on one hand, doing it in this PR would reduce the number of migrations, which is great. On the other hand, having PRs with DB migrations being as small as possible makes it easier to cherry pick them.
Do you have any preferences? I think updating the column `Dashboard.position_json` to use UUIDs is simple, but I'm worried about updating the logic that touches it. We'd also have to update the examples.
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on a change in pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r499740409
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin.py
##########
@@ -0,0 +1,154 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin
+
+Revision ID: b56500de1855
+Revises: 18532d70ab98
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import json
+import logging
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+from superset.utils import core as utils
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "18532d70ab98"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=False), primary_key=False, default=uuid.uuid4)
+
+
+table_names = [
+ # Core models
+ "dbs",
+ "dashboards",
+ "slices",
+ # SQLAlchemy connectors
+ "tables",
+ "table_columns",
+ "sql_metrics",
+ # Druid connector
+ "clusters",
+ "datasources",
+ "columns",
+ "metrics",
+ # Dashboard email schedules
+ "dashboard_email_schedules",
+ "slice_email_schedules",
+]
+models = {
+ table_name: type(table_name, (Base, ImportMixin), {"__tablename__": table_name})
+ for table_name in table_names
+}
+
+models["dashboards"].position_json = sa.Column(utils.MediumText())
+
+
+def add_uuids(objects, session, batch_size=100):
+ uuid_map = {}
+ count = len(objects)
+ for i, object_ in enumerate(objects):
+ object_.uuid = uuid.uuid4()
+ uuid_map[object_.id] = object_.uuid
+ session.merge(object_)
+ if (i + 1) % batch_size == 0:
+ session.commit()
+ print(f"uuid assigned to {i + 1} out of {count}")
+
+ session.commit()
+ print(f"Done! Assigned {count} uuids")
+
+ return uuid_map
+
+
+def update_position_json(dashboard, session, uuid_map):
+ layout = json.loads(dashboard.position_json or "{}")
+ for object_ in layout.values():
+ if (
+ isinstance(object_, dict)
+ and object_["type"] == "CHART"
+ and object_["meta"]["chartId"]
+ ):
+ chart_id = object_["meta"]["chartId"]
+ if chart_id in uuid_map:
+ object_["meta"]["uuid"] = str(uuid_map[chart_id])
+ elif object_["meta"].get("uuid"):
+ del object_["meta"]["uuid"]
+
+ dashboard.position_json = json.dumps(layout, indent=4)
+ session.merge(dashboard)
+ session.commit()
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ uuid_maps = {}
+ for table_name, model in models.items():
+ with op.batch_alter_table(table_name) as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "uuid",
+ UUIDType(binary=False),
+ primary_key=False,
+ default=uuid.uuid4,
+ )
Review comment:
Thanks for the feedback, @villebro! I haven't tried running with `binary=True`, I'll give it a try as soon as I fix the unit tests that are not passing.
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on a change in pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r497200836
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin_py.py
##########
@@ -0,0 +1,113 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin.py
+
+Revision ID: b56500de1855
+Revises: e5ef6828ac4e
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "e5ef6828ac4e"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=False), primary_key=False, default=uuid.uuid4)
+
+
+table_names = [
+ # Core models
+ "dbs",
+ "dashboards",
+ "slices",
+ # SQLAlchemy connectors
+ "tables",
+ "table_columns",
+ "sql_metrics",
+ # Druid connector
+ "clusters",
+ "datasources",
+ "columns",
+ "metrics",
+]
+models = [
+ type(table_name, (Base, ImportMixin), {"__tablename__": table_name})
+ for table_name in table_names
+]
+
+
+def batch_commit(objects, session, batch_size=100):
+ count = len(objects)
+ for i, object_ in enumerate(objects):
+ object_.uuid = uuid.uuid4()
+ session.merge(object_)
+ if (i + 1) % batch_size == 0:
+ session.commit()
+ print(f"uuid assigned to {i + 1} out of {count}")
+ session.commit()
+ print(f"Done! Assigned {count} uuids")
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for model in models:
+ # add column with NULLs
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "uuid",
+ UUIDType(binary=False),
+ primary_key=False,
+ default=uuid.uuid4,
+ )
+ )
+
+ # populate column
+ objects = session.query(model).all()
+ batch_commit(objects, session)
+
+ # add uniqueness constraint
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.create_unique_constraint("uq_uuid", ["uuid"])
Review comment:
Good point. I think not in the near future, we're using it just to ensure consistent relationships.
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on a change in pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r499216449
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin.py
##########
@@ -0,0 +1,154 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin
+
+Revision ID: b56500de1855
+Revises: 18532d70ab98
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import json
+import logging
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+from superset.utils import core as utils
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "18532d70ab98"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=False), primary_key=False, default=uuid.uuid4)
+
+
+table_names = [
+ # Core models
+ "dbs",
+ "dashboards",
+ "slices",
+ # SQLAlchemy connectors
+ "tables",
+ "table_columns",
+ "sql_metrics",
+ # Druid connector
+ "clusters",
+ "datasources",
+ "columns",
+ "metrics",
+ # Dashboard email schedules
+ "dashboard_email_schedules",
+ "slice_email_schedules",
+]
+models = {
+ table_name: type(table_name, (Base, ImportMixin), {"__tablename__": table_name})
+ for table_name in table_names
+}
+
+models["dashboards"].position_json = sa.Column(utils.MediumText())
+
+
+def add_uuids(objects, session, batch_size=100):
+ uuid_map = {}
+ count = len(objects)
+ for i, object_ in enumerate(objects):
+ object_.uuid = uuid.uuid4()
+ uuid_map[object_.id] = object_.uuid
+ session.merge(object_)
+ if (i + 1) % batch_size == 0:
+ session.commit()
+ print(f"uuid assigned to {i + 1} out of {count}")
+
+ session.commit()
+ print(f"Done! Assigned {count} uuids")
+
+ return uuid_map
+
+
+def update_position_json(dashboard, session, uuid_map):
+ layout = json.loads(dashboard.position_json or "{}")
+ for object_ in layout.values():
+ if (
+ isinstance(object_, dict)
+ and object_["type"] == "CHART"
+ and object_["meta"]["chartId"]
+ ):
+ chart_id = object_["meta"]["chartId"]
+ if chart_id in uuid_map:
+ object_["meta"]["uuid"] = str(uuid_map[chart_id])
+ elif object_["meta"].get("uuid"):
+ del object_["meta"]["uuid"]
+
+ dashboard.position_json = json.dumps(layout, indent=4)
+ session.merge(dashboard)
+ session.commit()
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ uuid_maps = {}
+ for table_name, model in models.items():
+ with op.batch_alter_table(table_name) as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "uuid",
+ UUIDType(binary=False),
+ primary_key=False,
+ default=uuid.uuid4,
+ )
Review comment:
I've had compatibility issues when using `sqlalchemy_utils.UUIDType` on different databases some time ago (I believe I was mixing Postgres and Sqlite at the time). I believe the resolution back then was to use `binary=False` like you've done, but I believe that eliminates the performance benefits of using a `UUIDType` over a traditional `CHAR`/`VARCHAR` implementation. DId you try running it with `binary=True`, did that cause CI trouble on Sqlite vs Postgres vs MySQL?
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida edited a comment on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida edited a comment on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-705230232
For some reason this PR broke master (https://github.com/apache/incubator-superset/commit/9785667a0dc970fd7b7033f172c2760ee0c22a6e errored), working on a fix on https://github.com/apache/incubator-superset/pull/11196.
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-704501773
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@94d4d55`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `14.81%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #11098 +/- ##
=========================================
Coverage ? 61.18%
=========================================
Files ? 829
Lines ? 39195
Branches ? 3688
=========================================
Hits ? 23981
Misses ? 15033
Partials ? 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `62.30% <ø> (?)` | |
| #python | `60.51% <14.81%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ns/b56500de1855\_add\_uuid\_column\_to\_import\_mixin.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9iNTY1MDBkZTE4NTVfYWRkX3V1aWRfY29sdW1uX3RvX2ltcG9ydF9taXhpbi5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/models/slice.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `88.02% <ø> (ø)` | |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.26% <0.00%> (ø)` | |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `94.38% <100.00%> (ø)` | |
| [superset/datasets/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvc2NoZW1hcy5weQ==) | `94.28% <100.00%> (ø)` | |
| [superset/models/helpers.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | `87.44% <100.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `89.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=footer). Last update [94d4d55...921960a](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-705230232
For some reason this PR broke master, working on a fix.
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida edited a comment on pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida edited a comment on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-701021326
```
-- BEFORE migration
sqlite> SELECT position_json FROM dashboards WHERE id=8;
{
"CHART-Hkx6154FEm": {
"children": [],
"id": "CHART-Hkx6154FEm",
"meta": {
"chartId": 82,
"height": 30,
"sliceName": "slice 1",
"width": 4
},
"type": "CHART"
},
"GRID_ID": {
"children": [
"ROW-SyT19EFEQ"
],
"id": "GRID_ID",
"type": "GRID"
},
"ROOT_ID": {
"children": [
"GRID_ID"
],
"id": "ROOT_ID",
"type": "ROOT"
},
"ROW-SyT19EFEQ": {
"children": [
"CHART-Hkx6154FEm"
],
"id": "ROW-SyT19EFEQ",
"meta": {
"background": "BACKGROUND_TRANSPARENT"
},
"type": "ROW"
},
"DASHBOARD_VERSION_KEY": "v2"
}
-- AFTER migration
sqlite> SELECT position_json FROM dashboards WHERE id=8;
{
"CHART-Hkx6154FEm": {
"children": [],
"id": "CHART-Hkx6154FEm",
"meta": {
"chartId": 82,
"height": 30,
"sliceName": "slice 1",
"width": 4,
"uuid": "706c8c3c-175b-4606-9016-4ef7e2ebff09"
},
"type": "CHART"
},
"GRID_ID": {
"children": [
"ROW-SyT19EFEQ"
],
"id": "GRID_ID",
"type": "GRID"
},
"ROOT_ID": {
"children": [
"GRID_ID"
],
"id": "ROOT_ID",
"type": "ROOT"
},
"ROW-SyT19EFEQ": {
"children": [
"CHART-Hkx6154FEm"
],
"id": "ROW-SyT19EFEQ",
"meta": {
"background": "BACKGROUND_TRANSPARENT"
},
"type": "ROW"
},
"DASHBOARD_VERSION_KEY": "v2"
}
sqlite> SELECT * FROM slices WHERE uuid=REPLACE('706c8c3c-175b-4606-9016-4ef7e2ebff09', '-', '');
2020-09-23 12:21:27.651468|2020-09-23 12:43:27.098505|82|Unicode Cloud|table||word_cloud|{"granularity_sqla": "dttm", "groupby": [], "limit": "100", "metric": {"aggregate": "SUM", "column": {"column_name": "value"}, "expressionType": "SIMPLE", "label": "Value"}, "rotation": "square", "row_limit": 50000, "series": "short_phrase", "since": "100 years ago", "size_from": "10", "size_to": "70", "until": "now", "viz_type": "word_cloud", "remote_id": 33, "datasource_name": "unicode_test", "schema": null, "database_name": "examples", "import_time": 1600890207}|2|2|||[examples].[unicode_test](id:4)|4||706c8c3c175b460690164ef7e2ebff09
sqlite>
```
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on a change in pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r496363957
##########
File path: superset/models/helpers.py
##########
@@ -51,6 +53,10 @@ def json_to_dict(json_str: str) -> Dict[Any, Any]:
class ImportMixin:
+ uuid = sa.Column(
Review comment:
Probably out of scope of this PR, but I think we should rename `ImportMixin` to `ExportImportMixin` eventually
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on a change in pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r496365110
##########
File path: superset/models/helpers.py
##########
@@ -51,6 +53,10 @@ def json_to_dict(json_str: str) -> Dict[Any, Any]:
class ImportMixin:
+ uuid = sa.Column(
Review comment:
Right, I'm planning to do it once we start working on the actual import/export refactoring.
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on a change in pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r496363644
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin_py.py
##########
@@ -0,0 +1,111 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin.py
+
+Revision ID: b56500de1855
+Revises: e5ef6828ac4e
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "e5ef6828ac4e"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=False), primary_key=False, default=uuid.uuid4)
+
+
+table_names = [
+ # Core models
+ "dbs",
+ "dashboards",
+ "slices",
+ # SQLAlchemy connectors
+ "tables",
+ "table_columns",
+ "sql_metrics",
+ # Druid connector
+ "clusters",
+ "datasources",
+ "columns",
+ "metrics",
+]
+models = [
+ type(table_name, (Base, ImportMixin), {"__tablename__": table_name})
+ for table_name in table_names
+]
+
+
+def batch_commit(objects, session, batch_size=100):
+ count = len(objects)
+ for i, object_ in enumerate(objects):
+ object_.uuid = uuid.uuid4()
+ session.merge(object_)
+ if (i + 1) % batch_size == 0:
+ session.commit()
+ print(f"uuid assigned to {i + 1} out of {count}")
+ session.commit()
+ print(f"Done! Assigned {count} uuids")
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for model in models:
+ # add column with NULLs
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "uuid",
+ UUIDType(binary=False),
+ primary_key=False,
+ default=uuid.uuid4,
+ )
+ )
+
+ # populate column
+ objects = session.query(model).all()
+ batch_commit(objects, session)
+
+ # add uniqueness constraint
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.create_unique_constraint("uq_uuid", ["uuid"])
+
+
+def downgrade():
+ for model in models:
+ try:
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.drop_column("uuid")
Review comment:
Oh, good point. I did test in SQLite, but I noticed the tests are failing in Postgres, probably because of that.
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-704501773
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@94d4d55`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `14.81%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #11098 +/- ##
=========================================
Coverage ? 65.80%
=========================================
Files ? 829
Lines ? 39198
Branches ? 3688
=========================================
Hits ? 25794
Misses ? 13297
Partials ? 107
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `55.81% <ø> (?)` | |
| #javascript | `62.30% <ø> (?)` | |
| #python | `61.19% <14.81%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ns/b56500de1855\_add\_uuid\_column\_to\_import\_mixin.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9iNTY1MDBkZTE4NTVfYWRkX3V1aWRfY29sdW1uX3RvX2ltcG9ydF9taXhpbi5weQ==) | `0.00% <0.00%> (ø)` | |
| [superset/models/slice.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `88.02% <ø> (ø)` | |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.26% <0.00%> (ø)` | |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `94.38% <100.00%> (ø)` | |
| [superset/datasets/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvc2NoZW1hcy5weQ==) | `94.28% <100.00%> (ø)` | |
| [superset/models/helpers.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | `87.44% <100.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11098/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `89.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=footer). Last update [94d4d55...aad4f87](https://codecov.io/gh/apache/incubator-superset/pull/11098?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on a change in pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r502057119
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin.py
##########
@@ -0,0 +1,154 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin
+
+Revision ID: b56500de1855
+Revises: 18532d70ab98
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import json
+import logging
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+from superset.utils import core as utils
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "18532d70ab98"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
Review comment:
Good point, @bkyryliuk! I'll add it today.
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#issuecomment-700418066
One problem with db migrations is that they cannot be cherry-picked out of order. Or they can if they both reference the same parent and ultimately need a converging migration. All this is fairly confusing. Less migrations is probably better. Either way I'd advise release managers to avoid cherry-picking anything with a migration.
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida merged pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida merged pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098
----------------------------------------------------------------
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.
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] [incubator-superset] ktmud commented on a change in pull request #11098: feat: add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r502719809
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin.py
##########
@@ -0,0 +1,154 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin
+
+Revision ID: b56500de1855
+Revises: 18532d70ab98
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import json
+import logging
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+from superset.utils import core as utils
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "18532d70ab98"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
Review comment:
@bkyryliuk @betodealmeida I've managed to rewrite the uuid generation with native SQL queries and sped up the migration process by more than 100x. The whole migration job can now complete in under 5 minutes for our Superset db of more than 200k `slices` and 1 million `table_columns`. Do you mind [taking a look](https://github.com/apache/incubator-superset/pull/11209) and maybe testing it on your Superset deployments as well?
----------------------------------------------------------------
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.
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] [incubator-superset] betodealmeida commented on a change in pull request #11098: Add UUID column to ImportMixin
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #11098:
URL: https://github.com/apache/incubator-superset/pull/11098#discussion_r496364376
##########
File path: superset/migrations/versions/b56500de1855_add_uuid_column_to_import_mixin_py.py
##########
@@ -0,0 +1,111 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_uuid_column_to_import_mixin.py
+
+Revision ID: b56500de1855
+Revises: e5ef6828ac4e
+Create Date: 2020-09-28 17:57:23.128142
+
+"""
+import uuid
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+
+# revision identifiers, used by Alembic.
+revision = "b56500de1855"
+down_revision = "e5ef6828ac4e"
+
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=False), primary_key=False, default=uuid.uuid4)
+
+
+table_names = [
+ # Core models
+ "dbs",
+ "dashboards",
+ "slices",
+ # SQLAlchemy connectors
+ "tables",
+ "table_columns",
+ "sql_metrics",
+ # Druid connector
+ "clusters",
+ "datasources",
+ "columns",
+ "metrics",
+]
+models = [
+ type(table_name, (Base, ImportMixin), {"__tablename__": table_name})
+ for table_name in table_names
+]
+
+
+def batch_commit(objects, session, batch_size=100):
+ count = len(objects)
+ for i, object_ in enumerate(objects):
+ object_.uuid = uuid.uuid4()
+ session.merge(object_)
+ if (i + 1) % batch_size == 0:
+ session.commit()
+ print(f"uuid assigned to {i + 1} out of {count}")
+ session.commit()
+ print(f"Done! Assigned {count} uuids")
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for model in models:
+ # add column with NULLs
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "uuid",
+ UUIDType(binary=False),
+ primary_key=False,
+ default=uuid.uuid4,
+ )
+ )
+
+ # populate column
+ objects = session.query(model).all()
+ batch_commit(objects, session)
+
+ # add uniqueness constraint
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.create_unique_constraint("uq_uuid", ["uuid"])
+
+
+def downgrade():
+ for model in models:
+ try:
+ with op.batch_alter_table(model.__tablename__) as batch_op:
+ batch_op.drop_column("uuid")
+ batch_op.drop_constraint("uq_uuid")
+ except Exception:
+ pass
Review comment:
Good point, done.
----------------------------------------------------------------
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.
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