justinpark opened a new pull request, #42288: URL: https://github.com/apache/superset/pull/42288
### SUMMARY Problem: Requesting time_grain=P1W (weekly) on a Superset MCP chart was silently ignored — time_grain_sqla got overwritten to None just because the ds column is typed VARCHAR. The LLM explained this away as "a fundamental limitation of the Superset MCP API," but that explanation was wrong — it was actually a bug in the MCP server's own code. Root cause: is_column_truly_temporal() in superset/mcp_service/chart/chart_utils.py completely ignored the dataset column's is_dttm flag and classified temporality purely from the raw SQL GenericDataType — so any VARCHAR column was unconditionally treated as non-temporal. This contradicts Superset core's own TableColumn.is_temporal property (superset/connectors/sqla/models.py:942-952), which explicitly trusts is_dttm over the raw SQL type whenever it's set — the standard, supported mechanism for treating string-typed date columns (e.g. Hive/Presto/Trino VARCHAR ds partition columns) as temporal. Fix: Extracted a _is_dataset_column_temporal() helper that mirrors Superset core's precedence rules: - If the raw SQL type is already TEMPORAL, always temporal. - If is_dttm=True, trust it regardless of raw SQL type (fixes the VARCHAR ds case). - Exception: a NUMERIC column flagged is_dttm=True with no python_date_format is still treated as non-temporal — preserving the original guard against columns Superset's naming heuristics may have mis-flagged (e.g. an integer year/month column), where applying DATE_TRUNC would fail at query time. ### TESTING INSTRUCTIONS Added 5 new cases to test_chart_utils.py covering VARCHAR+is_dttm, VARCHAR without is_dttm, and NUMERIC+is_dttm with/without python_date_format. All 190 unit tests pass, along with ruff format, ruff check, and pre-commit mypy. ### ADDITIONAL INFORMATION <!--- Check any relevant boxes with "x" --> <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue --> - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
