adwaitm1301 commented on PR #42639:
URL: https://github.com/apache/superset/pull/42639#issuecomment-5143163477
Thanks for the review, the flagged gap is real and I've fixed it.
After tracing all four `get_timestamp_expr` call sites, the untyped-column
case only occurs in `Query.adhoc_column_to_sqla` in
`superset/models/sql_lab.py`. The other callers already resolve the column type
before reaching the engine spec:
- `TableColumn.get_timestamp_expression`
(`superset/connectors/sqla/models.py`) builds the column with the type from
`get_column_spec` (falling back to `DateTime`), so a VARCHAR column marked
temporal arrives as `String`.
- `SqlaTable.adhoc_column_to_sqla` goes through `get_sqla_col`, which
carries the same type, and its ad-hoc (non-metadata) branch only treats a
column as temporal when the type probe says it is.
- `Dataset.get_timestamp_expression` (`superset/models/helpers.py`) does the
same type resolution.
`Query.adhoc_column_to_sqla` was the exception: it built
`literal_column(expression)` with no type even when `is_dttm` came from
metadata, which is exactly why the `isinstance(col.type, (String, Text))` check
missed it.
I went with your first suggested option rather than the `is_temporal`
attribute. The attribute is not set on any SQLAlchemy expression today, so the
check would be dead unless every caller starts setting it, and setting it
broadly on temporal columns would wrap genuinely temporal columns in a no-op
`CAST(... AS TIMESTAMP)` for Postgres. Propagating the resolved SQLAlchemy type
from the result-column metadata puts the same information in the canonical
place, so the existing engine-spec check handles it and no other engine is
affected.
Changes in `9e51247`:
- `superset/models/sql_lab.py`: resolve the SQLAlchemy type from metadata
and pass it to the `literal_column`.
- `tests/unit_tests/models/sql_lab_test.py`: regression test asserting the
Postgres spec now renders `DATE_TRUNC('day', CAST(event_ts AS TIMESTAMP))` for
a VARCHAR result column marked temporal.
--
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]
