Repository navigation
fix(mcp): honor extra_form_data time_grain_sqla on Generic Chart Axes charts - #44435
aminghadersohi merged 4 commits into
Conversation
… charts Charts built on Generic Chart Axes (anything with an `x_axis`, e.g. `echarts_timeseries_bar` and `mixed_timeseries`) keep their temporal axis as a `BASE_AXIS` adhoc column inside the query's `columns`, and that column's own `timeGrain` is what `SqlaTable.adhoc_column_to_sqla` turns into the SQL truncation. It also takes precedence in `ExploreMixin.get_time_grain`, which reads `columns[0]["timeGrain"]` before falling back to `extras`. The MCP merge path wrote a `time_grain_sqla` override only into `extras["time_grain_sqla"]`, which those queries never consult. `get_chart_data` and `get_chart_sql` therefore returned the chart's saved grain — identical row counts and timestamps, and no error — so a caller had no way to tell the override had been dropped. Propagate the override onto the `BASE_AXIS` column as well, mirroring what `apply_dashboard_filter_context` already does for the dashboard filter path.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44435 +/- ##
==========================================
+ Coverage 80.65% 80.70% +0.04%
==========================================
Files 2942 2944 +2
Lines 175692 176040 +348
Branches 40788 40837 +49
==========================================
+ Hits 141705 142066 +361
+ Misses 31325 31311 -14
- Partials 2662 2663 +1
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:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #aa841aActionable 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
LGTM. Good catch that the BASE_AXIS column, not extras, is what drives the truncation.
I ran it rather than just reading it. At the merge base (43fee87) 7 of the 10 new tests fail, and execute_chart_data with extra_form_data={"time_grain_sqla": "P1M"} on an echarts_timeseries_bar returns 90 daily rows; on this head the same call returns 3 monthly ones. Two things I wanted to be sure of, both fine:
- The grain reaches the cache key.
QueryObject.to_dict()carriescolumns, so the mutatedBASE_AXISis hashed byQueryObject.cache_key(), and I confirmed the override and the baseline come out with different keys. A grain cannot change the generated SQL without changing the key. timeGrainis already inGUEST_OVERRIDABLE_VALUE_KEYS(superset/security/manager.py:690), so mutating the saved column payload does not trip the guest tamper guard on the embedded read path.
Not a blocker, and nothing to change here. The docstring says apply_dashboard_filter_context does the same propagation, and it is close but not identical: that one writes columns[0]["timeGrain"] unconditionally (superset/charts/data/dashboard_filter_context.py:379), while yours targets columnType == "BASE_AXIS", which is what adhoc_column_to_sqla actually gates on (superset/connectors/sqla/models.py:2125). They coincide for anything the frontend builds, since Timeseries, MixedTimeseries, Heatmap and BigNumberWithTrendline all put the x-axis at columns[0]. With the axis at columns[1] I get start of month out of your path and start of day out of the dashboard one, which also writes an inert timeGrain onto a non-axis column. Worth pointing that one at _apply_time_grain_to_base_axis in a follow up so there is a single rule?
|
@gabotorresruiz thanks for running it rather than reading it — the Agreed on the follow-up: Nothing changing on this PR. |
|
Good catch, and yes — but in this PR rather than a follow-up, so there's one rule and not a promise. Doing exactly what you suggested: both paths key on Pushing on top — no force-push, your approval stays as is. Ping me when it lands if you want another look at the widened diff. |
Use a neutral query-payload helper for MCP and dashboard filters so time grain overrides follow BASE_AXIS rather than column position. Cover reordered axes with SQLite SQL assertions and preserve frontend axis-at-zero payloads. Co-Authored-By: GPT-6 Astra <noreply@openai.com>
|
Landed in Axis-at- Your approval is still on the PR and I haven't touched it — the diff is wider than what you signed off on, so take another look if you want one. |
|
Bito Automatic Review Skipped – PR Already Merged |
#44435's test called module.execute_chart_data, the undecorated core that master's #44113 (never backported -- it's the feat that added get_dashboard_data) split out of the @tool-wrapped get_chart_data. 7.0 only has the decorated function, which needs a live FastMCP Client context to run at all. The fix itself is still covered by this file's other four tests, which exercise dashboard_filter_context.py and time_grain_utils.py directly.
SUMMARY
Passing
time_grain_sqlathroughextra_form_datato the MCPget_chart_data/get_chart_sqltools was silently ignored for any chart built on Generic ChartAxes (anything with an
x_axis—echarts_timeseries_bar,mixed_timeseries,etc.). The tools returned the chart's saved grain: identical row counts and
timestamps to a call with no override at all, and no error, so an automated
consumer had no way to detect that the request had been dropped.
Root cause. Generic Chart Axes charts carry their temporal x-axis as a
BASE_AXISadhoc column inside the query'scolumns, and that column's owntimeGrainis what actually becomes the SQL truncation(
SqlaTable.adhoc_column_to_sqlareadscol["timeGrain"];has_timegrainrequires
columnType == "BASE_AXIS"). It also wins inExploreMixin.get_time_grain, which preferscolumns[0]["timeGrain"]overextras. The MCP merge path(
merge_form_data_filters_into_query) wrote the override only intoextras["time_grain_sqla"], which those queries never consult — so the savedgrain kept winning.
Fix (MCP and dashboard filter paths). Both paths propagate the override
onto columns tagged
columnType == "BASE_AXIS", wherever the axis sits incolumns, matching the gate inSqlaTable.adhoc_column_to_sqla. Neither pathadds or overwrites
timeGrainon a non-axis column. Queries without aBASE_AXIScolumn continue to useextras.The shared, public
apply_time_grain_to_base_axishelper lives insuperset/common/utils/time_grain_utils.py. This dependency-light query-payloadutility belongs below both chart data handling and MCP: neither service layer
imports the other, and dashboard handling does not acquire an MCP dependency.
Only the column-selection rule is shared; each caller retains its existing
handling of absent/empty overrides and other filter fields.
Reproduced before changing production code at
78adf0c53b: a daily savedaxis at
columns[1], preceded by a non-axisvalcolumn, overridden toP1Mproduced
DATETIME(ts, 'start of month')through MCP butDATETIME(ts, 'start of day')through the dashboard path. The dashboard pathalso added an inert
timeGrain: P1Mtocolumns[0]; MCP left it untouched.This PR fixes that divergence rather than deferring it to a follow-up.
Frontend-built Generic Chart Axes payloads put the axis at
columns[0](Timeseries, MixedTimeseries, Heatmap, BigNumberWithTrendline). Their behavior is
unchanged: the same value is written to the same column, with all other writes
unchanged. A before/after differential check confirmed byte-identical serialized
query contexts and extra form data in 336 comparisons (168 per entry point),
covering daily/weekly/monthly saved grains, absent/null/empty and
daily/weekly/monthly/yearly overrides, ordinary and adhoc trailing dimensions,
and one/two-query contexts.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — no UI change.
TESTING INSTRUCTIONS
New regression tests in
tests/unit_tests/mcp_service/chart/test_time_grain_generic_axes.pyexecute theresulting query against a real SQLite dataset (90 consecutive daily rows), so
the override is proven by the returned buckets rather than by an echoed field:
P1Dchart overridden withP1Mreturns 3 monthly buckets(
2024-01-01,2024-02-01,2024-03-01) instead of 90 daily ones.P1M, overridden toP1D) returns 90 rows.override preserves the saved axis grain.
At the original merge base (
43fee87), 7 of the 10 MCP regression tests fail;the monthly override still returns daily rows.
Manual check: call the MCP
get_chart_datatool on anecharts_timeseries_barchart saved with a daily grain, passing
extra_form_data={"time_grain_sqla": "P1M"}, and confirm the returned rows aremonthly.
Dashboard regression and validation of the widened scope:
incorrect first-column behavior. It covers axis positions 0 and 1, with and
without an existing grain on the non-axis column, asserts actual SQLite SQL
truncation, and compares the entire serialized query context.
78adf0c53b): 2 failed, 2 passed.Both axis-at-1 cases generated
start of dayinstead ofstart of month.dashboard-filter-context tests, and 49 chart-data API tests).
formatting, Ruff, and Pylint. User-facing grain override behavior is documented
in
docs/admin_docs/configuration/mcp-server.mdx.ADDITIONAL INFORMATION