codeant-ai-for-open-source[bot] commented on code in PR #39865:
URL: https://github.com/apache/superset/pull/39865#discussion_r3198274865
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -162,6 +177,44 @@ def _resolve_engine(
return "base"
+def _build_single_query_dict(
+ form_data: dict[str, Any],
+ columns: list[Any],
+ metrics: list[Any],
+) -> dict[str, Any]:
+ """Build one query entry for QueryContextFactory from form_data fields."""
+ qd: dict[str, Any] = {"columns": columns, "metrics": metrics}
+ if time_range := form_data.get("time_range"):
+ qd["time_range"] = time_range
+ if filters := form_data.get("filters"):
+ qd["filters"] = filters
+ if (row_limit := form_data.get("row_limit")) is not None:
+ qd["row_limit"] = row_limit
Review Comment:
**Suggestion:** Query dict construction always reads `row_limit` and never
supports `row_limit_b`, so the secondary `mixed_timeseries` query cannot honor
its own row limit. This causes incorrect SQL LIMIT behavior for query B. Use
the secondary-specific limit when building the secondary query. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Secondary series row limits ignored in get_chart_sql output.
- ⚠️ SQL preview shows incorrect LIMIT for mixed_timeseries B.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a `mixed_timeseries` chart whose Explore form_data contains
both `row_limit`
and a different `row_limit_b` (per-series secondary row limit). Example
persisted
configuration appears in
`superset/examples/featured_charts/charts/Mixed.yaml:80-81` where
both `row_limit` and `row_limit_b` are defined.
2. Use the MCP `get_chart_sql` tool for this chart in a context that goes
through the
form_data fallback, e.g. request SQL for an unsaved Explore state via
`form_data_key` so
`_handle_unsaved_chart_sql`
(`superset/mcp_service/chart/tool/get_chart_sql.py:24-56`)
loads the cached form_data and calls `_sql_from_form_data(form_data,
chart=None)` at lines
39-56.
3. `_sql_from_form_data` invokes `_build_query_context_from_form_data`
(lines 218-305),
which builds the primary query via `_build_single_query_dict(form_data,
groupby, metrics)`
and the secondary mixed_timeseries query via
`_build_mixed_timeseries_secondary` (lines
196-215). Inside `_build_mixed_timeseries_secondary`, the secondary query
dict is
constructed with `qd = _build_single_query_dict(form_data, groupby_b,
metrics_b)` at line
212.
4. `_build_single_query_dict` (lines 180-193) always reads `row_limit` from
`form_data.get("row_limit")` and, if set, assigns `qd["row_limit"] =
row_limit` (lines
191-192). There is no handling of `row_limit_b`, so both the primary query
and secondary
query use the same `row_limit` value. When `ChartDataCommand` executes this
`QueryContext`
(`_sql_from_form_data` lines 44-49), the generated SQL for series B uses the
primary row
limit instead of the configured `row_limit_b`, making the SQL preview
inconsistent with
the chart configuration.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py%0A%2A%2ALine%3A%2A%2A%20191%3A192%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Query%20dict%20construction%20always%20reads%20%60row_limit%60%20and%20never%20supports%20%60row_limit_b%60%2C%20so%20the%20secondary%20%60mixed_timeseries%60%20query%20cannot%20honor%20its%20own%20row%20limit.%20This%20causes%20incorrect%20SQL%20LIMIT%20behavior%20for%20query%20B.%20Use%20the%20secondary-specific%20limit%20when%20building%20the%20secondary%20query.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%
20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py%0A%2A%2ALine%3A%2A%2A%20191%3A192%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Query%20dict%20construction%20always%20reads%20%60row_limit%60%20and%20never%20supports%20%60row_limit_b%60%2C%20so%20the%20secondary%20%60mixed_timeseries%60%20query%20cannot%20honor%20its%20own%20row%20limit.%20This%20causes%20incorrect%20SQL%20LIMIT%20behavior%20for%20query%20B.%20Use%20the%20secondary-specific%20limit%20when%20building%20the%20secondary%20query.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20
resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 191:192
**Comment:**
*Logic Error: Query dict construction always reads `row_limit` and
never supports `row_limit_b`, so the secondary `mixed_timeseries` query cannot
honor its own row limit. This causes incorrect SQL LIMIT behavior for query B.
Use the secondary-specific limit when building the secondary query.
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%2F39865&comment_hash=13ee868e08f3c4c1c9d8e6726c08e07901fb90457a1bc8ee935fd7125496e994&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39865&comment_hash=13ee868e08f3c4c1c9d8e6726c08e07901fb90457a1bc8ee935fd7125496e994&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -162,6 +177,44 @@ def _resolve_engine(
return "base"
+def _build_single_query_dict(
+ form_data: dict[str, Any],
+ columns: list[Any],
+ metrics: list[Any],
+) -> dict[str, Any]:
+ """Build one query entry for QueryContextFactory from form_data fields."""
+ qd: dict[str, Any] = {"columns": columns, "metrics": metrics}
+ if time_range := form_data.get("time_range"):
+ qd["time_range"] = time_range
+ if filters := form_data.get("filters"):
+ qd["filters"] = filters
+ if (row_limit := form_data.get("row_limit")) is not None:
+ qd["row_limit"] = row_limit
+ return qd
+
+
+def _build_mixed_timeseries_secondary(
+ form_data: dict[str, Any],
+ x_axis_col: str | None,
+) -> dict[str, Any]:
+ """Build the secondary query dict for the ``mixed_timeseries`` viz type.
+
+ ``mixed_timeseries`` has two independent series layers; the secondary
+ layer uses ``metrics_b`` / ``groupby_b`` instead of the primary fields.
+ If ``time_range_b`` is set it overrides the primary ``time_range`` for
+ the secondary query.
+ """
+ metrics_b: list[Any] = list(form_data.get("metrics_b") or [])
+ raw_b = form_data.get("groupby_b") or []
+ groupby_b: list[Any] = [raw_b] if isinstance(raw_b, str) else list(raw_b)
+ if x_axis_col and x_axis_col not in groupby_b:
+ groupby_b = [x_axis_col] + groupby_b
+ qd = _build_single_query_dict(form_data, groupby_b, metrics_b)
Review Comment:
**Suggestion:** The secondary query for `mixed_timeseries` is built from the
original `form_data` without translating `_b` controls to base keys, so
secondary-only filters (for example `adhoc_filters_b` after preprocessing) are
never applied. This makes query B SQL use primary filter state and can return
incorrect results. Build a secondary form-data view (equivalent to
`retainFormDataSuffix(..., "_b")`) before constructing query B. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ get_chart_sql misrepresents mixed_timeseries secondary series filters.
- ⚠️ Unsaved mixed_timeseries SQL omits secondary adhoc_filters_b.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or edit a `mixed_timeseries` chart in Explore so its form_data (as
stored in
`Slice.params` / cached via `GetFormDataCommand`) contains both primary and
secondary
series, including `metrics`, `groupby`, `metrics_b`, `groupby_b`, and a
secondary-only
adhoc filter in `adhoc_filters_b`. This matches how mixed charts are
migrated/built in
`superset/migrations/shared/migrate_viz/processors.py:17-28` where
`adhoc_filters_b` is
populated from `adhoc_filters`.
2. From an MCP client, request SQL for this chart using only `form_data_key`
(unsaved
state), triggering `_handle_unsaved_chart_sql` at
`superset/mcp_service/chart/tool/get_chart_sql.py:24-56`, which loads the
cached form_data
and calls `_sql_from_form_data(form_data, chart=None)` at lines 39-56.
3. `_sql_from_form_data` calls `_build_query_context_from_form_data` (lines
218-305).
Inside that function, adhoc filters are preprocessed by
`merge_extra_filters` and
`split_adhoc_filters_into_base_filters` at lines 250-263, but
`split_adhoc_filters_into_base_filters` (`superset/utils/core.py:1387-1400`)
only reads
the `adhoc_filters` key, never `adhoc_filters_b`, so secondary-only adhoc
filters remain
unprocessed on `form_data` and are not converted into
`filters`/`where`/`having`.
4. Still in `_build_query_context_from_form_data`, the secondary query dict
for
mixed_timeseries is built by `_build_mixed_timeseries_secondary` (lines
196-215), which
calls `qd = _build_single_query_dict(form_data, groupby_b, metrics_b)` at
line 212.
`_build_single_query_dict` (lines 180-193) pulls `filters` and `time_range`
from the
shared `form_data`, so the secondary query's `qd["filters"]` is based only
on primary
`adhoc_filters` that were split into base filters, while `adhoc_filters_b`
is never
applied. When `ChartDataCommand` later runs on this `QueryContext`
(`_sql_from_form_data`
lines 44-49), the rendered SQL for query B ignores its intended
secondary-only filters.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py%0A%2A%2ALine%3A%2A%2A%20212%3A212%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20The%20secondary%20query%20for%20%60mixed_timeseries%60%20is%20built%20from%20the%20original%20%60form_data%60%20without%20translating%20%60_b%60%20controls%20to%20base%20keys%2C%20so%20secondary-only%20filters%20%28for%20example%20%60adhoc_filters_b%60%20after%20preprocessing%29%20are%20never%20applied.%20This%20makes%20query%20B%20SQL%20use%20primary%20filter%20state%20and%20can%20return%20incorrect%20results.%20Build%20a%20secondary%20form-data%20view%20%28equivalent%20to%20%60retainFormDataSuffix%28...%2C%20%22_b%22%29%60%29%20before%20constructing%20query%20B.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%2
0you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py%0A%2A%2ALine%3A%2A%2A%20212%3A212%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20The%20secondary%20query%20for%20%60mixed_timeseries%60%20is%20built%20from%20the%20original%20%60form_data%60%20without%20translating%20%60_b%60%20controls%20to%20base%20keys%2C%20so%20secondary-only%20filters%20%28for%20example%20%60adhoc_filters_b%60%20after%20prep
rocessing%29%20are%20never%20applied.%20This%20makes%20query%20B%20SQL%20use%20primary%20filter%20state%20and%20can%20return%20incorrect%20results.%20Build%20a%20secondary%20form-data%20view%20%28equivalent%20to%20%60retainFormDataSuffix%28...%2C%20%22_b%22%29%60%29%20before%20constructing%20query%20B.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 212:212
**Comment:**
*Logic Error: The secondary query for `mixed_timeseries` is built from
the original `form_data` without translating `_b` controls to base keys, so
secondary-only filters (for example `adhoc_filters_b` after preprocessing) are
never applied. This makes query B SQL use primary filter state and can return
incorrect results. Build a secondary form-data view (equivalent to
`retainFormDataSuffix(..., "_b")`) before constructing query B.
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%2F39865&comment_hash=8ad0b3b2c094b5d4c8307afd2dda001c0919cbca58cb0e31946c74fb6b36321b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39865&comment_hash=8ad0b3b2c094b5d4c8307afd2dda001c0919cbca58cb0e31946c74fb6b36321b&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
