fix: do not drop calculated column on metadata sync #11731
Conversation
| columns: List["BaseColumn"] = [] | ||
| metrics: List["BaseMetric"] = [] |
There was a problem hiding this comment.
Bycatch: not sure why these weren't typed (did the same for SQLA and Druid)
villebro
force-pushed
the
villebro/fix-sql-colsync
branch
from
49cfd30 to
b48a38d
Compare
| self.columns.extend( | ||
| [col for col in old_columns_by_name.values() if col.expression] | ||
| ) |
There was a problem hiding this comment.
This ensures that calculated columns are not dropped.
|
thank you for the quick fix.🙏.it's 2am in Finland please get some good rest! |
Codecov Report
@@ Coverage Diff @@
## master #11731 +/- ##
==========================================
- Coverage 63.06% 63.02% -0.05%
==========================================
Files 895 897 +2
Lines 43340 43444 +104
Branches 4015 4015
==========================================
+ Hits 27334 27381 +47
- Misses 15828 15885 +57
Partials 178 178
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| self.post_add(item, flash_message=False) | ||
| self.post_add(item, flash_message=False, fetch_metadata=False) |
There was a problem hiding this comment.
I'm not sure why we were syncing metadata on post_update, so I disabled it.
There was a problem hiding this comment.
@dpgaspar do you know why we call post_add every time we post_update the dataset? Previously we were calling fetch_metadata and create_table_permissions every time we updated, but after this change we'll only call create_table_permissions. Is this just to make sure any potentially stale perms get synced?
There was a problem hiding this comment.
To update the permissions if the dataset table_name or database change, yet if that is done it will leave a stale permission name behind
There was a problem hiding this comment.
Ok, then I'll just leave this like this, as I believe we won't be hitting post_edit with the new CRUD.
villebro
force-pushed
the
villebro/fix-sql-colsync
branch
from
b48a38d to
07ee06f
Compare
There was a problem hiding this comment.
this lgtm, although @john-bodley could be a good other reviewer
tests/datasource_tests.py
Outdated
| from typing import Dict | ||
|
|
||
| from superset import db | ||
| from superset.connectors.sqla.models import SqlaTable | ||
| from superset.connectors.sqla.models import SqlaTable, TableColumn |
There was a problem hiding this comment.
why did these get added if the rest of the file didn't change?
There was a problem hiding this comment.
Oh thanks for catching, I implemented the tests here first, but then decided the other test file was the correct place. These imports were left behind from that move.
villebro
force-pushed
the
villebro/fix-sql-colsync
branch
from
07ee06f to
0d501aa
Compare
| old_columns_by_name = {col.column_name: col for col in old_columns} | ||
| old_columns_by_name: Dict[str, TableColumn] = { | ||
| col.column_name: col for col in old_columns | ||
| } |
There was a problem hiding this comment.
Not sure if it's just my IDE, but PyCharm wasn't able to infer the type without the explicit type hint.
villebro
force-pushed
the
villebro/fix-sql-colsync
branch
from
0d501aa to
18dc486
Compare
|
fyi, John said this looks good, so I'm going to go ahead and merge since we do our weekly deploy today and it would be really good to have this change in
|
(cherry picked from commit 7ae8cd0)
mistercrunch
added
🏷️ bot
SUMMARY
PR #10645 introduced a regression that caused calculated columns to be dropped when syncing datasource metadata in the legacy CRUD list. When saving the dataset in the legacy CRUD view, metadata was also automatically synced, causing loss of calculated columns. Previously the metadata refresh was triggered every time the dataset was saved in the legacy CRUD view, but the effect of this was less noticeable as the previous metadata refresh only added new columns, updated existing types and left non-present physical columns in the dataset metadata.
This changes behavior in the following way:
TEST PLAN
Local testing + CI + new tests
ADDITIONAL INFORMATION