You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "AmoghAtreya (via GitHub)" <gi...@apache.org> on 2026/08/14 19:01:42 UTC
[PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
AmoghAtreya opened a new pull request, #43176:
URL: https://github.com/apache/superset/pull/43176
### SUMMARY
Follow-up to #42822 / #43174. Chart execution could resolve request-dependent Jinja (`filter_values`, `get_filters`, `url_param`, `get_time_filter`, implicit `metric()` dataset lookup) while sibling MCP paths (`get_chart_sql`, preview, compile, `query_dataset`, `get_table`) still rendered fallbacks such as `No filter`, so displayed SQL could diverge from executed SQL.
This extracts a shared helper that serializes a real `QueryObject`/`QueryContext` onto `g.form_data` in the chart-data API shape, and applies it immediately before every MCP `ChartDataCommand` construction. `query_dataset` and `get_table` also copy `time_range` onto the query dict so `get_time_filter()` sees the same value as SQL execution. Guest `authorize_query` still runs first. Non-MCP `ChartDataCommand` sites are unchanged.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TESTING INSTRUCTIONS
1. Create a virtual dataset whose SQL uses `{{ filter_values('region') }}`, `{{ url_param('tenant') }}`, and `{{ get_time_filter().time_range }}`.
2. Build a chart on that dataset with a region filter, `url_params.tenant`, and a time range.
3. Call `get_chart_data` and confirm the macros resolve (not `No filter` / empty).
4. Call `get_chart_sql` and `get_chart_preview` on the same chart and confirm the rendered SQL matches the executed query.
5. Repeat with an unsaved chart via `form_data_key`.
6. Call `query_dataset` / `get_table` with filters and `time_range` and confirm `get_time_filter()` matches the request.
7. Confirm an embedded guest still cannot query a chart outside the guest dashboard scope.
```bash
pytest -q \
tests/unit_tests/charts/data/form_data_test.py \
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py \
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py \
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py \
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py \
tests/unit_tests/mcp_service/chart/test_preview_utils.py \
tests/unit_tests/mcp_service/chart/test_compile.py \
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py \
tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
### ADDITIONAL INFORMATION
- [X] Has associated issue: Fixes #43174
- [ ] 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4178982832
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -228,6 +231,133 @@ def test_null_chart_metadata(self):
assert output.datasource_name is None
+class TestChartSqlJinjaContext:
+ """SQL generation must expose the same Jinja inputs as chart data execution."""
+
+ def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
Review Comment:
<!-- Bito Reply -->
The suggestion to add docstrings to the test helper functions is appropriate. Adding these docstrings improves code maintainability and adheres to the project's documentation standards for new test functions.
**tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py**
```
def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
"""Build a ChartDataCommand double that asserts request-dependent Jinja inputs during run()."""
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5684660888
@AmoghAtreya still conflicting with `master`, and the commits since your reply didn't touch the rebase, `test_compile.py`, `test_preview_utils.py`, and `test_get_table.py` are still the ones to resolve.
Also, bito's one-sided-range note on `tabular_query.py` seems right: get_table's `rewrite_one_sided_time_range` path emits GTE/LT filters instead of TEMPORAL_RANGE, so `_time_range_from_filters` can't hoist it and Jinja loses the range for semantic views on that path. Worth a fix before this lands.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5702768408
Thanks @gabotorresruiz — the hoist-from-processed-filters claim didn’t actually hold, and your `cache_values` catch was the right fix.
`_apply_granularity` deletes the `TEMPORAL_RANGE` filter on the granularity column before `set_query_context_form_data` runs, so Jinja was seeing `No filter`. The helper now falls back to the raw pre-processing query dicts on `query_context.cache_values["queries"]`. One-sided semantic-view `>=` / `<` rewrites are reconstructed too. `QueryObject.time_range` stays unset, so the cache-key/rollover fix is unchanged.
`test_query_dataset_exposes_filters_to_jinja_macros` and the matching `get_table` test now patch `QueryContextFactory._convert_to_model` instead of `create`, so they actually run `_apply_granularity`. They assert the processed filters no longer contain `TEMPORAL_RANGE` and that `get_time_filter()` still returns `Last week`.
Rebased onto latest master. Please let me know if there's anything else to change!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5648187102
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5801828145
Ahh, thanks for tracing that down. The tag skew got fixed on `master` in #44534 a little after your comment, so another Update branch should clear the unit-test failures and then this is good to go.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174970241
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val": "boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ columns=["region"],
+ metrics=["count"],
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {
+ "queries": [
+ {
+ "data": [{"region": "North"}],
+ "colnames": ["region"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ preview_module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ monkeypatch.setattr(
+ preview_module,
+ "build_query_context_from_form_data",
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped lambda in new test</b></div>
<div id="fix">
BITO.md adaptive rule 13350 requires typed helper functions instead of untyped lambdas. The new test adds `lambda *args, **kwargs: query_context` (line 612) patching `build_query_context_from_form_data`; define a nested `def _fake_build_query_context(*args: Any, **kwargs: Any) -> SimpleNamespace` above the `monkeypatch.setattr` call and pass its name. The file's 10 pre-existing lambdas are unchanged and out of scope.
</div>
</div>
<small><i>Code Review Run #4f788a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -228,6 +231,133 @@ def test_null_chart_metadata(self):
assert output.datasource_name is None
+class TestChartSqlJinjaContext:
+ """SQL generation must expose the same Jinja inputs as chart data execution."""
+
+ def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing docstring on test helper</b></div>
<div id="fix">
`_command_observing_jinja` (and the nested `ChartDataCommand` double) has no docstring, unlike the two test methods in this class. BITO rule 12147 requires docstrings on every new function in test files, explicitly including helper functions. Add a one-liner such as: "Build a ChartDataCommand double that asserts request-dependent Jinja inputs during run()."
</div>
</div>
<small><i>Code Review Run #4f788a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -228,6 +231,133 @@ def test_null_chart_metadata(self):
assert output.datasource_name is None
+class TestChartSqlJinjaContext:
+ """SQL generation must expose the same Jinja inputs as chart data execution."""
+
+ def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ class ChartDataCommand:
+ def __init__(self, query_context: object) -> None:
+ self.query_context = query_context
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {"queries": [{"query": "SELECT 1", "language": "sql"}]}
+
+ return ChartDataCommand
+
+ def test_sql_from_form_data_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """The form_data SQL path publishes filters, time_range, and url_params."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ datasource=SimpleNamespace(id=7, type="table"),
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unannotated locals in tests</b></div>
<div id="fix">
Locals `get_data_command_module`, `query`, `query_context`, `chart`, and `result` lack explicit type annotations; the second test mirrors this at lines 319-329 and 346-355. BITO rule 13153 requires annotations on all test-file locals even when inferable. Annotate e.g. `query: QueryObject`, `query_context: SimpleNamespace`, `result: ChartSql | ChartError`.
</div>
</div>
<small><i>Code Review Run #4f788a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5446004800
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!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5883131924
Heads up @AmoghAtreya, master's moved on since the last Update branch, so this isn't just the docker tag-skew CI hiccup anymore. Merging master now actually conflicts in `tests/unit_tests/mcp_service/chart/test_preview_utils.py`. Once that's resolved this should be good to go, the substance has already checked out through a few rounds.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174216365
##########
superset/common/tabular_query.py:
##########
@@ -397,6 +402,28 @@ def build_query_dict(
return query_dict
+def _time_range_from_filters(filters: Any) -> str | None:
Review Comment:
<!-- Bito Reply -->
The renaming of the helper function to `_temporal_range_in_filters` is a positive change. It effectively resolves the potential for confusion with the existing `_time_range_filters` function by providing a clearer, more distinct name that better reflects its purpose.
**superset/common/tabular_query.py**
```
+def _temporal_range_in_filters(filters: Any) -> str | None:
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r3825285636
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -185,6 +187,11 @@ def _sql_from_saved_query_context(
query_context = ChartDataQueryContextSchema().load(qc_json)
query_context.result_type = ChartDataResultType.QUERY
+ set_query_context_form_data(
+ query_context,
+ chart.datasource_id,
+ chart.datasource_type,
+ )
Review 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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5596416474
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:
1) OAuth `form_data_key` stub now returns `SimpleNamespace(queries=[], form_data={}) so _query_from_form_data` doesn’t blow up
2) mypy casts / ruff-format on the helper tests
3) rebased/merged onto master, keeping `datasource_id: int | str`
4) saved SQL / chart-data Jinja fallback now prefers the executed `query_context` datasource (with JSON, then chart, as a fallback so MagicMock doubles still work)
CI is green on the latest commits. The `get_time_filter()` no-column / `TEMPORAL_RANGE`-only note matches what I see as well — same as the chart-data API, so executed and displayed SQL stay in sync.
Please let me know if there's any other changes you would like me to implement. Happy to follow up on anything else!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4018374136
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -185,6 +187,11 @@ def _sql_from_saved_query_context(
query_context = ChartDataQueryContextSchema().load(qc_json)
query_context.result_type = ChartDataResultType.QUERY
+ set_query_context_form_data(
+ query_context,
+ chart.datasource_id,
+ chart.datasource_type,
+ )
Review Comment:
Oops, that last reply was meant for the other thread, ignore it here. On this one: `_datasource_id_and_type_from_query_context` already prefers `query_context.datasource.id`/`.type`, `chart.datasource_id` only kicks in as a last resort when that's not an int/str (MagicMock test doubles etc). That's the order it's been in since before this thread, just split into its own helper later to satisfy ruff's complexity check. Resolving.
##########
superset/charts/data/form_data.py:
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from typing import Any, TYPE_CHECKING
+
+from flask import g
+
+if TYPE_CHECKING:
+ from superset.common.query_context import QueryContext
+ from superset.common.query_object import QueryObject
+
+
+def set_form_data(form_data: dict[str, Any]) -> None:
+ """Expose chart data request fields to Jinja template macros."""
+ g.form_data = form_data
+
+
+def _serialize_query(
+ query: QueryObject,
+ form_data: dict[str, Any],
+) -> dict[str, Any]:
+ """Serialize query fields consumed by the Jinja form-data fallback."""
+ query_data = dict(query.to_dict())
+ query_data["filters"] = query.filter
+ if query.time_range is not None:
+ query_data["time_range"] = query.time_range
+ if url_params := form_data.get("url_params"):
+ query_data["url_params"] = url_params
+ return query_data
+
+
+def set_query_context_form_data(
+ query_context: QueryContext,
+ datasource_id: int | str,
+ datasource_type: str,
+) -> None:
+ """Expose a programmatically-created query like a chart data API request."""
+ form_data = query_context.form_data or {}
+ 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,
+ }
Review 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5297096760
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5780795523
Not this branch, @rusackas. I checked the result at `d11807b2`.
The Update branch merge did its job on the translations side: `messages.pot` now matches master byte for byte, `4c229cb6` is an ancestor, and `babel-extract` is green.
What is left is `docker_cache_ref_test.py::test_cache_ref_matches_dockerfile_py_ver` for `.github/workflows/superset-frontend.yml` and `.github/workflows/frontend-bundle-size-nightly.yml`. `Dockerfile` declares `ARG PY_VER=3.11-slim-trixie` while both workflows pull `apache/superset-cache:3.11.14-slim-trixie`. I ran the test's comparison against current master `21db4a5e` and it fails there identically, same two files, same values, while the three compose files pass. The branch only inherited it by merging master, and this PR touches neither workflow nor the `Dockerfile`.
I also re-diffed after the merge: the PR's net change against master is byte for byte what it was at `cf1be0c`, so the approval still stands. Nothing to do here until master fixes the tag skew, either by bumping the two workflows to `3.11-slim-trixie` or by pinning `ARG PY_VER` back to the patch version.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174220839
##########
superset/mcp_service/chart/compile.py:
##########
@@ -94,6 +94,7 @@ def _compile_chart(
Returns a :class:`CompileResult` with ``success=True`` when the
query executes cleanly.
"""
+ from superset.charts.data.form_data import set_query_context_form_data
Review Comment:
<!-- Bito Reply -->
The suggestion to move the function-local import to the module level is appropriate. It aligns the code with the established project pattern used in sibling tools and avoids unnecessary function-local imports, which is generally preferred for clarity and consistency in this codebase.
**superset/mcp_service/chart/compile.py**
```
from superset.charts.data.form_data import set_query_context_form_data
def _compile_chart(
# ...
Returns a :class:`CompileResult` with ``success=True`` when the
query executes cleanly.
"""
# ...
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5704538238
Thanks @gabotorresruiz — the hoist is out of the shared serializer.
`set_query_context_form_data` no longer invents a range from filters or `cache_values`, so chart-data / async `get_time_filter()` matches master again. Tabular callers pass the original range into `execute_tabular_query`; `QueryObject.time_range` stays unset. I skipped `>=` / `<` reconstruction because of the `get_table` comparison-filter edge.
Added the chart-shaped `NO_TIME_RANGE` case, the `get_table time_column` case, moved the hoist tests onto `execute_tabular_query`, and pinned `TZ=UTC` on the rollover test.
Will add update the PR description too.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5649043573
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-6310049655
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging.
- Original chart fields such as adhoc_filters and extra_form_data remain nested, so Jinja fallback macros may not see them and can use a different time filter.
**Assessed commit:** `08956a769d3e`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r3786506106
##########
superset/charts/data/form_data.py:
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from typing import Any, TYPE_CHECKING
+
+from flask import g
+
+if TYPE_CHECKING:
+ from superset.common.query_context import QueryContext
+ from superset.common.query_object import QueryObject
+
+
+def set_form_data(form_data: dict[str, Any]) -> None:
+ """Expose chart data request fields to Jinja template macros."""
+ g.form_data = form_data
+
+
+def _serialize_query(
+ query: QueryObject,
+ form_data: dict[str, Any],
+) -> dict[str, Any]:
+ """Serialize query fields consumed by the Jinja form-data fallback."""
+ query_data = dict(query.to_dict())
+ query_data["filters"] = query.filter
+ if query.time_range is not None:
+ query_data["time_range"] = query.time_range
+ if url_params := form_data.get("url_params"):
+ query_data["url_params"] = url_params
+ return query_data
+
+
+def set_query_context_form_data(
+ query_context: QueryContext,
+ datasource_id: int | str,
+ datasource_type: str,
+) -> None:
+ """Expose a programmatically-created query like a chart data API request."""
+ form_data = query_context.form_data or {}
+ 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,
+ }
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP SQL output can diverge from executed chart SQL.
- ⚠️ Column-specific Jinja time filters are ignored.
- ⚠️ Virtual dataset previews can show incorrect temporal ranges.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=54f8d9f4043e43ea89f925f371d71c48&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=54f8d9f4043e43ea89f925f371d71c48&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43176&comment_hash=65e2f73154011f53eec9ae7e16fdccae5924dca127276faebae67144da8ee8c7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43176&comment_hash=65e2f73154011f53eec9ae7e16fdccae5924dca127276faebae67144da8ee8c7&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -185,6 +187,11 @@ def _sql_from_saved_query_context(
query_context = ChartDataQueryContextSchema().load(qc_json)
query_context.result_type = ChartDataResultType.QUERY
+ set_query_context_form_data(
+ query_context,
+ chart.datasource_id,
+ chart.datasource_type,
+ )
Review Comment:
**Suggestion:** The saved-query-context path executes the datasource encoded in `query_context`, but the new Jinja fallback is populated with `chart.datasource_id` and `chart.datasource_type`. If a chart's saved query context is stale after its datasource changes, `metric()` or `get_dataset_id_from_context()` resolves the chart's current datasource while SQL generation queries the saved context's datasource, producing inconsistent or cross-dataset SQL. Use the datasource carried by the parsed `query_context` so the template context and execution target cannot diverge. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Saved chart SQL can target the wrong dataset.
- ⚠️ Jinja `metric()` expansion can mismatch execution.
- ⚠️ Chart datasource changes can produce misleading SQL previews.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b6527dd9e4d24f40bf7e53c14b27c39b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b6527dd9e4d24f40bf7e53c14b27c39b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_sql.py
**Line:** 190:194
**Comment:**
*Stale Reference: The saved-query-context path executes the datasource encoded in `query_context`, but the new Jinja fallback is populated with `chart.datasource_id` and `chart.datasource_type`. If a chart's saved query context is stale after its datasource changes, `metric()` or `get_dataset_id_from_context()` resolves the chart's current datasource while SQL generation queries the saved context's datasource, producing inconsistent or cross-dataset SQL. Use the datasource carried by the parsed `query_context` so the template context and execution target cannot diverge.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43176&comment_hash=f7f119f66cd8a512ed35566da6f421ef2c6e8cc95699fda4bf8d0eaf08c2e9f9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43176&comment_hash=f7f119f66cd8a512ed35566da6f421ef2c6e8cc95699fda4bf8d0eaf08c2e9f9&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4068195260
##########
tests/unit_tests/mcp_service/chart/test_compile.py:
##########
@@ -766,3 +771,59 @@ def test_aggregation_ambiguity_returns_validation_errors() -> None:
)
assert len(errors) == 1
assert errors[0].error_code == "AMBIGUOUS_DATASET_REFERENCE"
+
+
+def test_compile_chart_exposes_jinja_context() -> None:
+ """Compile checks publish the same Jinja inputs used during chart execution."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from superset.mcp_service.chart.compile import _compile_chart
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, object]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {"queries": [{"data": [{"region": "North"}]}]}
+
+ with (
+ current_app.test_request_context(),
+ patch(
+ "superset.common.query_context_factory.QueryContextFactory.create",
+ return_value=query_context,
+ ),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
+ ChartDataCommand,
+ ),
+ ):
+ result = _compile_chart(
+ {
+ "metrics": ["count"],
+ "url_params": {"tenant": "acme"},
+ "time_range": "Last week",
+ },
+ dataset_id=7,
+ )
+
+ assert result.success is True
+ assert observed["ran"] is True
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test asserts helper, not compile wiring</b></div>
<div id="fix">
`run()` only calls `assert_request_dependent_jinja_macros()`, so the test proves the helper works (already covered by `test_query_context_form_data_supports_request_dependent_jinja_macros`) but never asserts what `_compile_chart` published via `set_query_context_form_data` (e.g. `g.form_data['datasource']`). Asserting the published state would test this wiring, not re-test the helper.
</div>
</div>
<small><i>Code Review Run #528d96</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val": "boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ columns=["region"],
+ metrics=["count"],
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Cryptic stub param qc</b></div>
<div id="fix">
The new test's `ChartDataCommand.__init__` names its parameter `qc`, while the five sibling stubs of the same class in this file (e.g. in `test_table_preview_converts_saved_adhoc_filters_to_query_filters` and `test_table_preview_uses_singular_metric`) all use `query_context`. The two-letter abbreviation is cryptic and breaks naming consistency across the duplicated stub pattern; renaming keeps the family uniform.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
def __init__(self, query_context: object) -> None:
self.query_context = query_context
````
</div>
</details>
</div>
<small><i>Code Review Run #528d96</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val": "boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ columns=["region"],
+ metrics=["count"],
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {
+ "queries": [
+ {
+ "data": [{"region": "North"}],
+ "colnames": ["region"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ preview_module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ monkeypatch.setattr(
+ preview_module,
+ "build_query_context_from_form_data",
+ lambda *args, **kwargs: query_context,
+ )
+ monkeypatch.setattr(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped lambda in monkeypatch</b></div>
<div id="fix">
The monkeypatch uses an untyped lambda (`lambda *args, **kwargs: query_context`). BITO rule 13350 asks that untyped lambdas be extracted into named helper functions with explicit type hints for parameters and return values. A typed `def _fake_build_query_context(*args: Any, **kwargs: Any) -> SimpleNamespace` defined above the `monkeypatch.setattr` call satisfies the rule and documents the stub's contract.
</div>
</div>
<small><i>Code Review Run #528d96</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/charts/data/form_data_test.py:
##########
@@ -15,11 +15,58 @@
# specific language governing permissions and limitations
# under the License.
-from typing import Any
+from types import SimpleNamespace
+from typing import Any, cast
from flask import current_app, g
-from superset.charts.data.form_data import set_form_data
+from superset.charts.data.form_data import (
+ set_form_data,
+ set_query_context_form_data,
+)
+from superset.common.query_object import QueryObject
+from superset.constants import NO_TIME_RANGE
+from superset.jinja_context import ExtraCache, get_dataset_id_from_context
+
+
+def _jinja_query_context(
+ *,
+ filters: list[dict[str, Any]] | None = None,
+ time_range: str = "Last week",
+ url_params: dict[str, str] | None = None,
+) -> SimpleNamespace:
+ """Build a QueryContext-shaped object from a real QueryObject."""
+ query = QueryObject(
+ filters=cast(Any, filters or [{"col": "region", "op": "IN", "val": ["North"]}]),
+ time_range=time_range,
+ )
+ return SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": url_params or {"tenant": "acme"}},
+ )
+
+
+def assert_request_dependent_jinja_macros(
+ *,
+ expected_filter_col: str = "region",
+ expected_filter_val: str = "North",
+ expected_url_param: str | None = "tenant",
+ expected_url_value: str = "acme",
+ expected_time_range: str | None = "Last week",
+ expected_dataset_id: int = 7,
+) -> None:
+ """Assert Jinja macros resolve the same inputs as a chart-data API request."""
+ extra_cache = ExtraCache()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing local type annotations</b></div>
<div id="fix">
BITO adaptive rule 13153 requires explicit type annotations on all test-file locals, even when inferable. The new locals `extra_cache` (line 59), `query_context` (lines 84, 111, 124, 143), and `query` (lines 98, 120, 135) are unannotated. Add e.g. `query: QueryObject` and `query_context: SimpleNamespace` to satisfy the org-mandated typing standard.
</div>
</div>
<small><i>Code Review Run #528d96</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val": "boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing local type annotations</b></div>
<div id="fix">
Several locals in the new test (`query`, `query_context`, `chart`, `get_data_command_module`, `preview_module`) lack explicit type annotations, while `observed: dict[str, bool]` on line 584 is annotated. BITO rule 13153 asks for explicit annotations on all test-file locals even when inferable; annotating these keeps the new test consistent with the rule and with the annotated `observed`/`captured_query_contexts` variables.
</div>
</div>
<small><i>Code Review Run #528d96</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -762,6 +762,84 @@ def run(self) -> dict[str, Any]:
assert queries[1]["metrics"] == ["sum__profit"]
assert queries[1]["row_limit"] == 99
+ @pytest.mark.asyncio
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant asyncio marker</b></div>
<div id="fix">
`pytest.ini` sets `asyncio_mode = auto`, so every async test is collected without a marker; the sibling tests in `TestUnsavedChartDataQueryConstruction` (e.g. `test_gauge_preserves_sort_order_and_validates_saved_metric_output`) still carry the decorator, but new code should not copy it. Drop `@pytest.mark.asyncio` here.
</div>
</div>
<small><i>Code Review Run #528d96</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -762,6 +762,84 @@ def run(self) -> dict[str, Any]:
assert queries[1]["metrics"] == ["sum__profit"]
assert queries[1]["row_limit"] == 99
+ @pytest.mark.asyncio
+ async def test_form_data_key_path_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Unsaved-chart execution publishes the same Jinja inputs as get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inline imports violate rule 12745</b></div>
<div id="fix">
BITO.md rule 12745 requires module-level imports unless a documented circular dependency exists. `current_app`, `QueryObject`, and `assert_request_dependent_jinja_macros` have no circularity here — the file already imports `superset.mcp_service.chart.tool.get_chart_data` at module scope (lines 42-49). Move these three imports to the top; the `importlib.import_module` calls can stay since they support monkeypatching.
</div>
</div>
<small><i>Code Review Run #528d96</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4030438675
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +33,132 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
+def _filter_op(op: Any) -> str | None:
+ if isinstance(op, FilterOperator):
+ return op.value
+ return op if isinstance(op, str) else None
+
+
+def _raw_query_dicts(query_context: QueryContext) -> list[Any]:
+ cache_values = getattr(query_context, "cache_values", None)
+ if not isinstance(cache_values, dict):
+ return []
+ return _as_query_list(cache_values.get("queries"))
+
+
+def _time_range_from_filters(filters: Any, time_column: Any = None) -> str | None:
+ """Read a time range for Jinja get_time_filter().
+
+ Prefer a TEMPORAL_RANGE comparator (datasets and two-sided views).
+ Semantic views rewrite one-sided ranges to ``>=`` / ``<`` on the
+ granularity column and emit no TEMPORAL_RANGE, so reconstruct those
+ in the ``since : until`` grammar get_time_filter() already parses.
+ """
+ if not isinstance(filters, list):
+ return None
+
+ since: str | None = None
+ until: str | None = None
+ for flt in filters:
+ if not isinstance(flt, dict):
+ continue
+ op = _filter_op(flt.get("op"))
+ val = flt.get("val")
+ if op == FilterOperator.TEMPORAL_RANGE.value and isinstance(val, str):
+ return val
+ if time_column is None or flt.get("col") != time_column:
+ continue
+ if not isinstance(val, str):
+ continue
+ if op == FilterOperator.GREATER_THAN_OR_EQUALS.value and since is None:
+ since = val
+ elif op == FilterOperator.LESS_THAN.value and until is None:
+ until = val
+
+ if since is None and until is None:
+ return None
+ return f"{since or ''} : {until or ''}"
+
+
+def _hoisted_time_range(
+ query: QueryObject,
+ filters: Any,
+ raw_query: Any = None,
+) -> str | None:
+ if (time_range := getattr(query, "time_range", None)) is not None:
+ return time_range
+ granularity = getattr(query, "granularity", None)
+ if hoisted := _time_range_from_filters(filters, granularity):
+ return hoisted
+ raw = raw_query if isinstance(raw_query, dict) else {}
+ # QueryContextFactory._apply_granularity deletes TEMPORAL_RANGE on the
+ # granularity column before this helper runs. The raw pre-processing
+ # dict in cache_values still has it.
+ return _time_range_from_filters(
+ raw.get("filters"), granularity or raw.get("granularity")
+ )
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ raw_query: Any = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
+ if hoisted := _hoisted_time_range(query, filters, raw_query):
Review Comment:
This block worries me a bit. `_hoisted_time_range` sits inside `_serialize_query`, so it fires for every `set_query_context_form_data` caller, including `superset/tasks/async_queries.py:298`, which is the async chart-data path and not an MCP one. The helper's contract is "Expose a programmatically-created query like a chart data API request" (`form_data.py:149`), and that parity is what it now breaks.
I verified it at `ddc5617`. Same ECharts timeseries query (string `x_axis`, no `granularity`, range carried only as a `TEMPORAL_RANGE` filter), one virtual dataset template, both paths:
```sql
-- chart-data API request body
SELECT * FROM events WHERE region = 'North' AND range = 'No filter'
-- g.form_data path (async worker, MCP tools)
SELECT * FROM events WHERE region = 'North' AND range = 'Last week'
```
`filter_values` agrees, which is this PR working. `get_time_filter()` does not. Both paths returned `No filter` at `b98f43b` and on master, so the divergence is new in `1468a17` / `27ded6a`.
The `>=` / `<` reconstruction has a second edge. `get_table` sets `granularity` from `time_column` even when no `time_range` is given (`get_table.py:112` and `:326`), so `_apply_granularity` has no `TEMPORAL_RANGE` to strip and removes nothing, and `GetTableFilter.op` is a free string (`semantic_layer/schemas.py:140`). So `get_table(time_column="ds", filters=[{"col": "ds", "op": ">=", "val": "Q1"}])` makes `get_time_filter()` raise `TimeRangeParseFailError: Cannot parse time string [Q1]` where master renders `No filter`. A parseable value is wrong in a quieter way: it turns a plain comparison filter into the reported time range.
Could we scope the hoist to the callers that need it? `execute_tabular_query` already holds the raw pre-factory dict, so it can pass the range explicitly and the shared helper goes back to being a pure serializer:
```python
# superset/common/tabular_query.py
set_query_context_form_data(
query_context,
datasource_id,
datasource_type,
time_range=_time_range_from_filters(
query_dict.get("filters"), query_dict.get("granularity")
),
)
```
with a `time_range: str | None = None` argument threaded down to `_serialize_query`, and `_hoisted_time_range` / `_raw_query_dicts` dropped from the shared path. Chart and async callers go back to byte identical with master, `QueryObject.time_range` stays unset so your rollover fix is untouched, and the existing hoist tests move to the `execute_tabular_query` call shape.
For coverage: a chart-shaped case in `tests/unit_tests/charts/data/form_data_test.py` asserting `get_time_filter().time_range == NO_TIME_RANGE` when the query carries only a `TEMPORAL_RANGE` filter and no explicit range, plus a `get_table` case with `time_column` set and no `time_range`. Both fail on this head today.
Happy to dig in with you on this one if it helps.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174215784
##########
superset/common/tabular_query.py:
##########
@@ -397,6 +402,28 @@ def build_query_dict(
return query_dict
+def _time_range_from_filters(filters: Any) -> str | None:
Review Comment:
Resolved this as well - Renamed `_time_range_from_filters` to `_temporal_range_in_filters` so it is harder to mix up with `_time_range_filters`. Pushed in latest commit
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4178982264
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -228,6 +231,133 @@ def test_null_chart_metadata(self):
assert output.datasource_name is None
+class TestChartSqlJinjaContext:
+ """SQL generation must expose the same Jinja inputs as chart data execution."""
+
+ def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
Review Comment:
Added one-line docstrings on `_command_observing_jinja` and the nested `ChartDataCommand` double.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5603401545
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5595873699
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 69bdd5a2d0b95f116a688ccb80e5b6fba801e74e |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa0e23a90a473000800234f |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43176--superset-docs-preview.netlify.app](https://deploy-preview-43176--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBDoMgEEDRu8xa4lSIWG_gMaY4phCBCSFq03h32f2_eH-QkqNUmAE6-BRK7tt681dv9MsaFZ2o4FMg5XKqfFUlhQ_PZ_sofufGVpY9_5a1wZEIedD0RjJWI-KEOGizwf0A) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43176--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174207599
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
+ resolved = time_range
+ if resolved is None:
+ obj_range = getattr(query, "time_range", None)
+ if isinstance(obj_range, str):
+ resolved = obj_range
+ if resolved is not None:
+ query_data["time_range"] = resolved
if url_params := form_data.get("url_params"):
query_data["url_params"] = url_params
return query_data
def set_query_context_form_data(
query_context: QueryContext,
- datasource_id: int,
+ datasource_id: int | str,
datasource_type: str,
+ time_range: str | None = None,
) -> None:
"""Expose a programmatically-created query like a chart data API request."""
- form_data = query_context.form_data or {}
+ form_data = _as_form_data_dict(getattr(query_context, "form_data", None))
+ queries = _as_query_list(getattr(query_context, "queries", None))
Review Comment:
<!-- Bito Reply -->
The changes you have pushed correctly address the issue of relying on `getattr` with defaults, which previously caused soft-failures for incomplete stubs. By introducing `_as_form_data_dict` and `_as_query_list` to read `form_data` and `queries` directly from `QueryContext`, you have improved the robustness of the Jinja context population. This approach ensures that the helper functions explicitly handle the expected attributes, avoiding the silent empty-context behavior that was previously flagged.
**superset/charts/data/form_data.py**
```
def _as_form_data_dict(value: Any) -> dict[str, Any]:
return value if isinstance(value, dict) else {}
def _as_query_list(value: Any) -> list[Any]:
if isinstance(value, (list, tuple)):
return list(value)
return []
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174223630
##########
tests/unit_tests/mcp_service/chart/test_preview_utils.py:
##########
@@ -398,3 +398,80 @@ def test_vega_preview_y_axis_fallback_accepts_decimal() -> None:
)
assert encoding["y"]["field"] == "revenue"
+
+
+def test_generate_preview_from_form_data_exposes_jinja_context(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """Unsaved-chart previews expose the same Jinja inputs as execution."""
+ from types import SimpleNamespace
+ from typing import Any
+ from unittest.mock import MagicMock
Review Comment:
<!-- Bito Reply -->
The redundant local import of `MagicMock` has been removed, and the module-level import is now correctly utilized. This change is appropriate and improves the code by eliminating unnecessary shadowing and binding.
**tests/unit_tests/mcp_service/chart/test_preview_utils.py**
```
def test_generate_preview_from_form_data_exposes_jinja_context(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""Unsaved-chart previews expose the same Jinja inputs as execution."""
from types import SimpleNamespace
from typing import Any
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174223110
##########
tests/unit_tests/mcp_service/chart/test_preview_utils.py:
##########
@@ -398,3 +398,80 @@ def test_vega_preview_y_axis_fallback_accepts_decimal() -> None:
)
assert encoding["y"]["field"] == "revenue"
+
+
+def test_generate_preview_from_form_data_exposes_jinja_context(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """Unsaved-chart previews expose the same Jinja inputs as execution."""
+ from types import SimpleNamespace
+ from typing import Any
+ from unittest.mock import MagicMock
Review Comment:
Fixed as well - dropped the redundant local MagicMock import; the module-level one is enough. Updated in latest commit.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5985135145
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4142154727
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
+ resolved = time_range
+ if resolved is None:
+ obj_range = getattr(query, "time_range", None)
+ if isinstance(obj_range, str):
+ resolved = obj_range
+ if resolved is not None:
+ query_data["time_range"] = resolved
if url_params := form_data.get("url_params"):
query_data["url_params"] = url_params
return query_data
def set_query_context_form_data(
query_context: QueryContext,
- datasource_id: int,
+ datasource_id: int | str,
datasource_type: str,
+ time_range: str | None = None,
) -> None:
"""Expose a programmatically-created query like a chart data API request."""
- form_data = query_context.form_data or {}
+ form_data = _as_form_data_dict(getattr(query_context, "form_data", None))
+ queries = _as_query_list(getattr(query_context, "queries", None))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Misleading QueryContext annotation</b></div>
<div id="fix">
Signature declares `QueryContext`, but `_as_form_data_dict(getattr(query_context, `form_data`, None))` accepts any object — tests pass `object()` (form_data_test.py:153). The nominal annotation misleads callers, and the `getattr` defaults turn a renamed `form_data`/`queries` field into silently empty Jinja context instead of a loud error. Prefer direct attribute access or a structural Protocol matching what is actually accepted.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a href="https://github.com/apache/superset/blob/bd6d88f/.cursor/rules/dev-standard.mdc#L28">dev-standard.mdc:28</a>
</li>
</ul>
</details>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead defensive branch for stubs</b></div>
<div id="fix">
`getattr(query, `to_dict`, None)` and `getattr(query, `filter`, None)` differ from direct access only for stubs; every production caller passes `QueryObject`, which defines both, so the `None` return and the `filters: None` publication are production-unreachable branches that also mask attribute renames as silently wrong Jinja form data. Consider direct access, handling stubs in tests.
</div>
</div>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/compile.py:
##########
@@ -94,6 +94,7 @@ def _compile_chart(
Returns a :class:`CompileResult` with ``success=True`` when the
query executes cleanly.
"""
+ from superset.charts.data.form_data import set_query_context_form_data
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inline import violates guideline</b></div>
<div id="fix">
This function-local import lacks the circular-dependency justification or explanatory comment that BITO.md rule 12745 requires for inline imports. Unlike the deferred `ChartDataCommand` imports beside it, `superset.charts.data.form_data` imports only `flask.g` at runtime, and sibling tools (`get_chart_data.py`, `get_chart_preview.py`, `get_chart_sql.py`) import it at module level. Move it to the module imports.
</div>
</div>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_preview_utils.py:
##########
@@ -398,3 +398,80 @@ def test_vega_preview_y_axis_fallback_accepts_decimal() -> None:
)
assert encoding["y"]["field"] == "revenue"
+
+
+def test_generate_preview_from_form_data_exposes_jinja_context(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """Unsaved-chart previews expose the same Jinja inputs as execution."""
+ from types import SimpleNamespace
+ from typing import Any
+ from unittest.mock import MagicMock
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant duplicate import</b></div>
<div id="fix">
`MagicMock` is already imported at module level (line 26: `from unittest.mock import MagicMock, Mock, patch`); re-importing it inside the test shadows the identical name and adds a second binding to keep in sync. The other function-local imports here (`SimpleNamespace`, `current_app`, `QueryObject`, the form_data_test helper) have no module-level counterpart, so only this one is redundant. Drop line 409.
</div>
</div>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/common/tabular_query.py:
##########
@@ -397,6 +402,28 @@ def build_query_dict(
return query_dict
+def _time_range_from_filters(filters: Any) -> str | None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Confusable helper names</b></div>
<div id="fix">
`_time_range_from_filters` (extracts a range from filters) and the pre-existing `_time_range_filters` (builds filters from a range) differ by one word while performing opposite transformations, and both sit in this module's time-filter flow: `build_query_dict` calls the latter, `execute_tabular_query` the former. A distinct name such as `_temporal_range_in_filters` would prevent mix-ups.
</div>
</div>
<small><i>Code Review Run #540efc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5434998370
Heya @AmoghAtreya, @gabotorresruiz left a really thorough review above with the exact fixes needed (test stub, pre-commit, rebase off `master`). Once those land and CI's green, this should be quick to get merged. Holler if you get stuck on any of it, happy to push some commits.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5705029432
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5612055870
@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 `get_chart_sql` and `get_chart_data` now prefer `query_context.datasource.id/type` over `chart.datasource_id`, matching what you asked for. Worth another look when you get a chance?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4020501045
##########
superset/charts/data/form_data.py:
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from typing import Any, TYPE_CHECKING
+
+from flask import g
+
+if TYPE_CHECKING:
+ from superset.common.query_context import QueryContext
+ from superset.common.query_object import QueryObject
+
+
+def set_form_data(form_data: dict[str, Any]) -> None:
+ """Expose chart data request fields to Jinja template macros."""
+ g.form_data = form_data
+
+
+def _serialize_query(
+ query: QueryObject,
+ form_data: dict[str, Any],
+) -> dict[str, Any]:
+ """Serialize query fields consumed by the Jinja form-data fallback."""
+ query_data = dict(query.to_dict())
+ query_data["filters"] = query.filter
+ if query.time_range is not None:
+ query_data["time_range"] = query.time_range
+ if url_params := form_data.get("url_params"):
+ query_data["url_params"] = url_params
+ return query_data
+
+
+def set_query_context_form_data(
+ query_context: QueryContext,
+ datasource_id: int | str,
+ datasource_type: str,
+) -> None:
+ """Expose a programmatically-created query like a chart data API request."""
+ form_data = query_context.form_data or {}
+ 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,
+ }
Review 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?
##########
superset/mcp_service/dataset/tool/query_dataset.py:
##########
@@ -298,8 +298,11 @@ async def query_dataset( # noqa: C901
# Step 4: Build query dict
# ------------------------------------------------------------------
await ctx.report_progress(3, 5, "Building query")
- # time_range is not passed through: the TEMPORAL_RANGE clause is already
- # in query_filters above, alongside the effective_filters bookkeeping.
+ # TEMPORAL_RANGE is already in query_filters. Do not also set
+ # QueryObject.time_range: relative ranges must keep from_dttm/to_dttm
+ # in the cache key so rollovers re-execute. Jinja get_time_filter()
+ # still sees the range via set_query_context_form_data hoisting it
+ # from the TEMPORAL_RANGE filter.
Review Comment:
First, the cache side of 5ca36d0 checks out, and I verified it in both directions on this branch: two explicit ranges produce different `QueryObject.cache_key()` values through the real `QueryContextFactory`, identical ranges share one key, and `test_query_dataset_reexecutes_across_rollover` fails at 3d17c49 and passes here. So the rollover regression from putting `time_range` on the `QueryObject` was real and this commit fixes it. Nice catch.
This comment block worries me a bit though: the claim that Jinja `get_time_filter()` still sees the range via the `set_query_context_form_data` hoist does not hold on this path. `QueryContextFactory.create()` runs `_apply_granularity`, and because `query_dataset` always sets `granularity` to the same column carrying the `TEMPORAL_RANGE` filter, that filter is deleted from `QueryObject.filter` before `execute_tabular_query` reaches `set_query_context_form_data`. I verified it on this branch: after the real factory runs, `queries[0].filter` is `[]`, `time_range` is `None`, and `ExtraCache().get_time_filter().time_range` renders `No filter` instead of the requested `Last week`. The same sequence at 3d17c49 rendered `Last week`.
The new tests pass because they never run `_process_query_object`: `test_query_context_form_data_hoists_time_range_from_temporal_filter` builds the `QueryObject` by hand with the filter still attached, and `test_query_dataset_exposes_filters_to_jinja_macros` patches `QueryContextFactory.create` with `_query_context_from_factory_kwargs`, which skips `_apply_granularity` entirely.
Not a blocker on its own, master never exposed the range to Jinja on this path either, so nothing regresses against master. But the comments here and in `build_query_dict` document a mechanism that does not fire, and Jinja alignment is the point of this PR. Since a rebase is needed anyway, could we make the hoist real in the same pass? `QueryContext` keeps the raw pre-processing query dicts in `cache_values["queries"]`, so `set_query_context_form_data` can fall back to those when the processed query lost the filter:
```python
raw_queries = (getattr(query_context, "cache_values", None) or {}).get("queries") or []
```
and hoist via `_time_range_from_filters(raw.get("filters"))` per query. Then turn `test_query_dataset_exposes_filters_to_jinja_macros` into a real factory test (patch `QueryContextFactory._convert_to_model` instead of `create`) so the assertion actually exercises `_apply_granularity`; that variant fails on the current head, which is exactly the coverage we are missing. Happy to dig in with you if it helps.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174206861
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
+ resolved = time_range
+ if resolved is None:
+ obj_range = getattr(query, "time_range", None)
+ if isinstance(obj_range, str):
+ resolved = obj_range
+ if resolved is not None:
+ query_data["time_range"] = resolved
if url_params := form_data.get("url_params"):
query_data["url_params"] = url_params
return query_data
def set_query_context_form_data(
query_context: QueryContext,
- datasource_id: int,
+ datasource_id: int | str,
datasource_type: str,
+ time_range: str | None = None,
) -> None:
"""Expose a programmatically-created query like a chart data API request."""
- form_data = query_context.form_data or {}
+ form_data = _as_form_data_dict(getattr(query_context, "form_data", None))
+ queries = _as_query_list(getattr(query_context, "queries", None))
Review Comment:
This was also an issue and resolved in the latest push — those `getattr` defaults only soft-failed for incomplete stubs. `QueryContext` defines `form_data` and `queries`, so the helper now reads them directly (`form_data` can still be `None`). Have pushed these changes.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5973361973
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4178979714
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val": "boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ columns=["region"],
+ metrics=["count"],
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {
+ "queries": [
+ {
+ "data": [{"region": "North"}],
+ "colnames": ["region"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ preview_module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ monkeypatch.setattr(
+ preview_module,
+ "build_query_context_from_form_data",
Review Comment:
<!-- Bito Reply -->
The suggestion to replace the untyped lambda with a typed nested function is appropriate and improves code readability and type safety in the test suite. Applying this change aligns with the project's coding standards for test helpers.
**tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py**
```
def _fake_build_query_context(*args: Any, **kwargs: Any) -> SimpleNamespace:
return query_context
# ...
monkeypatch.setattr(
preview_module,
"build_query_context_from_form_data",
_fake_build_query_context,
)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4178979079
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -556,6 +556,90 @@ def run(self) -> dict[str, Any]:
assert query["filters"] == [{"col": "gender", "op": "==", "val": "boy"}]
assert "adhoc_filters" not in query
+ def test_table_preview_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """Chart preview queries expose the same Jinja inputs as get_chart_data."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ columns=["region"],
+ metrics=["count"],
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ )
+ observed: dict[str, bool] = {}
+
+ class ChartDataCommand:
+ def __init__(self, qc: object) -> None:
+ self.query_context = qc
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {
+ "queries": [
+ {
+ "data": [{"region": "North"}],
+ "colnames": ["region"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ preview_module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ monkeypatch.setattr(
+ preview_module,
+ "build_query_context_from_form_data",
Review Comment:
Replaced the untyped lambda with a typed nested `_fake_build_query_context(*args: Any, **kwargs: Any) -> SimpleNamespace`. Pre-existing lambdas left as-is.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5647546583
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5770393584
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5656903025
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4018369834
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -185,6 +187,11 @@ def _sql_from_saved_query_context(
query_context = ChartDataQueryContextSchema().load(qc_json)
query_context.result_type = ChartDataResultType.QUERY
+ set_query_context_form_data(
+ query_context,
+ chart.datasource_id,
+ chart.datasource_type,
+ )
Review 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174198924
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
Review Comment:
Resolved this — those `getattr` branches were only for incomplete test stubs. Production always passes a `QueryObject`, so `_serialize_query` now uses `query.to_dict()`, `query.filter`, and `query.time_range` directly. Have pushed this change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4178987465
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -228,6 +231,133 @@ def test_null_chart_metadata(self):
assert output.datasource_name is None
+class TestChartSqlJinjaContext:
+ """SQL generation must expose the same Jinja inputs as chart data execution."""
+
+ def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ class ChartDataCommand:
+ def __init__(self, query_context: object) -> None:
+ self.query_context = query_context
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {"queries": [{"query": "SELECT 1", "language": "sql"}]}
+
+ return ChartDataCommand
+
+ def test_sql_from_form_data_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """The form_data SQL path publishes filters, time_range, and url_params."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ datasource=SimpleNamespace(id=7, type="table"),
+ )
Review Comment:
Annotated `get_data_command_module`, `query`, `query_context`, `chart`, and `result` in both Jinja SQL tests.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5906077950
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4020504291
##########
superset/charts/data/form_data.py:
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from typing import Any, TYPE_CHECKING
+
+from flask import g
+
+if TYPE_CHECKING:
+ from superset.common.query_context import QueryContext
+ from superset.common.query_object import QueryObject
+
+
+def set_form_data(form_data: dict[str, Any]) -> None:
+ """Expose chart data request fields to Jinja template macros."""
+ g.form_data = form_data
+
+
+def _serialize_query(
+ query: QueryObject,
+ form_data: dict[str, Any],
+) -> dict[str, Any]:
+ """Serialize query fields consumed by the Jinja form-data fallback."""
+ query_data = dict(query.to_dict())
+ query_data["filters"] = query.filter
+ if query.time_range is not None:
+ query_data["time_range"] = query.time_range
+ if url_params := form_data.get("url_params"):
+ query_data["url_params"] = url_params
+ return query_data
+
+
+def set_query_context_form_data(
+ query_context: QueryContext,
+ datasource_id: int | str,
+ datasource_type: str,
+) -> None:
+ """Expose a programmatically-created query like a chart data API request."""
+ form_data = query_context.form_data or {}
+ 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,
+ }
Review 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:
```python
query.filter == []
query.time_range is None
```
may 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5779932184
@gabotorresruiz I'm gonna click the Update branch button to see if that clears things. Please feel free to keep an eye on the result, or update branches when needed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5297133178
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43176?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `28.57143%` with `40 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.27%. Comparing base ([`1e65d93`](https://app.codecov.io/gh/apache/superset/commit/1e65d93a8367f6cbea1f66b3544cdea4548dcd3e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ff13ce0`](https://app.codecov.io/gh/apache/superset/commit/ff13ce07c520065e45a4192eaca5a365cdeb45cd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43176?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/charts/data/form\_data.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fcharts%2Fdata%2Fform_data.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2RhdGEvZm9ybV9kYXRhLnB5) | 43.75% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...perset/mcp\_service/chart/tool/get\_chart\_preview.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_preview.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfcHJldmlldy5weQ==) | 18.18% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/tool/get\_chart\_sql.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfc3FsLnB5) | 14.28% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/tool/get\_chart\_data.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_data.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfZGF0YS5weQ==) | 25.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/dataset/tool/query\_dataset.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdataset%2Ftool%2Fquery_dataset.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGF0YXNldC90b29sL3F1ZXJ5X2RhdGFzZXQucHk=) | 25.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...erset/mcp\_service/semantic\_layer/tool/get\_table.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fsemantic_layer%2Ftool%2Fget_table.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2Uvc2VtYW50aWNfbGF5ZXIvdG9vbC9nZXRfdGFibGUucHk=) | 25.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/compile.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fcompile.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvY29tcGlsZS5weQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/preview\_utils.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fpreview_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvcHJldmlld191dGlscy5weQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/tasks/async\_queries.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Ftasks%2Fasync_queries.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdGFza3MvYXN5bmNfcXVlcmllcy5weQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/charts/data/api.py](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&filepath=superset%2Fcharts%2Fdata%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2RhdGEvYXBpLnB5) | 75.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43176?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1e65d93) and HEAD (ff13ce0). Click for more details.
>
> <details><summary>HEAD has 7 uploads less than BASE</summary>
>
>| Flag | BASE (1e65d93) | HEAD (ff13ce0) |
>|------|------|------|
>|python|8|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43176 +/- ##
==========================================
- Coverage 66.70% 57.27% -9.43%
==========================================
Files 2872 2873 +1
Lines 163444 163474 +30
Branches 37725 37728 +3
==========================================
- Hits 109024 93630 -15394
- Misses 52291 68955 +16664
+ Partials 2129 889 -1240
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43176/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/43176/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43176/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43176/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43176/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.17% <28.57%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43176/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.17% <28.57%> (-19.19%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43176/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43176/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43176?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5595719522
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174220339
##########
superset/mcp_service/chart/compile.py:
##########
@@ -94,6 +94,7 @@ def _compile_chart(
Returns a :class:`CompileResult` with ``success=True`` when the
query executes cleanly.
"""
+ from superset.charts.data.form_data import set_query_context_form_data
Review Comment:
Resolved this as well — no circular dependency here, so moved `set_query_context_form_data` to the module imports to match the other chart tools.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4178988021
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -228,6 +231,133 @@ def test_null_chart_metadata(self):
assert output.datasource_name is None
+class TestChartSqlJinjaContext:
+ """SQL generation must expose the same Jinja inputs as chart data execution."""
+
+ def _command_observing_jinja(self, observed: dict[str, bool]) -> type:
+ from tests.unit_tests.charts.data.form_data_test import (
+ assert_request_dependent_jinja_macros,
+ )
+
+ class ChartDataCommand:
+ def __init__(self, query_context: object) -> None:
+ self.query_context = query_context
+
+ def validate(self) -> None:
+ pass
+
+ def run(self) -> dict[str, Any]:
+ assert_request_dependent_jinja_macros()
+ observed["ran"] = True
+ return {"queries": [{"query": "SELECT 1", "language": "sql"}]}
+
+ return ChartDataCommand
+
+ def test_sql_from_form_data_exposes_jinja_context(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
+ ) -> None:
+ """The form_data SQL path publishes filters, time_range, and url_params."""
+ from flask import current_app
+
+ from superset.common.query_object import QueryObject
+
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ query = QueryObject(
+ filters=[{"col": "region", "op": "IN", "val": ["North"]}],
+ time_range="Last week",
+ )
+ query_context = SimpleNamespace(
+ queries=[query],
+ form_data={"url_params": {"tenant": "acme"}},
+ datasource=SimpleNamespace(id=7, type="table"),
+ )
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to annotate local variables in the test file is a valid request to improve code maintainability and adhere to project standards. You should apply these type annotations to `get_data_command_module`, `query`, `query_context`, `chart`, and `result` as requested to ensure consistency with the project's typing requirements.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "AmoghAtreya (via GitHub)" <gi...@apache.org>.
AmoghAtreya commented on PR #43176:
URL: https://github.com/apache/superset/pull/43176#issuecomment-5972896710
@rusackas just rebased the branch and got it updated with master! Please let me know if there are any other changes to make.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): align Jinja context across query preview and compile paths … [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4174200043
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +31,69 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ time_range: str | None = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+
+ ``time_range`` is an optional overlay for callers that deliberately leave
+ ``QueryObject.time_range`` unset (tabular queries, so relative ranges keep
+ ``from_dttm``/``to_dttm`` in the cache key). Chart and async callers omit
+ it so ``get_time_filter()`` matches the chart-data API: a TEMPORAL_RANGE
+ filter alone is not a published time range.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
Review Comment:
<!-- Bito Reply -->
The change is appropriate as it removes defensive branching that was only necessary for test stubs, aligning the production code with the expected `QueryObject` interface. By using direct access to `to_dict()`, `filter`, and `time_range`, the implementation is now cleaner and avoids masking potential attribute errors.
**superset/charts/data/form_data.py**
```
query_data = dict(query.to_dict())
query_data["filters"] = query.filter
if query.time_range is not None:
query_data["time_range"] = query.time_range
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org