fix(mcp): trust dataset is_dttm flag when applying time_grain to VARCHAR temporal columns#42288
fix(mcp): trust dataset is_dttm flag when applying time_grain to VARCHAR temporal columns#42288justinpark wants to merge 2 commits into
Conversation
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #60212c
Actionable Suggestions - 1
-
tests/unit_tests/mcp_service/chart/test_chart_utils.py - 1
- Wrong mock method in test · Line 1597-1597
Review Details
-
Files reviewed - 2 · Commit Range:
8027b25..8027b25- superset/mcp_service/chart/chart_utils.py
- tests/unit_tests/mcp_service/chart/test_chart_utils.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| is_dttm=True, | ||
| python_date_format="%Y-%m-%d", | ||
| ) | ||
| mock_dao.find_by_id.return_value = mock_dataset |
There was a problem hiding this comment.
The test mocks mock_dao.find_by_id but is_column_truly_temporal resolves its dataset via DatasetDAO.find_by_id_or_uuid (called by _find_dataset_by_id_or_uuid). The mocked return value is never used, so the test exercises the real DAO path instead. All other tests in this class correctly mock find_by_id_or_uuid.
Code Review Run #60212c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42288 +/- ##
==========================================
- Coverage 65.15% 65.15% -0.01%
==========================================
Files 2789 2789
Lines 157546 157546
Branches 35864 35865 +1
==========================================
- Hits 102651 102645 -6
- Misses 52927 52933 +6
Partials 1968 1968
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DatasetDAO.find_by_id_or_uuid is what is_column_truly_temporal actually calls, but 5 tests mocked find_by_id instead. The unconfigured mock returned an auto-mock dataset whose empty .columns caused the function to fall through to its column-not-found default of True, masking two tests that expected False.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #4f8bbcActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed at HEAD 5667164 (566716483bd64337f7adf51ab6ee0efd1709218d).
The fix is correct and the tests are non-vacuous. Traced _is_dataset_column_temporal() (chart_utils.py:292-330) side-by-side against TableColumn.is_temporal (superset/connectors/sqla/models.py:1037-1046):
- Core's actual property is simpler than a strict "raw-type → is_dttm → NUMERIC-guard" chain — it's just
return self.is_dttm if self.is_dttm is not None else type_generic == TEMPORAL. Sinceis_dttmdefaults toFalse(neverNone) for persisted columns, core in practice always trustsis_dttmunconditionally, with nopython_date_formatrequirement. - This PR's helper adds a stricter NUMERIC+
is_dttm+no-python_date_formatguard that core's property doesn't have. I checked whether that's an over-restriction by tracingget_timestamp_expr(db_engine_specs/base.py:1070) — without apdf,time_grainon a non-temporal SQL type produces an unsubstitutedDATE_TRUNC-style expression (broken SQL). So the extra guard here is catching a real query-execution failure core's bare property doesn't protect against — it's a justified, safer addition, not a functional regression. - The raw-TEMPORAL-wins-first branch (chart_utils.py:311-312) would override an explicit
is_dttm=Falseset on a native TIMESTAMP/DATE column, which differs from core's precedence — but this is pre-existing behavior carried over unchanged from the old implementation, not something this diff introduces.
Net: the "mirrors TableColumn.is_temporal" framing in the docstring/PR body is a bit stronger than reality (it's closer to core's behavior plus one extra safety guard), but functionally it's correct and strictly safer than the bug being fixed. Nit-level only, not blocking.
Traced the removed 35 lines against the new helper — the "no column_spec" and "no col_type" fallbacks are fully subsumed (still covered by test_falls_back_to_is_dttm_when_no_column_spec / test_falls_back_to_is_dttm_when_no_type), nothing dropped.
Tests: the 5 new cases each assert the actual boolean result (not vacuous), and cover all four precedence branches (VARCHAR+is_dttm+/-format, NUMERIC+is_dttm+/-format). Pre-existing raw-TEMPORAL coverage (test_returns_true_for_temporal_column, test_returns_true_for_date_column) still applies unchanged.
Nice fix for the Hive/Presto/Trino VARCHAR partition-column case — approving.
2 participants
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:
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