fix: overwrite update override columns on PUT /dataset #20862
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20862 +/- ##
=======================================
Coverage 66.30% 66.30%
=======================================
Files 1758 1758
Lines 67065 67073 +8
Branches 7122 7122
=======================================
+ Hits 44468 44476 +8
Misses 20767 20767
Partials 1830 1830
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. |
| assert rv.status_code == 200 | ||
|
|
||
| columns = db.session.query(TableColumn).filter_by(table_id=dataset.id).all() | ||
| assert len(columns) == 3 |
There was a problem hiding this comment.
Can you add an assert that the number of columns was not 3 before the update? (ie, in line ~790) Otherwise we don't know if the update actually did anything.
| properties.pop("columns"), | ||
| commit=commit, | ||
| override_columns=bool(properties.get("override_columns")), | ||
| ) | ||
|
|
||
| if "metrics" in properties: | ||
| cls.update_metrics(model, properties.pop("metrics"), commit=commit) |
There was a problem hiding this comment.
for put /dataset, does override_columns literally mean just the columns or should we be overriding the metrics, too?
There was a problem hiding this comment.
from sqllab, we don't allow users to create metrics so if you look at the update_metrics function we end up just deleting all the rows since properties["metrics"] is always None from that context.
mistercrunch
added
🏷️ bot
SUMMARY
Update logic for overriding columns on update. Initially was getting the following error when trying to make an update with column name that are the same:
With this new pattern, users will be able to overwrite a dataset event if the columns are named the same.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION