Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add UUID column to ImportMixin #11098

Merged
merged 18 commits into from
Oct 7, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset-frontend/src/dashboard/actions/sliceEntities.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export function fetchAllSlices(userId) {
slice_id: slice.id,
slice_url: slice.slice_url,
slice_name: slice.slice_name,
uuid: slice.uuid,
edit_url: slice.edit_url,
form_data,
datasource_name: slice.datasource_name_text,
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/dashboard/components/SliceAdder.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ class SliceAdder extends React.Component {
const meta = {
chartId: cellData.slice_id,
sliceName: cellData.slice_name,
uuid: cellData.uuid,
};
return (
<DragDroppable
Expand Down
2 changes: 2 additions & 0 deletions superset-frontend/src/dashboard/reducers/getInitialState.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ export default function getInitialState(bootstrapData) {
slice_id: key,
slice_url: slice.slice_url,
slice_name: slice.slice_name,
uuid: slice.slice_uuid,
form_data: slice.form_data,
edit_url: slice.edit_url,
viz_type: slice.form_data.viz_type,
Expand Down Expand Up @@ -158,6 +159,7 @@ export default function getInitialState(bootstrapData) {
CHART_TYPE,
{
chartId: slice.slice_id,
uuid: slice.uuid,
},
(newSlicesContainer.parents || []).slice(),
);
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/dashboard/util/propShapes.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export const slicePropShape = PropTypes.shape({
slice_id: PropTypes.number.isRequired,
slice_url: PropTypes.string.isRequired,
slice_name: PropTypes.string.isRequired,
uuid: PropTypes.string.isRequired,
edit_url: PropTypes.string.isRequired,
datasource: PropTypes.string,
datasource_name: PropTypes.string,
Expand Down
Original file line number Diff line number Diff line change
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, @bkyryliuk! I'll add it today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 and maybe testing it on your Superset deployments as well?

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,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

# populate column
objects = session.query(model).all()
uuid_maps[table_name] = add_uuids(objects, session)

# add uniqueness constraint
with op.batch_alter_table(table_name) as batch_op:
batch_op.create_unique_constraint(f"uq_{table_name}_uuid", ["uuid"])

# add UUID to Dashboard.position_json
Dashboard = models["dashboards"]
for dashboard in session.query(Dashboard).all():
update_position_json(dashboard, session, uuid_maps["slices"])


def downgrade():
bind = op.get_bind()
session = db.Session(bind=bind)

# remove uuid from position_json
Dashboard = models["dashboards"]
for dashboard in session.query(Dashboard).all():
update_position_json(dashboard, session, {})

# remove uuid column
for table_name, model in models.items():
with op.batch_alter_table(model) as batch_op:
batch_op.drop_constraint(f"uq_{table_name}_uuid")
batch_op.drop_column("uuid")
6 changes: 6 additions & 0 deletions superset/models/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import json
import logging
import re
import uuid
from datetime import datetime, timedelta
from json.decoder import JSONDecodeError
from typing import Any, Dict, List, Optional, Set, Union
Expand All @@ -35,6 +36,7 @@
from sqlalchemy.ext.declarative import declared_attr
from sqlalchemy.orm import Mapper, Session
from sqlalchemy.orm.exc import MultipleResultsFound
from sqlalchemy_utils import UUIDType

from superset.utils.core import QueryStatus

Expand All @@ -51,6 +53,10 @@ def json_to_dict(json_str: str) -> Dict[Any, Any]:


class ImportMixin:
uuid = sa.Column(
betodealmeida marked this conversation as resolved.
Show resolved Hide resolved
UUIDType(binary=False), primary_key=False, unique=True, default=uuid.uuid4
)

export_parent: Optional[str] = None
# The name of the attribute
# with the SQL Alchemy back reference
Expand Down
7 changes: 4 additions & 3 deletions superset/models/slice.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,20 +193,21 @@ def data(self) -> Dict[str, Any]:
"slice_id": self.id,
"slice_name": self.slice_name,
"slice_url": self.slice_url,
"uuid": str(self.uuid),
}

@property
def digest(self) -> str:
"""
Returns a MD5 HEX digest that makes this dashboard unique
Returns a MD5 HEX digest that makes this dashboard unique
"""
return utils.md5_hex(self.params)

@property
def thumbnail_url(self) -> str:
"""
Returns a thumbnail URL with a HEX digest. We want to avoid browser cache
if the dashboard has changed
Returns a thumbnail URL with a HEX digest. We want to avoid browser cache
if the dashboard has changed
"""
return f"/api/v1/chart/{self.id}/thumbnail/{self.digest}/"

Expand Down
4 changes: 3 additions & 1 deletion superset/utils/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -310,13 +310,15 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
super().__init__(*args, **kwargs)
self.sort_keys = True

def default(self, o: Any) -> Dict[Any, Any]:
def default(self, o: Any) -> Union[Dict[Any, Any], str]:
try:
vals = {k: v for k, v in o.__dict__.items() if k != "_sa_instance_state"}
return {"__{}__".format(o.__class__.__name__): vals}
except Exception: # pylint: disable=broad-except
if isinstance(o, datetime):
return {"__datetime__": o.replace(microsecond=0).isoformat()}
if isinstance(o, uuid.UUID):
return str(o)
return json.JSONEncoder(sort_keys=True).default(o)


Expand Down
1 change: 1 addition & 0 deletions superset/views/chart/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,5 +105,6 @@ class SliceAsync(SliceModelView): # pylint: disable=too-many-ancestors
"slice_name",
"slice_url",
"viz_type",
"uuid",
]
label_columns = {"icons": " ", "slice_link": _("Chart")}