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 2022/06/10 07:46:27 UTC
[GitHub] [superset] zhaoyongjie opened a new pull request, #20346: feat: TreeMap migration
zhaoyongjie opened a new pull request, #20346:
URL: https://github.com/apache/superset/pull/20346
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR intends to introduce an approach that migrates from v1 viz to v2 viz. The first PR implement TreeMap migration.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
```shell
# upgrade
superset db upgrade
# downgrade
superset db downgrade
```
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r894334023
##########
superset/migrations/versions/2021-05-27_16-10_6f139c533bea_add_advanced_data_types_to_column_models.py:
##########
@@ -14,10 +14,12 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-"""adding_advanced_data_type.py
+"""adding advanced data type to column models
+
Revision ID: 6f139c533bea
Revises: cbe71abde154
Create Date: 2021-05-27 16:10:59.567684
+
Review Comment:
bycatch: pretty output
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20346:
URL: https://github.com/apache/superset/pull/20346#issuecomment-1176180074
@zhaoyongjie @rusackas Shouldn't we disable TreeMap v1 after the migration? By that, I mean no more TreeMap v1 on Add Slice and Chart Type filter in the charts list. Right now, the user can still create the old version which kind of defeats the whole purpose of the 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r894348577
##########
tests/unit_tests/conftest.py:
##########
@@ -68,7 +69,9 @@ def app() -> Iterator[SupersetApp]:
app = SupersetApp(__name__)
app.config.from_object("superset.config")
- app.config["SQLALCHEMY_DATABASE_URI"] = "sqlite://"
+ app.config["SQLALCHEMY_DATABASE_URI"] = (
+ os.environ.get("SUPERSET__SQLALCHEMY_DATABASE_URI") or "sqlite://"
+ )
Review Comment:
Use integration test database instance for local debugging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r894334023
##########
superset/migrations/versions/2021-05-27_16-10_6f139c533bea_add_advanced_data_types_to_column_models.py:
##########
@@ -14,10 +14,12 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-"""adding_advanced_data_type.py
+"""adding advanced data type to column models
+
Revision ID: 6f139c533bea
Revises: cbe71abde154
Create Date: 2021-05-27 16:10:59.567684
+
Review Comment:
bycatch: pretty output
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r899079359
##########
tests/unit_tests/utils/viz_migration/treemap_migration_test.py:
##########
@@ -0,0 +1,93 @@
+# 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.
+import json
+
+from superset.app import SupersetApp
+from superset.utils.migrate_viz import get_migrate_class, MigrateVizEnum
+
+treemap_form_data = """{
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": [
+ "Edward"
+ ],
+ "expressionType": "SIMPLE",
+ "filterOptionName": "filter_xhbus6irfa_r10k9nwmwy",
+ "isExtra": false,
+ "isNew": false,
+ "operator": "IN",
+ "operatorId": "IN",
+ "sqlExpression": null,
+ "subject": "name"
+ }
+ ],
+ "color_scheme": "bnbColors",
+ "datasource": "2__table",
+ "extra_form_data": {},
+ "granularity_sqla": "ds",
+ "groupby": [
+ "state",
+ "gender"
+ ],
+ "metrics": [
+ "sum__num"
+ ],
+ "number_format": ",d",
+ "order_desc": true,
+ "row_limit": 10,
+ "time_range": "No filter",
+ "timeseries_limit_metric": "sum__num",
+ "treemap_ratio": 1.618033988749895,
+ "viz_type": "treemap"
+}
+"""
+
+treemap_processor = get_migrate_class[MigrateVizEnum.treemap]
+
+
+def test_treemap_migrate(app_context: SupersetApp) -> None:
Review Comment:
Maybe have separated tests for upgrade and downgrade?
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
+ target_viz_type: str
+
+ def __init__(self, form_data: str) -> None:
+ self.data = json.loads(form_data)
+
+ def _pre_action(self) -> None:
+ """some actions before migrate"""
+
+ def _migrate(self) -> None:
+ if self.data.get("viz_type") != self.source_viz_type:
+ return
+
+ if "viz_type" in self.data:
+ self.data["viz_type"] = self.target_viz_type
+
+ rv_data = {}
+ for (key, value) in self.data.items():
+ if key in self.mapping_keys and self.mapping_keys[key] in rv_data:
+ raise ValueError("Duplicate key in target viz")
+
+ if key in self.mapping_keys:
+ rv_data[self.mapping_keys[key]] = value
+
+ if key in self.remove_keys:
+ continue
+
+ rv_data[key] = value
+
+ self.data = rv_data
+
+ def _post_action(self) -> None:
+ """some actions after migrate"""
+
+ @classmethod
+ def upgrade(cls, slc: Slice) -> Slice:
+ clz = cls(slc.params)
+ slc.viz_type = cls.target_viz_type
+ form_data_bak = clz.data.copy()
+
+ clz._pre_action()
+ clz._migrate()
+ clz._post_action()
+
+ # only backup params
+ slc.params = json.dumps({**clz.data, "form_data_bak": form_data_bak})
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = clz.data
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+ @classmethod
+ def downgrade(cls, slc: Slice) -> Slice:
+ form_data = json.loads(slc.params)
+ if "form_data_bak" in form_data and "viz_type" in form_data.get(
+ "form_data_bak"
+ ):
+ form_data_bak = form_data["form_data_bak"]
+ slc.params = json.dumps(form_data_bak)
+ slc.viz_type = form_data_bak.get("viz_type")
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = form_data_bak
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+
+class MigrateTreeMap(MigrateViz):
Review Comment:
Maybe move `MigrateTreeMap` to its own file? I guess we'll have a bunch of these.
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
+ target_viz_type: str
+
+ def __init__(self, form_data: str) -> None:
+ self.data = json.loads(form_data)
+
+ def _pre_action(self) -> None:
+ """some actions before migrate"""
+
+ def _migrate(self) -> None:
+ if self.data.get("viz_type") != self.source_viz_type:
+ return
+
+ if "viz_type" in self.data:
+ self.data["viz_type"] = self.target_viz_type
+
+ rv_data = {}
+ for (key, value) in self.data.items():
+ if key in self.mapping_keys and self.mapping_keys[key] in rv_data:
+ raise ValueError("Duplicate key in target viz")
+
+ if key in self.mapping_keys:
+ rv_data[self.mapping_keys[key]] = value
Review Comment:
I personally find this more readable, but it's not a blocker.
```suggestion
if key in self.mapping_keys:
mapped_key = self.mapping_keys[key]
if mapped_key in rv_data:
raise ValueError("Duplicate key in target viz")
rv_data[mapped_key] = value
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r915402075
##########
superset/migrations/versions/2022-06-30_22-04_c747c78868b6_migrating_legacy_treemap.py:
##########
@@ -0,0 +1,104 @@
+# 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.
+"""Migrating legacy TreeMap
+
+Revision ID: c747c78868b6
+Revises: e786798587de
+Create Date: 2022-06-30 22:04:17.686635
+
+"""
+
+# revision identifiers, used by Alembic.
+
+revision = "c747c78868b6"
+down_revision = "7fb8bca906d2"
+
+from alembic import op
+from sqlalchemy import and_, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils.migrate_viz import get_migrate_class, MigrateVizEnum
+
+treemap_processor = get_migrate_class[MigrateVizEnum.treemap]
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ slice_name = Column(String(250))
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices = session.query(Slice).filter(
+ Slice.viz_type == treemap_processor.source_viz_type
+ )
+ total = slices.count()
+ idx = 0
+ for slc in slices.yield_per(100):
+ try:
+ idx += 1
+ print(f"Upgrading ({idx}/{total}): {slc.slice_name}#{slc.id}")
+ new_viz = treemap_processor.upgrade(slc)
+ session.merge(new_viz)
+ session.commit()
Review Comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20346:
URL: https://github.com/apache/superset/pull/20346#issuecomment-1176254610
> @zhaoyongjie @rusackas Shouldn't we disable TreeMap v1 after the migration? By that, I mean no more TreeMap v1 on Add Slice and Chart Type filter in the charts list. Right now, the user can still create the old version which kind of defeats the whole purpose of the migration.
I intend to do a separated PR that removes treemap v1 after merged migration PR. the follow-up PR will 1) remove Legacy TreeMap from plugins, 2) remove legacy TreeMap from MainPresent.js.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r899095814
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
+ target_viz_type: str
+
+ def __init__(self, form_data: str) -> None:
+ self.data = json.loads(form_data)
+
+ def _pre_action(self) -> None:
+ """some actions before migrate"""
+
+ def _migrate(self) -> None:
+ if self.data.get("viz_type") != self.source_viz_type:
+ return
+
+ if "viz_type" in self.data:
+ self.data["viz_type"] = self.target_viz_type
+
+ rv_data = {}
+ for (key, value) in self.data.items():
+ if key in self.mapping_keys and self.mapping_keys[key] in rv_data:
+ raise ValueError("Duplicate key in target viz")
+
+ if key in self.mapping_keys:
+ rv_data[self.mapping_keys[key]] = value
+
+ if key in self.remove_keys:
+ continue
+
+ rv_data[key] = value
+
+ self.data = rv_data
+
+ def _post_action(self) -> None:
+ """some actions after migrate"""
+
+ @classmethod
+ def upgrade(cls, slc: Slice) -> Slice:
+ clz = cls(slc.params)
+ slc.viz_type = cls.target_viz_type
+ form_data_bak = clz.data.copy()
+
+ clz._pre_action()
+ clz._migrate()
+ clz._post_action()
+
+ # only backup params
+ slc.params = json.dumps({**clz.data, "form_data_bak": form_data_bak})
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = clz.data
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+ @classmethod
+ def downgrade(cls, slc: Slice) -> Slice:
+ form_data = json.loads(slc.params)
+ if "form_data_bak" in form_data and "viz_type" in form_data.get(
+ "form_data_bak"
+ ):
+ form_data_bak = form_data["form_data_bak"]
+ slc.params = json.dumps(form_data_bak)
+ slc.viz_type = form_data_bak.get("viz_type")
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = form_data_bak
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+
+class MigrateTreeMap(MigrateViz):
Review Comment:
This is a one-time script. There is no need to split moudles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r899094790
##########
tests/unit_tests/utils/viz_migration/treemap_migration_test.py:
##########
@@ -0,0 +1,93 @@
+# 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.
+import json
+
+from superset.app import SupersetApp
+from superset.utils.migrate_viz import get_migrate_class, MigrateVizEnum
+
+treemap_form_data = """{
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": [
+ "Edward"
+ ],
+ "expressionType": "SIMPLE",
+ "filterOptionName": "filter_xhbus6irfa_r10k9nwmwy",
+ "isExtra": false,
+ "isNew": false,
+ "operator": "IN",
+ "operatorId": "IN",
+ "sqlExpression": null,
+ "subject": "name"
+ }
+ ],
+ "color_scheme": "bnbColors",
+ "datasource": "2__table",
+ "extra_form_data": {},
+ "granularity_sqla": "ds",
+ "groupby": [
+ "state",
+ "gender"
+ ],
+ "metrics": [
+ "sum__num"
+ ],
+ "number_format": ",d",
+ "order_desc": true,
+ "row_limit": 10,
+ "time_range": "No filter",
+ "timeseries_limit_metric": "sum__num",
+ "treemap_ratio": 1.618033988749895,
+ "viz_type": "treemap"
+}
+"""
+
+treemap_processor = get_migrate_class[MigrateVizEnum.treemap]
+
+
+def test_treemap_migrate(app_context: SupersetApp) -> None:
Review Comment:
the method of downgrade needs an updated slice so combine upgrade and downgrade in the same unit test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r899106922
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
+ target_viz_type: str
+
+ def __init__(self, form_data: str) -> None:
+ self.data = json.loads(form_data)
+
+ def _pre_action(self) -> None:
+ """some actions before migrate"""
+
+ def _migrate(self) -> None:
+ if self.data.get("viz_type") != self.source_viz_type:
+ return
+
+ if "viz_type" in self.data:
+ self.data["viz_type"] = self.target_viz_type
+
+ rv_data = {}
+ for (key, value) in self.data.items():
+ if key in self.mapping_keys and self.mapping_keys[key] in rv_data:
+ raise ValueError("Duplicate key in target viz")
+
+ if key in self.mapping_keys:
+ rv_data[self.mapping_keys[key]] = value
+
+ if key in self.remove_keys:
+ continue
+
+ rv_data[key] = value
+
+ self.data = rv_data
+
+ def _post_action(self) -> None:
+ """some actions after migrate"""
+
+ @classmethod
+ def upgrade(cls, slc: Slice) -> Slice:
+ clz = cls(slc.params)
+ slc.viz_type = cls.target_viz_type
+ form_data_bak = clz.data.copy()
+
+ clz._pre_action()
+ clz._migrate()
+ clz._post_action()
+
+ # only backup params
+ slc.params = json.dumps({**clz.data, "form_data_bak": form_data_bak})
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = clz.data
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+ @classmethod
+ def downgrade(cls, slc: Slice) -> Slice:
+ form_data = json.loads(slc.params)
+ if "form_data_bak" in form_data and "viz_type" in form_data.get(
+ "form_data_bak"
+ ):
+ form_data_bak = form_data["form_data_bak"]
+ slc.params = json.dumps(form_data_bak)
+ slc.viz_type = form_data_bak.get("viz_type")
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = form_data_bak
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+
+class MigrateTreeMap(MigrateViz):
Review Comment:
Won't we have other viz types to be migrated? If yes, are you going to add all the viz types to this file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r915041354
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
+ target_viz_type: str
+
+ def __init__(self, form_data: str) -> None:
+ self.data = json.loads(form_data)
+
+ def _pre_action(self) -> None:
+ """some actions before migrate"""
+
+ def _migrate(self) -> None:
+ if self.data.get("viz_type") != self.source_viz_type:
+ return
+
+ if "viz_type" in self.data:
+ self.data["viz_type"] = self.target_viz_type
+
+ rv_data = {}
+ for (key, value) in self.data.items():
Review Comment:
Nit. You don't need the `()` around `(key, value)`.
##########
superset/migrations/versions/2022-06-30_22-04_c747c78868b6_migrating_legacy_treemap.py:
##########
@@ -0,0 +1,104 @@
+# 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.
+"""Migrating legacy TreeMap
+
+Revision ID: c747c78868b6
+Revises: e786798587de
+Create Date: 2022-06-30 22:04:17.686635
+
+"""
+
+# revision identifiers, used by Alembic.
+
+revision = "c747c78868b6"
+down_revision = "7fb8bca906d2"
+
+from alembic import op
+from sqlalchemy import and_, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils.migrate_viz import get_migrate_class, MigrateVizEnum
+
+treemap_processor = get_migrate_class[MigrateVizEnum.treemap]
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ slice_name = Column(String(250))
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def upgrade():
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices = session.query(Slice).filter(
+ Slice.viz_type == treemap_processor.source_viz_type
+ )
+ total = slices.count()
+ idx = 0
+ for slc in slices.yield_per(100):
+ try:
+ idx += 1
+ print(f"Upgrading ({idx}/{total}): {slc.slice_name}#{slc.id}")
+ new_viz = treemap_processor.upgrade(slc)
+ session.merge(new_viz)
+ session.commit()
Review Comment:
We probably don't need to commit after every slice. Every 1,000 or so is likely preferable.
##########
tests/unit_tests/utils/viz_migration/treemap_migration_test.py:
##########
@@ -0,0 +1,93 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Thanks for modularizing and adding unit tests for migrations.
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
Review Comment:
```suggestion
```
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
Review Comment:
```suggestion
```
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20346:
URL: https://github.com/apache/superset/pull/20346#issuecomment-1176984412
@john-bodley thanks for the reviewing, I have addressed 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie merged pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie merged PR #20346:
URL: https://github.com/apache/superset/pull/20346
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20346:
URL: https://github.com/apache/superset/pull/20346#issuecomment-1152168655
# [Codecov](https://codecov.io/gh/apache/superset/pull/20346?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20346](https://codecov.io/gh/apache/superset/pull/20346?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5766763) into [master](https://codecov.io/gh/apache/superset/commit/354a89950c4d001da3e107f60788cea873bd6bf6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (354a899) will **decrease** coverage by `12.13%`.
> The diff coverage is `6.57%`.
> :exclamation: Current head 5766763 differs from pull request most recent head 8303fe3. Consider uploading reports for the commit 8303fe3 to get more accurate results
```diff
@@ Coverage Diff @@
## master #20346 +/- ##
===========================================
- Coverage 66.64% 54.51% -12.14%
===========================================
Files 1738 1739 +1
Lines 65053 65127 +74
Branches 6884 6885 +1
===========================================
- Hits 43356 35501 -7855
- Misses 19950 27879 +7929
Partials 1747 1747
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.46% <5.63%> (-0.11%)` | :arrow_down: |
| python | `57.61% <5.63%> (-25.18%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.05% <5.63%> (-0.11%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20346?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/utils/migrate\_viz.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvbWlncmF0ZV92aXoucHk=) | `0.00% <0.00%> (ø)` | |
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `58.82% <20.00%> (-1.79%)` | :arrow_down: |
| [superset/db\_engine\_specs/databricks.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RhdGFicmlja3MucHk=) | `92.30% <100.00%> (+1.39%)` | :arrow_up: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/key\_value/commands/upsert.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3Vwc2VydC5weQ==) | `0.00% <0.00%> (-89.59%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-89.37%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| ... and [276 more](https://codecov.io/gh/apache/superset/pull/20346/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20346?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20346?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [354a899...8303fe3](https://codecov.io/gh/apache/superset/pull/20346?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r910555694
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
+ target_viz_type: str
+
+ def __init__(self, form_data: str) -> None:
+ self.data = json.loads(form_data)
+
+ def _pre_action(self) -> None:
+ """some actions before migrate"""
+
+ def _migrate(self) -> None:
+ if self.data.get("viz_type") != self.source_viz_type:
+ return
+
+ if "viz_type" in self.data:
+ self.data["viz_type"] = self.target_viz_type
+
+ rv_data = {}
+ for (key, value) in self.data.items():
+ if key in self.mapping_keys and self.mapping_keys[key] in rv_data:
+ raise ValueError("Duplicate key in target viz")
+
+ if key in self.mapping_keys:
+ rv_data[self.mapping_keys[key]] = value
Review Comment:
Exception handling is easier to maintain if it can be put together.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20346:
URL: https://github.com/apache/superset/pull/20346#issuecomment-1176259703
> > @zhaoyongjie @rusackas Shouldn't we disable TreeMap v1 after the migration? By that, I mean no more TreeMap v1 on Add Slice and Chart Type filter in the charts list. Right now, the user can still create the old version which kind of defeats the whole purpose of the migration.
>
> I intend to do a separated PR that removes the Legacy TreeMap after merged migration PR. the follow-up PR will 1) remove Legacy TreeMap from plugins, 2) remove legacy TreeMap from MainPresent.js.
May I suggest implementing this in the same PR to avoid the risk of someone executing the script and creating an old TreeMap before the follow-up PR is ready?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r899111768
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
+ target_viz_type: str
+
+ def __init__(self, form_data: str) -> None:
+ self.data = json.loads(form_data)
+
+ def _pre_action(self) -> None:
+ """some actions before migrate"""
+
+ def _migrate(self) -> None:
+ if self.data.get("viz_type") != self.source_viz_type:
+ return
+
+ if "viz_type" in self.data:
+ self.data["viz_type"] = self.target_viz_type
+
+ rv_data = {}
+ for (key, value) in self.data.items():
+ if key in self.mapping_keys and self.mapping_keys[key] in rv_data:
+ raise ValueError("Duplicate key in target viz")
+
+ if key in self.mapping_keys:
+ rv_data[self.mapping_keys[key]] = value
+
+ if key in self.remove_keys:
+ continue
+
+ rv_data[key] = value
+
+ self.data = rv_data
+
+ def _post_action(self) -> None:
+ """some actions after migrate"""
+
+ @classmethod
+ def upgrade(cls, slc: Slice) -> Slice:
+ clz = cls(slc.params)
+ slc.viz_type = cls.target_viz_type
+ form_data_bak = clz.data.copy()
+
+ clz._pre_action()
+ clz._migrate()
+ clz._post_action()
+
+ # only backup params
+ slc.params = json.dumps({**clz.data, "form_data_bak": form_data_bak})
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = clz.data
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+ @classmethod
+ def downgrade(cls, slc: Slice) -> Slice:
+ form_data = json.loads(slc.params)
+ if "form_data_bak" in form_data and "viz_type" in form_data.get(
+ "form_data_bak"
+ ):
+ form_data_bak = form_data["form_data_bak"]
+ slc.params = json.dumps(form_data_bak)
+ slc.viz_type = form_data_bak.get("viz_type")
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = form_data_bak
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+
+class MigrateTreeMap(MigrateViz):
Review Comment:
Generally, _scripts_ are waterfalls, while _libary_ should be modular.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r899108283
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,125 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+# pylint: disable=invalid-name
+class MigrateVizEnum(str, Enum):
+ # the Enum member name is viz_type in database
+ treemap = "treemap"
+
+
+class MigrateViz:
+ remove_keys: Set[str] = set()
+
+ mapping_keys: Dict[str, str] = {}
+
+ source_viz_type: str
+
+ target_viz_type: str
+
+ def __init__(self, form_data: str) -> None:
+ self.data = json.loads(form_data)
+
+ def _pre_action(self) -> None:
+ """some actions before migrate"""
+
+ def _migrate(self) -> None:
+ if self.data.get("viz_type") != self.source_viz_type:
+ return
+
+ if "viz_type" in self.data:
+ self.data["viz_type"] = self.target_viz_type
+
+ rv_data = {}
+ for (key, value) in self.data.items():
+ if key in self.mapping_keys and self.mapping_keys[key] in rv_data:
+ raise ValueError("Duplicate key in target viz")
+
+ if key in self.mapping_keys:
+ rv_data[self.mapping_keys[key]] = value
+
+ if key in self.remove_keys:
+ continue
+
+ rv_data[key] = value
+
+ self.data = rv_data
+
+ def _post_action(self) -> None:
+ """some actions after migrate"""
+
+ @classmethod
+ def upgrade(cls, slc: Slice) -> Slice:
+ clz = cls(slc.params)
+ slc.viz_type = cls.target_viz_type
+ form_data_bak = clz.data.copy()
+
+ clz._pre_action()
+ clz._migrate()
+ clz._post_action()
+
+ # only backup params
+ slc.params = json.dumps({**clz.data, "form_data_bak": form_data_bak})
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = clz.data
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+ @classmethod
+ def downgrade(cls, slc: Slice) -> Slice:
+ form_data = json.loads(slc.params)
+ if "form_data_bak" in form_data and "viz_type" in form_data.get(
+ "form_data_bak"
+ ):
+ form_data_bak = form_data["form_data_bak"]
+ slc.params = json.dumps(form_data_bak)
+ slc.viz_type = form_data_bak.get("viz_type")
+
+ query_context = json.loads(slc.query_context or "{}")
+ if "form_data" in query_context:
+ query_context["form_data"] = form_data_bak
+ slc.query_context = json.dumps(query_context)
+ return slc
+
+
+class MigrateTreeMap(MigrateViz):
Review Comment:
yep, all migrations are placed in this script.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r894341974
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,122 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Any, Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+class MirateVizEnum(str, Enum):
Review Comment:
Ah!! nice catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20346:
URL: https://github.com/apache/superset/pull/20346#discussion_r894332918
##########
superset/utils/migrate_viz.py:
##########
@@ -0,0 +1,122 @@
+# 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.
+from __future__ import annotations
+
+import json
+from enum import Enum
+from typing import Any, Dict, Set, Type, TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+
+class MirateVizEnum(str, Enum):
Review Comment:
Typo? MirateVizEnum -> MigrateVizEnum
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie closed pull request #20346: feat: TreeMap migration
Posted by GitBox <gi...@apache.org>.
zhaoyongjie closed pull request #20346: feat: TreeMap migration
URL: https://github.com/apache/superset/pull/20346
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org