gabotorresruiz commented on code in PR #43921:
URL: https://github.com/apache/superset/pull/43921#discussion_r3961015094
##########
superset/connectors/sqla/models.py:
##########
@@ -152,6 +152,20 @@ class MetadataResult:
modified: list[str] = field(default_factory=list)
+def _is_calculated_column(column: TableColumn) -> bool:
+ """Return whether *column* is a user-defined virtual column.
+
+ ``fetch_metadata`` keeps calculated columns that the source table does
+ not list. Engine specs such as Trino also store an ``expression`` on
+ expanded nested ``ROW`` fields (dotted names like ``metadata.uuid``).
+ Those are still physical columns: if the source no longer lists them
+ they must be dropped so chart cache keys invalidate. See #43918.
+ """
+ if not column.expression:
+ return False
+ return "." not in (column.column_name or "")
Review Comment:
This block worries me a bit: the dot heuristic classifies by column name,
but users can legally create a calculated column with a dot in its name
(`DatasetColumnsPutSchema.column_name` only enforces `Length(1, 255)`). I
verified on this branch that an existing calculated column named `revenue.usd`
with expression `revenue * fx_rate` is dropped from `self.columns` by
`fetch_metadata()`, and since the relationship has `cascade="all,
delete-orphan"`, the row is deleted: the user's column is silently destroyed on
every refresh. It survives on master. The Bito bot hinted at this edge too.
Trino is the only engine spec that emits `expression` in metadata, and for
expanded ROW fields it is always the quoted dotted path of the column's own
name (`trino.py::_expand_columns`). Matching that signature instead of a bare
dot keeps the drop behavior for genuinely removed nested fields while sparing
user columns:
```python
def _is_calculated_column(column: TableColumn) -> bool:
if not column.expression:
return False
name = column.column_name or ""
quoted_path = ".".join(f'"{piece}"' for piece in name.split("."))
return column.expression != quoted_path
```
Could you also add a unit test alongside the new ones, e.g.
`test_fetch_metadata_keeps_dotted_calculated_column`, asserting a `revenue.usd`
calculated column survives the refresh? It fails on the current head. Happy to
dig in if the Trino-signature approach misses a case I am not seeing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
