fix(mcp): bind Big Number chart to a temporal column for dashboard time filters#41895
Conversation
…me filters Big Number charts created via the MCP server did not respond to dashboard time-range filters. map_big_number_config() never added a TEMPORAL_RANGE adhoc filter (nor set granularity_sqla) for the default big_number_total viz_type, so the dashboard's Time Range filter had no column to bind to. Bind an explicit temporal_column, or fall back to the dataset's main_dttm_col, and ensure a TEMPORAL_RANGE adhoc filter is present. This mirrors the existing _ensure_temporal_adhoc_filter pattern already used by map_xy_config for the same purpose, and matches the Explore UI's BigNumberTotal control panel, which always exposes an adhoc_filters control even without a dedicated time-column control.
dosubot
Bot
added
change:backend
There was a problem hiding this comment.
Pull request overview
This PR fixes Big Number (specifically big_number_total, i.e. without a trendline) charts created via the MCP service not responding to dashboard-native time range filters by ensuring the generated form_data contains a temporal binding via a TEMPORAL_RANGE adhoc filter (using an explicit temporal_column or falling back to the dataset’s main_dttm_col).
Changes:
- Add a best-effort dataset lookup helper to retrieve
main_dttm_col, and always ensure aTEMPORAL_RANGEadhoc filter is present for Big Number charts when a temporal column can be determined. - Wire
dataset_idthrough the Big Number MCP plugin so the mapping layer can fall back to dataset metadata when needed. - Add unit tests covering dataset fallback, no-op when no temporal column exists, explicit temporal column handling, and trendline behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
superset/mcp_service/chart/chart_utils.py |
Adds dataset temporal-column fallback and ensures TEMPORAL_RANGE adhoc filter binding for Big Number charts so dashboard time filters apply. |
superset/mcp_service/chart/plugins/big_number.py |
Passes dataset_id through to the Big Number form-data mapper to enable dataset-based fallback behavior. |
tests/unit_tests/mcp_service/chart/test_big_number_chart.py |
Adds regression/unit tests validating temporal binding behavior for big_number_total and trendline variants. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41895 +/- ##
==========================================
+ Coverage 64.59% 64.77% +0.17%
==========================================
Files 2712 2739 +27
Lines 151091 152919 +1828
Branches 34751 35032 +281
==========================================
+ Hits 97602 99052 +1450
- Misses 51663 51967 +304
- Partials 1826 1900 +74
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:
|
…dataset lookup Address code review findings on the Big Number dashboard time-filter fix: - Guard the new TEMPORAL_RANGE adhoc filter binding with is_column_truly_temporal(), matching the check map_xy_config already applies to its x-axis. Without this, an explicit non-trendline temporal_column that isn't actually a temporal SQL type would get bound into a dashboard time filter, which can produce invalid SQL. - Extract _find_dataset_by_id_or_uuid() and reuse it in is_column_truly_temporal(), _resolve_default_x_axis(), and _find_dataset_main_dttm_col(), which had all duplicated the same numeric-id-vs-uuid dispatch. - Add a dispatch-level regression test through map_config_to_form_data() covering the dataset_id forwarding fix in BigNumberChartPlugin.to_form_data().
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged performance issue is correct. The current implementation of To resolve this, you should fetch the dataset once at the beginning of the function and pass it to the helper functions. Below is the corrected implementation for def map_big_number_config(
config: BigNumberChartConfig, dataset_id: int | str | None = None
) -> Dict[str, Any]:
# ... (existing code)
dataset = _find_dataset_by_id_or_uuid(dataset_id)
main_dttm_col = dataset.main_dttm_col if dataset else None
temporal_column = config.temporal_column or main_dttm_col
if temporal_column and is_column_truly_temporal(temporal_column, dataset_id):
_ensure_temporal_adhoc_filter(form_data, temporal_column)
return form_dataWould you like me to check the remaining comments on this PR and implement fixes for them as well? superset/mcp_service/chart/chart_utils.py |
Address code review comment on apache#41895: map_big_number_config queried DatasetDAO twice for the same dataset_id when temporal_column was omitted (once for the main_dttm_col fallback, again inside is_column_truly_temporal). Extract the fallback + guard into _resolve_big_number_temporal_column(), which fetches the dataset once and passes it into is_column_truly_temporal for reuse.
Code Review Agent Run #b98819Actionable 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 |
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
Griffen + Codex Dual Review
Verdict: LGTM with nits — Two-way door, no blocking findings from either reviewer.
Nice fix — this closes a real customer-reported gap (SC-112870) between Explore-UI-created and MCP-created Big Number charts, and does it by extending the existing _ensure_temporal_adhoc_filter pattern already used by map_xy_config, rather than inventing a new mechanism. The 3-commit history shows good responsiveness to review feedback: correctness guard first (is_column_truly_temporal, avoiding invalid SQL from a non-temporal column), then efficiency (removing a redundant DAO lookup) — the right order.
Door classification
Two-way door. Purely additive change to MCP chart form_data generation. No schema/migration, no public API contract break (dataset_id param is backward-compatible, defaults to None), isolated blast radius (only new/re-mapped MCP-created Big Number charts). Simple rollback via revert.
Findings (non-blocking)
MEDIUM — untested guard-suppression branch. The is_column_truly_temporal guard added in the 2nd commit specifically to stop a non-temporal explicit temporal_column from being bound into a TEMPORAL_RANGE filter has no test exercising the "column resolves to non-temporal → suppress filter" branch for big_number_total. test_total_binds_dataset_main_dttm_col_for_dashboard_filters uses mock_dataset.columns = [], which hits the "column not found → default True" branch, not "found a non-temporal column type → False". map_xy_config has a direct analog for this exact scenario (test_non_temporal_x_axis_no_temporal_filter) — worth mirroring here so the guard this PR added stays covered against regressions.
LOW — duplicate id/uuid dataset lookup helper (Griffen + Codex, found independently). The new _find_dataset_by_id_or_uuid() reimplements dispatch-on-isdigit() logic that DatasetDAO's inherited BaseDAO.find_by_id_or_uuid() already provides in a single query. Not a correctness issue today, but it's a second implementation of the same lookup that can silently drift from the DAO's (e.g. around base_filter/visibility semantics). Consider consolidating onto find_by_id_or_uuid() and dropping the new private helper.
LOW — trendline path now does two temporal-type lookups per chart create (Codex). For trendline charts, _resolve_big_number_temporal_column() calls is_column_truly_temporal() (its own DAO lookup, since dataset isn't pre-fetched when temporal_column is explicit), and the pre-existing post_map_validate() calls it again independently. Not a correctness problem, but it's the same class of redundancy commit 3 just fixed for the non-trendline path, newly introduced here for trendline. Worth a quick follow-up to share the result between the two call sites.
LOW — same double-lookup pattern survives, untouched, in map_xy_config (Griffen). _resolve_default_x_axis() fetches the dataset when x-axis is omitted, then is_column_truly_temporal(config.x.name, dataset_id) a few lines later doesn't reuse it — the exact inefficiency just fixed here for Big Number. Non-blocking, but since this PR's own docstring says it "mirrors" map_xy_config's pattern, it'd be a quick consistency win to apply the same dataset= reuse there too.
None of the above block merge — they're follow-up-worthy, not regressions in this diff's core fix. The trendline path is also already protected from invalid SQL by the independent (pre-existing) post_map_validate() check regardless of the redundant-lookup nit above.
…r Big Number - _find_dataset_by_id_or_uuid now delegates to DatasetDAO.find_by_id_or_uuid (already used by the dataset API) instead of reimplementing the id/uuid dispatch. - map_xy_config reuses the dataset fetched while resolving a default x-axis instead of re-querying DatasetDAO in is_column_truly_temporal, mirroring the dedup already applied to map_big_number_config. - Added a regression test for Big Number Total suppressing the TEMPORAL_RANGE filter when is_column_truly_temporal resolves a real (non-mocked) non- temporal column, plus a dataset-reuse regression test for map_xy_config's default x-axis path. Addresses review feedback from @fitzee on PR apache#41895.
|
Thanks for the thorough dual review, @fitzee — really appreciate the "door classification" framing and the concrete repro steps. Pushed
Not fixed here — LOW (trendline path: All 2599 mcp_service unit tests pass, |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #dfe0ffActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here — reviewed da4f9dc.
BLOCKER
superset/mcp_service/chart/chart_utils.py:281andtests/unit_tests/mcp_service/chart/test_chart_utils.py:964: could we moveDatasetDAOandColumnSpecto module scope? IfDatasetDAOgenuinely needs to remain local to break a cycle, please document the concrete circular dependency.
MEDIUM
superset/mcp_service/chart/schemas.py:1405: could we update the publictemporal_columndescription? It currently says the field is only for the trendline x-axis, while this change also uses it to select dashboard time-filter binding forbig_number_total.superset/mcp_service/chart/chart_utils.py:269: could we use the concreteSqlaTable | Nonetype for the new dataset helper, prefetched parameter, and tuple return instead ofAny? A type-checking-only import would avoid runtime coupling if needed.
NIT
tests/unit_tests/mcp_service/chart/test_big_number_chart.py:385: theall(...call_args_list)assertion looks redundant with the strongerassert_called_once_with("42")immediately below it.
PRAISE
- Reusing the existing temporal-filter helper keeps Big Number and XY behavior aligned, and the fallback, explicit-column, non-temporal, trendline, and dispatcher cases are well covered.
…import, temporal_column docstring
|
Thanks for the thorough review, @richardfogaca — addressed in 21233a5: BLOCKER — module scope vs. local
MEDIUM — MEDIUM — NIT —
|
|
Bito Automatic Review Skipped – PR Already Merged |
sadpandajoe
added
the
review:checkpoint
SUMMARY
Big Number charts created via the Superset MCP server did not respond to dashboard time-range filters.
map_big_number_config()insuperset/mcp_service/chart/chart_utils.pynever added aTEMPORAL_RANGEadhoc filter (nor setgranularity_sqla) for the defaultbig_number_totalviz_type (Big Number without a trendline), so the dashboard's native Time Range filter had no column to bind to.This mirrors the Explore UI's
BigNumberTotalcontrol panel, which always exposes anadhoc_filterscontrol (defaulted to aTEMPORAL_RANGEfilter on the dataset's main temporal column) even though there's no dedicated time-column control for the total variant.BEFORE
Big Number charts (without a trendline) created via the MCP server ignored dashboard-level time-range filters — no adhoc filter existed to bind to.
AFTER
map_big_number_config()now binds aTEMPORAL_RANGEadhoc filter using an explicittemporal_column, or falls back to the dataset'smain_dttm_colwhen one isn't specified. This reuses the existing_ensure_temporal_adhoc_filterhelper already used bymap_xy_configfor the same purpose, so both chart types now follow the same pattern for making dashboard time filters effective.TESTING INSTRUCTIONS
tests/unit_tests/mcp_service/chart/test_big_number_chart.pycovering: dataset fallback binding, no-op when dataset has no temporal column, explicittemporal_columnwithout a trendline, and trendline charts getting bothgranularity_sqlaand the adhoc filter.pytest tests/unit_tests/mcp_service/chart/test_big_number_chart.py tests/unit_tests/mcp_service/chart/test_chart_utils.py -q→ 204 passedpytest tests/unit_tests/mcp_service/ -q→ 2596 passed (no regressions)ruff format --check/ruff checkclean on all changed filesADDITIONAL INFORMATION