Repository navigation
fix(mcp): align Jinja context across query preview and compile paths … - #43176
AmoghAtreya wants to merge 24 commits into
Conversation
dosubot
Bot
added
api
Code Review Agent Run #a1a3d1Actionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43176 +/- ##
=======================================
Coverage 84.12% 84.12%
=======================================
Files 3056 3056
Lines 201392 201394 +2
Branches 47090 47088 -2
=======================================
+ Hits 169419 169424 +5
+ Misses 28727 28722 -5
- Partials 3246 3248 +2
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| set_form_data( | ||
| { | ||
| "datasource": {"id": datasource_id, "type": datasource_type}, | ||
| "queries": [ | ||
| _serialize_query(query, form_data) for query in query_context.queries | ||
| ], | ||
| "form_data": form_data, | ||
| } |
There was a problem hiding this comment.
Suggestion: The synthetic global form data keeps the original chart fields only under the nested form_data key, while get_form_data() exposes the first serialized query as the top-level form data. Consequently, fields such as adhoc_filters and extra_form_data from query_context.form_data are invisible to Jinja macros that inspect top-level form data, notably get_time_filter(column), which can render a fallback or the wrong temporal range even though the executed query has the filter. Merge the relevant original form-data fields into the top level or serialize them into the query using the chart-data request shape. [api mismatch]
Severity Level: Major ⚠️
- ❌ MCP SQL output can diverge from executed chart SQL.
- ⚠️ Column-specific Jinja time filters are ignored.
- ⚠️ Virtual dataset previews can show incorrect temporal ranges.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/data/form_data.py
**Line:** 55:62
**Comment:**
*Api Mismatch: The synthetic global form data keeps the original chart fields only under the nested `form_data` key, while `get_form_data()` exposes the first serialized query as the top-level form data. Consequently, fields such as `adhoc_filters` and `extra_form_data` from `query_context.form_data` are invisible to Jinja macros that inspect top-level form data, notably `get_time_filter(column)`, which can render a fallback or the wrong temporal range even though the executed query has the filter. Merge the relevant original form-data fields into the top level or serialize them into the query using the chart-data request shape.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
I think this is covered too, just less directly. _serialize_query still carries query.filter up as filters, and get_time_filter() runs convert_legacy_filters_into_adhoc() first, converting any TEMPORAL_RANGE entries into adhoc_filters before matching on column. No test covers the column-specific case though, so flagging before closing this out.
There was a problem hiding this comment.
Reopening this one briefly, @rusackas. On the dataset path I do not think it is covered: by the time _serialize_query runs, _apply_granularity has already deleted the TEMPORAL_RANGE entry from QueryObject.filter (it fires whenever granularity names the filter's column, which query_dataset always does when time_range is set), so there is nothing left for convert_legacy_filters_into_adhoc() to convert. I verified it on the head: after the real factory, queries[0].filter is [] and get_time_filter() returns No filter. Details and a suggested fix in my comment on query_dataset.py. Are the chart tool paths safe only because their form_data carries a top level time_range, or is there another mechanism I am missing for the column specific case there?
There was a problem hiding this comment.
You’re right—the dataset path is not covered by this helper as currently written.
_serialize_query() only sees the post-factory QueryObject. For query_dataset, _apply_granularity() removes the TEMPORAL_RANGE filter when the temporal column is used as granularity, so both:
query.filter == []
query.time_range is Nonemay be true by the time set_query_context_form_data() runs. The fallback hoisting logic therefore has nothing to serialize, and get_time_filter() can still return No filter.
The chart paths are safer when their original form_data includes a top-level time_range, but that does not fully solve column-specific get_time_filter("column") behavior. They should still rely on the serialized query’s filter payload where possible.
The minimal fix is to preserve the request’s temporal filter for the Jinja payload before the query factory mutates the QueryObject, without adding QueryObject.time_range (which would affect cache-key behavior). For example, pass the original filters/time range through the query context’s form data or attach a chart-data-shaped filter payload, then let _serialize_query() use that preserved value when the final query has no temporal filter.
So the concern is valid and should remain open until the dataset path has a regression test asserting get_time_filter().time_range after the real QueryContextFactory runs.
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @AmoghAtreya, this closes out the follow-up from #42822 nicely, and the approach is the right one: the synthesized payload mirrors the chart-data API body shape, so the g fallback in get_form_data() and convert_legacy_filters_into_adhoc() see the same inputs the executed QueryObject carries.
I verified this empirically. All 8 new regression tests fail on the parent commit and pass on this head, and the full mcp_service, charts and tasks unit suites pass locally (3,976 tests) except one, see below. I also ran it live: booted the MCP service from this branch over streamable HTTP, built a virtual dataset using {{ filter_values("region") }}, {{ url_param("tenant") }}, {{ get_time_filter().time_range }} and {{ get_time_filter("order_date").time_range }}, and called the tools end to end. On this branch get_chart_sql returns exactly the SQL get_chart_data executed, with the macros resolved (North, acme, Last month), and query_dataset/get_table resolve the time macros too. The same scenario on the parent commit renders none and No filter across all of them, so the fix does what it says.
Blocking items:
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:1612(intest_oauth2_redirect_on_form_data_key_path) still stubsbuild_query_context_from_form_dataas a bareobject(), and_query_from_form_datanow dereferencesquery_context.form_databefore running the command, so the OAuth error surfaces asInternalErrorand the unit-tests job fails. I verified the one-line fix locally, it matches your other stub updates:
monkeypatch.setattr(
chart_data_module,
"build_query_context_from_form_data",
lambda *args, **kwargs: SimpleNamespace(queries=[], form_data={}),
)-
pre-commit: 5 mypy errors and one ruff-format reflow. I reproduced them with the repo's mypy hook and confirmed they clear with casts in the three test files (
cast(Any, ...)for theSimpleNamespacechart and context doubles,dict[str, object]for the baredictreturn attest_compile.py:609) plusruff formatontests/unit_tests/charts/data/form_data_test.py. -
Rebase needed: #42822 merged about an hour after this PR opened, so master already has
superset/charts/data/form_data.py, and this branch conflicts with it (add/add on the helper and its test file, content conflict intest_query_dataset.py). When rebasing, keep yourdatasource_id: int | strsignature (master hasint, butresolve_form_data_datasourcecan return a string id on the combined-datasource path). Yourquery_dataset/get_tabletime_rangecopy and all the sibling-path coverage are net new on top of master, so they survive the rebase.
One observation from the live run, not a blocker: with the modern saved-query shape where the temporal range only exists as a TEMPORAL_RANGE filter (no time_range key on the query), {{ get_time_filter().time_range }} without a column renders No filter on the chart paths, while the column-scoped {{ get_time_filter("order_date").time_range }} resolves. That matches chart-data API semantics (QueryObjectFactory.create keeps QueryObject.time_range as None and only derives from_dttm/to_dttm from the filter), and executed and displayed SQL stay identical, so it is consistent. Just worth knowing when running step 3 of your testing instructions, which only holds when the query carries an explicit time_range.
Happy to approve once CI is green on the rebased branch.
| query_context, | ||
| chart.datasource_id, | ||
| chart.datasource_type, | ||
| ) |
There was a problem hiding this comment.
Not a blocker, a parity question: here the Jinja fallback gets chart.datasource_id, but the command executes the datasource encoded in the saved query_context. If a chart is repointed to another dataset while its saved query_context is stale, get_dataset_id_from_context() (and so an implicit metric() lookup) resolves a different dataset than the SQL we return. Since query_context is already loaded here, query_context.datasource.id and str(query_context.datasource.type) would make the template context and the execution target unable to diverge, matching the fallback you already added in _sql_from_form_data. Same question for the saved-context branch in get_chart_data (line 681), where merge_extra_form_data_filters_into_query a few lines up already uses query_context_json["datasource"]["id"]. Or is the chart-level datasource intentional there?
|
Heya @AmoghAtreya, @gabotorresruiz left a really thorough review above with the exact fixes needed (test stub, pre-commit, rebase off |
|
Thanks @gabotorresruiz for the super thorough review and detailed feedback, and @rusackas for checking in! Sorry for the delay on my end—I was out of town for the past week and was unable to work on the fixes. I'm getting to work on the requested fixes now (updating the test stub, fixing the pre-commit issues, and rebasing on master). I'll also address the comments from CodeAnt and update the stale datasource references in get_chart_sql.py so the template context and execution target stay strictly in sync. I'll ping back as soon as the updated commits are pushed and CI is green! |
github-actions
Bot
added
the
requires:rebase
ff13ce0 to
d3fe6a8
Compare
github-actions
Bot
removed
api
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thanks again @gabotorresruiz for the thorough review and detailed instructions on what to fix and @rusackas for following up. Sorry for the delay , but the blocking items from the last review are in. Here's what I fixed:
CI is green on the latest commits. The Please let me know if there's any other changes you would like me to implement. Happy to follow up on anything else! |
Code Review Agent Run #e1ef8aActionable 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 |
github-actions
Bot
added
the
requires:rebase
|
@gabotorresruiz the three blockers are in (OAuth stub, mypy/ruff, rebase) and CI's green now. Looks like your datasource parity question got fixed too, both |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @AmoghAtreya for the careful follow-through, and @rusackas for shepherding. All three blockers from my last review are resolved. I verified each at b98f43b rather than taking the diff's word for it:
- The OAuth
form_data_keystub now returnsSimpleNamespace(queries=[], form_data={})and the routing tests pass. - The repo's mypy pre-commit hook passes locally on all changed files, ruff format is clean, and CI is green at the head.
- The branch is synced over #42822 with
datasource_id: int | strkept. Full local run of themcp_service,chartsandtabular_queryunit suites: 4,050 passed, 2 xfailed, 0 failed.
The datasource parity question from my inline comment is addressed too: get_chart_sql and get_chart_data now prefer query_context.datasource over the chart row, so the template context cannot diverge from the execution target.
Since form_data.py, compile.py, preview_utils.py and the preview seed hunks are unchanged from the head I verified live in August (virtual dataset with filter_values, url_param and get_time_filter over the MCP HTTP transport, rendered SQL matching executed SQL on every path), that end-to-end evidence carries over to this head.
One mechanical item before merge: master moved again after your sync, and the branch now conflicts in three test files (test_compile.py, test_preview_utils.py, test_get_table.py, from #43943 and #44105). One more master sync should clear it; please re-run those suites afterwards since #44105 landed in the same tabular time-range territory. The openapi.json regen is fine to carry, it is the CI-enforced sync for the schema change already on master.
Approving. And thanks for consolidating #43711 into this PR, the compile and preview paths finally get the same Jinja context as every other ChartDataCommand site.
|
@AmoghAtreya master moved again since your last sync, GitHub shows conflicts now in the test files @gabotorresruiz flagged (test_compile.py, test_preview_utils.py, test_get_table.py, from #43943 and #44105). Rebase and re-run those suites and this should be ready, as @gabotorresruiz already approved the substance. Let us know if you need a hand. |
b98f43b to
3d17c49
Compare
|
Thanks for the commit, @rusackas ! If you'd like, I can fix the failing tests and push my updates once the current test run finishes. |
Harden set_query_context_form_data for incomplete query-context doubles and hoist TEMPORAL_RANGE into Jinja form data so MCP queries keep rollover cache behavior from apache#43914. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Granularity processing strips TEMPORAL_RANGE before the form-data helper runs, so read the raw query dicts in cache_values and reconstruct one-sided semantic-view comparisons. Co-authored-by: Cursor <cursoragent@cursor.com>
Chart-like preview objects may omit datasource_id; skip the helper instead of raising so typed errors still surface. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the shared serializer chart-API identical so async get_time_filter() is not invented from TEMPORAL_RANGE filters. Tabular callers pass the original range explicitly. Co-authored-by: Cursor <cursoragent@cursor.com>
bd6d88f to
8de6b69
Compare
The getattr guards only existed for test stubs; production always passes a QueryObject, so direct access is clearer and will fail loudly if those fields move. Co-authored-by: Cursor <cursoragent@cursor.com>
getattr defaults only soft-failed for incomplete stubs; QueryContext defines both fields, so direct access matches the annotation and fails loudly on renames. Co-authored-by: Cursor <cursoragent@cursor.com>
_time_range_from_filters and _time_range_filters were easy to confuse; rename the reader to _temporal_range_in_filters. Co-authored-by: Cursor <cursoragent@cursor.com>
…compile No circular dependency requires a deferred import; sibling chart tools already import it at the top of the module. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Direct QueryObject access stays; the QueryContext envelope again accepts unit-test stubs so preview/compile mocks without form_data or list queries do not crash. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
removed
the
requires:rebase
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@rusackas just rebased the branch and got it updated with master! Please let me know if there are any other changes to make. |
![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 #4f788a
Actionable Suggestions - 3
-
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py - 1
- Untyped lambda in new test · Line 611-611
-
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py - 2
- Missing docstring on test helper · Line 237-237
- Unannotated locals in tests · Line 265-276
Additional Suggestions - 7
-
tests/unit_tests/common/test_tabular_query.py - 2
-
Helper missing docstring · Line 515-522`_factory_dataset` is the only new function in this file without a docstring, while sibling helpers like `_column` and every new test document their intent. A one-liner noting that every column is temporal except the literal `"region"` would also document the sentinel coupling noted separately. ([BITO.md](BITO.md))
-
Magic string sentinel · Line 518-521The literal `"region"` encodes the business rule 'this column is the non-temporal one' in three places: the `is_dttm` predicate (518), the `main_dttm_col` generator (521), and the call site `_factory_dataset("region", "order_date")` (561). Renaming the test column would silently flip every column's `is_dttm`. Hoist it into a named constant.
-
-
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py - 1
-
Duplicated test setup block · Line 265-295Both tests repeat identical setup: the `importlib.import_module` of `get_data_command`, the `QueryObject(filters=[...], time_range="Last week")` construction, and a near-identical `chart` SimpleNamespace (lines 319-329 and 346-353 mirror this block). A shared helper or fixture would keep each test focused on its differing patches.
-
-
superset/mcp_service/chart/tool/get_chart_preview.py - 3
-
New Jinja-context behavior untested · Line 224-247The new `_run_chart_data_command` behavior — publishing `g.form_data` via `set_query_context_form_data` before `ChartDataCommand.run` — has no test coverage: no test references the helper, and existing tests in test_get_chart_preview.py patch `ChartDataCommand` without asserting the Jinja-context publication. BITO.md rule 11730 asks for tests covering new feature behavior; please add one.
-
Inline import violates rule 12745 · Line 226-226BITO.md adaptive rule 12745 requires module-level imports unless a documented circular dependency exists. `superset.commands.chart.data.get_data_command` has no mcp_service back-import (verified), and this file already imports `superset.commands.exceptions` and `set_query_context_form_data` at module level, so the inline import in `_run_chart_data_command` is unnecessary; move it beside line 29.
-
Use specific type for query_context · Line 224-224The `query_context` parameter is typed as `Any`, which is disallowed by `ANN401`. Use a specific type like `QueryContext` from `superset.models.sql.types` to improve type safety.
-
-
tests/unit_tests/tasks/test_async_queries.py - 1
-
Missing local var type hint · Line 235-235The new local `query` (line 235) lacks a type annotation, contrary to the org-mandated BITO.md adaptive rule 13153/13347 requiring explicit annotations on all local variables in test files. Add `query: QueryObject = QueryObject(...)` for consistency with the repo's typing standard.
-
Review Details
-
Files reviewed - 21 · Commit Range:
7c49ba6..2bb5c98- superset/charts/data/form_data.py
- superset/common/tabular_query.py
- superset/datasource/api.py
- superset/mcp_service/chart/compile.py
- superset/mcp_service/chart/preview_utils.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_preview.py
- superset/mcp_service/chart/tool/get_chart_sql.py
- superset/mcp_service/dataset/tool/query_dataset.py
- superset/mcp_service/semantic_layer/tool/get_table.py
- tests/unit_tests/charts/data/form_data_test.py
- tests/unit_tests/common/test_tabular_query.py
- tests/unit_tests/mcp_service/chart/test_compile.py
- tests/unit_tests/mcp_service/chart/test_preview_utils.py
- tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
- tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py
- tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py
- tests/unit_tests/tasks/test_async_queries.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 an incremental AI Review. -
/review full- 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
Code Review Agent Run #3d1718Actionable 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 |
github-actions
Bot
added
the
requires:rebase
Co-authored-by: Cursor <cursoragent@cursor.com>
CodeAnt PR Risk: Medium Risk
Assessed commit: |
github-actions
Bot
added
requires:rebase
Co-authored-by: Cursor <cursoragent@cursor.com>
The master merge left a duplicate table seed call and a redefined import, which broke semantic-view compile and pre-commit ruff. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
removed
the
requires:rebase
SUMMARY
Follow-up to #42822 / #43174. Chart execution could resolve request-dependent Jinja (
filter_values,get_filters,url_param,get_time_filter, implicitmetric()dataset lookup) while sibling MCP paths (get_chart_sql, preview, compile,query_dataset,get_table) still rendered fallbacks such asNo filter, so displayed SQL could diverge from executed SQL.This extracts a shared helper that serializes a real
QueryObject/QueryContextontog.form_datain the chart-data API shape, and applies it immediately before every MCPChartDataCommandconstruction. Forquery_datasetandget_table,execute_tabular_querypasses the resolvedtime_rangealongside the query dict soget_time_filter()sees the same value as SQL execution, without putting it onQueryObject.time_rangeitself (keeps the sc-120967 cache-key fix intact). Guestauthorize_querystill runs first. Non-MCPChartDataCommandsites are unchanged.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
{{ filter_values('region') }},{{ url_param('tenant') }}, and{{ get_time_filter().time_range }}.url_params.tenant, and a time range.get_chart_dataand confirm the macros resolve (notNo filter/ empty).get_chart_sqlandget_chart_previewon the same chart and confirm the rendered SQL matches the executed query.form_data_key.query_dataset/get_tablewith filters andtime_rangeand confirmget_time_filter()matches the request.ADDITIONAL INFORMATION