[GitHub] [superset] john-bodley commented on a diff in pull request #22957: chore(datasets): Refactor DatasetDAO with bulk operations

["john-bodley (via GitHub)" <gi...@apache.org>]

john-bodley commented on code in PR #22957:
URL: https://github.com/apache/superset/pull/22957#discussion_r1094170324


##########
superset/datasets/dao.py:
##########
@@ -241,26 +252,36 @@ def update_metrics(
         then we delete.
         """
 
-        metric_by_id = {metric.id: metric for metric in model.metrics}
-        seen = set()
-
-        for properties in property_metrics:
-            if "id" in properties:
-                seen.add(properties["id"])
+        metrics_by_id = {metric.id: metric for metric in model.metrics}

Review Comment:
   metrics (plural) and not metric (singular).



##########
superset/datasets/dao.py:
##########
@@ -241,26 +252,36 @@ def update_metrics(
         then we delete.
         """
 
-        metric_by_id = {metric.id: metric for metric in model.metrics}
-        seen = set()
-
-        for properties in property_metrics:
-            if "id" in properties:
-                seen.add(properties["id"])
+        metrics_by_id = {metric.id: metric for metric in model.metrics}
+
+        property_metrics_by_id = {
+            properties["id"]: properties
+            for properties in property_metrics
+            if "id" in properties
+        }
+
+        db.session.bulk_insert_mappings(
+            SqlMetric,
+            [
+                {**properties, "table_id": model.id}
+                for properties in property_metrics
+                if not "id" in properties
+            ],
+        )
 
-                DatasetDAO.update_metric(
-                    metric_by_id[properties["id"]],
-                    properties,
-                    commit=False,
-                )
-            else:
-                DatasetDAO.create_metric(
-                    {**properties, "table_id": model.id},
-                    commit=False,
-                )
+        db.session.bulk_update_mappings(
+            SqlMetric,
+            [
+                {**metrics_by_id[properties["id"]].__dict__, **properties}
+                for properties in property_metrics_by_id.values()
+            ],
+        )
 
-        for id_ in {obj.id for obj in model.metrics} - seen:
-            DatasetDAO.delete_column(metric_by_id[id_], commit=False)
+        db.session.query(SqlMetric).filter(
+            SqlMetric.id.in_(
+                {metric.id for metric in model.metrics} - property_metrics_by_id.keys()
+            )
+        ).delete(synchronize_session="fetch")

Review Comment:
   Using the same `synchronize_session` as the `bulk_delete` method for deleting multiple datasets.



-- 
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