refactor: Minor optimization for updating DatasetDAO columns and metrics #20473

john-bodley · 2022-06-22T23:04:24Z

SUMMARY

This PR makes the minimal updates as possible to the DatasetDAO update logic to improve performance as the request was timing out when we (Airbnb) were trying to update a very very large dataset. If this is not suffice, the next and more aggressive update would be to add bulk updating.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

john-bodley · 2022-06-22T23:05:03Z

superset/datasets/dao.py

-            properties["columns"] = cls.update_columns(
-                model, properties.get("columns", []), commit=commit
-            )
+            properties["columns"] = cls.update_columns(model, properties["columns"])


get(...) is not needed here per the check on line #165.

john-bodley · 2022-06-22T23:05:58Z

superset/datasets/dao.py

        for column in property_columns:
            column_id = column.get("id")

            if column_id:
-                column_obj = db.session.query(TableColumn).get(column_id)


Leverage the columns defined by the model rather than re-querying for each column individually.

john-bodley · 2022-06-22T23:08:45Z

superset/datasets/dao.py

-                column_obj = DatasetDAO.update_column(column_obj, column, commit=commit)
+                column_obj = DatasetDAO.update_column(
+                    column_by_id.get(column_id),
+                    commit=False,


No need to commit when updating the column. It can all be committed when the dataset is updated.

john-bodley · 2022-06-22T23:10:11Z

superset/datasets/dao.py

            else:
-                column_obj = DatasetDAO.create_column(column, commit=commit)
+                column_obj = DatasetDAO.create_column(column, commit=False)


No need to commit when creating the column. Also the TableColumn object has no table_id at this time and thus the record needs to be updated when the entire dataset is committed. Also autoflush=True by default so the new column should have a primary key associated with it.

john-bodley · 2022-06-22T23:14:42Z

superset/datasets/dao.py

-                column_obj = db.session.query(TableColumn).get(column_id)
-                column_obj = DatasetDAO.update_column(column_obj, column, commit=commit)
+                column_obj = DatasetDAO.update_column(
+                    column_by_id.get(column_id),


Same behavior as before, i.e., the input column_obj is None if for some reason it doesn't exist—which shouldn't happen.

codecov · 2022-06-22T23:18:26Z

Codecov Report

Merging #20473 (a242b25) into master (a169b60) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #20473      +/-   ##
==========================================
- Coverage   66.75%   66.75%   -0.01%     
==========================================
  Files        1740     1740              
  Lines       65149    65143       -6     
  Branches     6898     6898              
==========================================
- Hits        43492    43486       -6     
  Misses      19908    19908              
  Partials     1749     1749

Flag	Coverage Δ
hive	`53.72% <9.09%> (+0.01%)`	⬆️
mysql	`82.33% <100.00%> (-0.01%)`	⬇️
postgres	`82.40% <100.00%> (-0.01%)`	⬇️
presto	`53.58% <9.09%> (+0.01%)`	⬆️
python	`82.83% <100.00%> (-0.01%)`	⬇️
sqlite	`82.18% <100.00%> (-0.01%)`	⬇️
unit	`50.56% <9.09%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/datasets/dao.py	`93.57% <100.00%> (-0.27%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a169b60...a242b25. Read the comment docs.

john-bodley · 2022-06-23T02:45:10Z

superset/datasets/dao.py

+                # Note for new columns the primary key is undefined sans a commit/flush.
+                columns.append(DatasetDAO.create_column(properties, commit=False))
+
+        for id_ in {obj.id for obj in model.columns} - {obj.id for obj in columns}:


Optimized looping.

john-bodley · 2022-06-23T02:45:48Z

superset/datasets/dao.py

+                    DatasetDAO.update_column(
+                        column_by_id[properties["id"]],
+                        properties,
+                        commit=False,


Commit once for the dataset.

john-bodley · 2022-06-23T02:46:37Z

superset/datasets/dao.py

+        model: TableColumn,
+        properties: Dict[str, Any],
+        commit: bool = True,
+    ) -> TableColumn:


The return is not Optional[...] per the superset/dao/base.py methods.

john-bodley · 2022-06-23T02:47:03Z

superset/datasets/dao.py

@@ -287,24 +296,27 @@ def find_dataset_metric(
        return db.session.query(SqlMetric).get(metric_id)

    @classmethod
-    def delete_metric(
-        cls, model: SqlMetric, commit: bool = True
-    ) -> Optional[TableColumn]:


Should have been SqlMetric and not TableColumn.

Why does the command still return the metric when it is deleted?

yeah, prob not needed

ktmud · 2022-06-23T17:27:41Z

superset/datasets/dao.py

+                metrics.append(DatasetDAO.create_metric(properties, commit=False))
+
+        for id_ in {obj.id for obj in model.metrics} - {obj.id for obj in metrics}:
+            DatasetDAO.delete_column(metric_by_id[id_], commit=False)


Wouldn't SQLA automatically delete orphaned metrics once they are not associated with a dataset?

ktmud

I think it's a good idea to always skip auto-commit.

…ics (apache#20473) Co-authored-by: John Bodley <john.bodley@airbnb.com>

* refactor: Improve performance regression introduced in #20473 * Update dao.py

pull-request-size bot added the size/M label Jun 22, 2022

john-bodley marked this pull request as ready for review June 22, 2022 23:12

john-bodley requested review from dpgaspar, betodealmeida, etr2460 and ktmud June 22, 2022 23:12

john-bodley commented Jun 22, 2022

View reviewed changes

john-bodley force-pushed the john-bodley-dataset-dao-update-commit branch from b74e5e6 to cc1533a Compare June 22, 2022 23:40

refactor: Minor optimization for updating DatasetDAO columns and metrics

a242b25

john-bodley force-pushed the john-bodley-dataset-dao-update-commit branch from cc1533a to a242b25 Compare June 23, 2022 02:44

pull-request-size bot added size/L and removed size/M labels Jun 23, 2022

john-bodley commented Jun 23, 2022

View reviewed changes

ktmud reviewed Jun 23, 2022

View reviewed changes

ktmud approved these changes Jun 23, 2022

View reviewed changes

john-bodley merged commit f29cde2 into apache:master Jun 25, 2022

john-bodley deleted the john-bodley-dataset-dao-update-commit branch June 25, 2022 21:31

akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022

refactor: Minor optimization for updating DatasetDAO columns and metr…

1336d80

…ics (apache#20473) Co-authored-by: John Bodley <john.bodley@airbnb.com>

john-bodley mentioned this pull request Jul 21, 2022

refactor: Improve performance regression introduced in #20473 #20810

Merged

9 tasks

john-bodley added a commit to john-bodley/superset that referenced this pull request Jul 21, 2022

refactor: Improve performance regression introduced in apache#20473

24690ef

john-bodley added a commit that referenced this pull request Jul 27, 2022

refactor: Improve performance regression introduced in #20473 (#20810)

d327437

* refactor: Improve performance regression introduced in #20473 * Update dao.py

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Minor optimization for updating DatasetDAO columns and metrics #20473

refactor: Minor optimization for updating DatasetDAO columns and metrics #20473

john-bodley commented Jun 22, 2022 •

edited

Loading

john-bodley Jun 22, 2022

john-bodley Jun 22, 2022 •

edited

Loading

john-bodley Jun 22, 2022

john-bodley Jun 22, 2022

john-bodley Jun 22, 2022

codecov bot commented Jun 22, 2022 •

edited

Loading

john-bodley Jun 23, 2022

john-bodley Jun 23, 2022

john-bodley Jun 23, 2022

john-bodley Jun 23, 2022

ktmud Jun 23, 2022

dpgaspar Jun 23, 2022

ktmud Jun 23, 2022

ktmud left a comment

refactor: Minor optimization for updating DatasetDAO columns and metrics #20473

refactor: Minor optimization for updating DatasetDAO columns and metrics #20473

Conversation

john-bodley commented Jun 22, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

john-bodley Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

john-bodley commented Jun 22, 2022 •

edited

Loading

john-bodley Jun 22, 2022 •

edited

Loading

codecov bot commented Jun 22, 2022 •

edited

Loading