-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
refactor: Improve performance regression introduced in #20473 #20810
refactor: Improve performance regression introduced in #20473 #20810
Conversation
|
||
for properties in property_columns: | ||
if "id" in properties: | ||
columns.append( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
9bab0fe
to
ef3dda9
Compare
ef3dda9
to
f1916a0
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
f1916a0
to
24690ef
Compare
@@ -154,10 +154,10 @@ def update( | |||
""" | |||
|
|||
if "columns" in properties: | |||
properties["columns"] = cls.update_columns(model, properties["columns"]) | |||
cls.update_columns(model, properties.pop("columns"), commit=commit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pop
instead of direct access?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
SUMMARY
This PR resolves a performance regression introduced in #20473. Previously only the new columns/metrics where returned via the
update_columns
andupdate_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 theupdate
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
andupdate_metrics
were returning a subset of entities.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION