refactor: Improve performance regression introduced in #20473

[john-bodley]

SUMMARY

This PR resolves a performance regression introduced in #20473. Previously only the new columns/metrics where returned via the update_columns and update_metrics methods respectively—which were defined as properties of the dataset and then bound to the table via the DAO update. I also re-introduced the commit to both methods as one likely would expect these methods to commit rather than waiting for the update commit. Note the previous optimization exists, i.e., the commit occurs at the end rather than for each create, update, or delete.

In #20473 both new and existing columns were returned which added unnecessary overhead. Furthermore I'm not sure how complex a SQLAlchemy ORM model update with relationships is, i.e., where it tries to re-update all the columns and metrics, but given we already have dedicated methods to "update" columns/metrics—actually create, update, and delete—it seems simpler/clearer to bind new columns/metrics to the parent table on create and exclude all columns/metrics when updating the dataset model, i.e., previously it wasn't overly clear from the code—without digging—why the update_columns and update_metrics were returning a subset of entities.

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]

This was the root of the problem, i.e., existing columns shouldn't be added to the set of columns which need to be bound to the dataset when updated.

[codecov]

Codecov Report

Merging #20810 (db10275) into master (1101922) will decrease coverage by 11.40%.
The diff coverage is 46.81%.

❗ Current head db10275 differs from pull request most recent head 22498db. Consider uploading reports for the commit 22498db to get more accurate results

@@             Coverage Diff             @@
##           master   #20810       +/-   ##
===========================================
- Coverage   66.20%   54.79%   -11.41%     
===========================================
  Files        1754     1757        +3     
  Lines       66678    66864      +186     
  Branches     7049     7077       +28     
===========================================
- Hits        44143    36640     -7503     
- Misses      20738    28415     +7677     
- Partials     1797     1809       +12     

Flag	Coverage Δ
hive	53.25% <37.98%> (?)
mysql	?
postgres	?
presto	53.11% <38.31%> (?)
python	57.82% <40.58%> (-23.56%)	⬇️
sqlite	?
unit	50.25% <26.29%> (-0.19%)	⬇️

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

Impacted Files	Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts	100.00% <ø> (ø)
...ackages/superset-ui-core/src/query/types/Column.ts	100.00% <ø> (ø)
...ages/superset-ui-core/src/query/types/Dashboard.ts	100.00% <ø> (ø)
...ges/superset-ui-core/src/query/types/Datasource.ts	100.00% <ø> (+33.33%)	⬆️
...s/legacy-plugin-chart-country-map/src/countries.ts	100.00% <ø> (ø)
...s/legacy-plugin-chart-heatmap/src/controlPanel.tsx	57.14% <ø> (ø)
...ns/legacy-preset-chart-deckgl/src/utils/explore.js	0.00% <0.00%> (ø)
...set-chart-nvd3/src/vendor/superset/exploreUtils.js	0.00% <0.00%> (ø)
.../plugin-chart-echarts/src/MixedTimeseries/index.ts	25.00% <ø> (ø)
...ins/plugin-chart-echarts/src/components/Echart.tsx	0.00% <0.00%> (ø)
... and 409 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

[ktmud]

Why pop instead of direct access?

[john-bodley]

@ktmud these properties need to be popped so they're not re-included in the base update method which updates the underlying dataset. The new columns and metrics are already bound to the model in the respective update_columns and update_metrics method.