fix(mcp): include x_axis column in query context for series charts with group_by #37639
+482
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…th group_by When generating chart previews and data for XY charts with a group_by dimension, the query context only included groupby columns but omitted the x_axis column. This caused series charts to lose their x-axis dimension, producing incorrect results (e.g., only showing the groupby breakdown without the x-axis categories). Fixed in 4 locations: - TablePreviewStrategy in get_chart_preview.py - VegaLitePreviewStrategy in get_chart_preview.py - Fallback query context in get_chart_data.py - generate_preview_from_form_data in preview_utils.py The fix follows the pattern already used by ASCIIPreviewStrategy, which correctly included both x_axis and groupby columns.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #37639 +/- ##
===========================================
+ Coverage 0 66.30% +66.30%
===========================================
Files 0 646 +646
Lines 0 49459 +49459
Branches 0 5548 +5548
===========================================
+ Hits 0 32795 +32795
- Misses 0 15364 +15364
- Partials 0 1300 +1300
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Renamed the query columns variable from 'columns' to 'query_columns' to avoid shadowing the later 'columns' variable which holds list[DataColumn], causing a mypy attr-defined error.
Add extra="forbid" to XYChartConfig and TableChartConfig so that unknown fields like "series" cause a validation error instead of being silently ignored. LLMs were sending "series" instead of "group_by", which was silently dropped, resulting in charts without the groupby dimension. With this change, the LLM gets a clear error message telling it to use the correct field name.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #bc15b0
Actionable Suggestions - 1
-
tests/unit_tests/mcp_service/chart/test_preview_utils.py - 1
- Incorrect Column Logic in Tests · Line 83-90
Additional Suggestions - 6
-
superset/mcp_service/chart/preview_utils.py - 2
-
Logic Bug in Column Selection · Line 73-73The logic for building the columns list incorrectly treats an empty "columns" array in form_data as falsy, causing it to use groupby_columns instead. This could lead to unexpected query results for table charts with explicitly empty columns. The fix ensures that if "columns" is present in form_data, its value is used regardless of emptiness.
Code suggestion
@@ -73,1 +73,1 @@ - columns = raw_columns.copy() if raw_columns else groupby_columns.copy() + columns = raw_columns.copy() if "columns" in form_data else groupby_columns.copy()
-
Missing Type Hints · Line 69-80The newly added code lacks type hints, which are required for all new Python code per development standards. Adding proper typing improves maintainability and enables better static analysis.
-
-
tests/unit_tests/mcp_service/chart/test_preview_utils.py - 2
-
Tests Don't Assert Actual Behavior · Line 23-146The tests replicate the column building logic instead of testing the actual behavior of generate_preview_from_form_data, violating the rule to assert business logic directly. This duplication can miss bugs in the function itself.
-
Missing Test Coverage & Duplication · Line 30-146The test suite lacks coverage for edge cases like empty 'columns' lists in table charts and x_axis dict duplication in XY charts. Adding these tests ensures robustness. The repeated column building code across methods creates maintainability issues; consider a helper function to avoid duplication.
-
-
superset/mcp_service/chart/tool/get_chart_preview.py - 2
-
Missing Type Hints · Line 189-192The new variables lack type hints, which are required for new Python code per development standards. Consider adding annotations like `x_axis_config: str | dict | None = form_data.get("x_axis")` to ensure type safety.
-
Code Duplication · Line 188-200The column-building logic is duplicated between TablePreviewGenerator.generate and VegaLitePreviewGenerator.generate. Consider extracting it into a private method like `_build_columns_list` to improve maintainability.
-
Review Details
-
Files reviewed - 6 · Commit Range:
0352eca..0352eca- superset/mcp_service/chart/preview_utils.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_preview.py
- tests/unit_tests/mcp_service/chart/test_preview_utils.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Comment on lines
83
to
90
| columns = raw_columns.copy() if raw_columns else groupby_columns.copy() | ||
| if x_axis_config and isinstance(x_axis_config, str): | ||
| if x_axis_config not in columns: | ||
| columns.insert(0, x_axis_config) | ||
| elif x_axis_config and isinstance(x_axis_config, dict): | ||
| col_name = x_axis_config.get("column_name") | ||
| if col_name and col_name not in columns: | ||
| columns.insert(0, col_name) |
Contributor
There was a problem hiding this comment.
Incorrect Column Logic in Tests
The column building logic in the test methods incorrectly treats empty 'columns' lists the same as missing 'columns', which can lead to wrong column selection for table charts with empty columns. The logic should distinguish table charts (use 'columns' if present) from XY charts (use groupby + x_axis). This matches the function's docstring and prevents bugs like using groupby for table charts with empty columns. The implementation in preview_utils.py has the same issue and should be fixed similarly.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| columns = raw_columns.copy() if raw_columns else groupby_columns.copy() | |
| if x_axis_config and isinstance(x_axis_config, str): | |
| if x_axis_config not in columns: | |
| columns.insert(0, x_axis_config) | |
| elif x_axis_config and isinstance(x_axis_config, dict): | |
| col_name = x_axis_config.get("column_name") | |
| if col_name and col_name not in columns: | |
| columns.insert(0, col_name) | |
| if "columns" in form_data: | |
| columns = raw_columns.copy() | |
| else: | |
| columns = groupby_columns.copy() | |
| if x_axis_config and isinstance(x_axis_config, str): | |
| if x_axis_config not in columns: | |
| columns.insert(0, x_axis_config) | |
| elif x_axis_config and isinstance(x_axis_config, dict): | |
| col_name = x_axis_config.get("column_name") | |
| if col_name and col_name not in columns: | |
| columns.insert(0, col_name) |
Code Review Run #bc15b0
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
kgabryje
reviewed
Feb 3, 2026
| group_by: ColumnRef | None = Field( | ||
| None, | ||
| description="Column to group by (creates series/breakdown). " | ||
| "Use this field for series grouping — do NOT use 'series'.", |
Member
There was a problem hiding this comment.
I think that's good enough for now since we support only a few chart types, but FYI in case we add support for more in the future - some viz plugins (e.g. bubble, word cloud) DO have a 'series' field in their form data
Contributor
Author
There was a problem hiding this comment.
Good to know, thanks for the heads up. We can revisit this when we add support for those viz types — at that point we'd likely need separate config schemas for bubble/word cloud anyway since they have different field requirements.
Use "columns" in form_data instead of truthiness check on the value, so that an explicitly empty columns list is respected rather than falling back to groupby columns.
Contributor
Code Review Agent Run #d94d67Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…ignores Extract inline column-building logic in VegaLitePreviewStrategy.generate() into shared _build_query_columns() helper to fix C901 complexity violation. Remove unnecessary type: ignore comments from test file.
Contributor
Code Review Agent Run #f37ea9Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Member
|
resolved the bot comments that have been responded to... a little pre-commit and we'll be there! |
… C901 complexity Move column-building logic into a shared helper function to reduce generate_preview_from_form_data complexity below the C901 threshold.
| def _build_query_columns(form_data: Dict[str, Any]) -> list[str]: | ||
| """Build query columns list from form_data, including both x_axis and groupby.""" | ||
| x_axis_config = form_data.get("x_axis") | ||
| groupby_columns: list[str] = form_data.get("groupby", []) |
Contributor
There was a problem hiding this comment.
Suggestion: When form_data["groupby"] is present but set to None (or any non-list value), assigning it directly to groupby_columns and then calling .copy() later will raise an AttributeError, causing preview generation to fail instead of gracefully treating it as no groupby columns. [possible bug]
Severity Level: Major ⚠️
- ❌ generate_preview_from_form_data preview fails (AttributeError)
- ⚠️ LLM/form_data with null groupby triggers failure
Suggested change
| groupby_columns: list[str] = form_data.get("groupby", []) | |
| groupby_columns = form_data.get("groupby") or [] | |
| if not isinstance(groupby_columns, list): | |
| groupby_columns = [] |
Steps of Reproduction ✅
1. Call generate_preview_from_form_data(...) in
superset/mcp_service/chart/preview_utils.py (the function defined at the top of the file;
its usage of _build_query_columns is at line 86 in the PR hunk).
2. Pass form_data containing "groupby": null (JSON null -> Python None). Example payload:
form_data = {"viz_type": "echarts_timeseries_bar", "x_axis": "territory", "groupby":
None}. The call site that triggers this is the line: columns =
_build_query_columns(form_data) at line 86.
3. Execution enters _build_query_columns (defined in the same file at lines 39-53). Line
42 assigns groupby_columns = form_data.get("groupby", []) which returns None (because the
key exists with null).
4. Later, at line 45 (columns = raw_columns.copy() if "columns" in form_data else
groupby_columns.copy()), Python attempts groupby_columns.copy() and raises AttributeError:
'NoneType' object has no attribute 'copy'. The preview generation path blows up and
returns a ChartError fallback. This reproduces deterministically when "groupby" is present
and null.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/preview_utils.py
**Line:** 42:42
**Comment:**
*Possible Bug: When `form_data["groupby"]` is present but set to `None` (or any non-list value), assigning it directly to `groupby_columns` and then calling `.copy()` later will raise an `AttributeError`, causing preview generation to fail instead of gracefully treating it as no groupby columns.
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.| """Build query columns list from form_data, including both x_axis and groupby.""" | ||
| x_axis_config = form_data.get("x_axis") | ||
| groupby_columns: list[str] = form_data.get("groupby", []) | ||
| raw_columns: list[str] = form_data.get("columns", []) |
Contributor
There was a problem hiding this comment.
Suggestion: If form_data["columns"] exists but is None (or another non-list type), assigning it directly to raw_columns and then calling .copy() will raise an AttributeError, breaking previews for such requests instead of safely falling back to an empty list. [possible bug]
Severity Level: Major ⚠️
- ❌ Preview generation errors when "columns" key is null
- ⚠️ Table and XY previews may be blocked intermittently
Suggested change
| raw_columns: list[str] = form_data.get("columns", []) | |
| raw_columns = form_data.get("columns") or [] | |
| if not isinstance(raw_columns, list): | |
| raw_columns = [] |
Steps of Reproduction ✅
1. Invoke generate_preview_from_form_data(form_data, dataset_id, preview_format) in
superset/mcp_service/chart/preview_utils.py (caller uses _build_query_columns at line 86).
2. Provide form_data that contains the "columns" key with a null or non-list value, e.g.
form_data = {"viz_type": "table", "columns": None}. The presence of the key is important.
3. Inside _build_query_columns (lines 39-53), line 43 sets raw_columns =
form_data.get("columns", []) which yields None. Because "columns" key exists, the later
expression at line 45 uses raw_columns.copy() and attempts to call .copy() on None.
4. This causes AttributeError: 'NoneType' object has no attribute 'copy', failing preview
creation along the same path as ChartDataCommand execution. This reproduces whenever a
request includes "columns": null.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/preview_utils.py
**Line:** 43:43
**Comment:**
*Possible Bug: If `form_data["columns"]` exists but is `None` (or another non-list type), assigning it directly to `raw_columns` and then calling `.copy()` will raise an `AttributeError`, breaking previews for such requests instead of safely falling back to an empty list.
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.
Comment on lines
+51
to
+52
| if col_name and col_name not in columns: | ||
| columns.insert(0, col_name) |
Contributor
There was a problem hiding this comment.
Suggestion: When the x-axis is configured as an ad-hoc column (a dict without column_name but with sqlExpression), the current logic ignores it entirely, so the generated query columns omit the x-axis dimension and the preview query may not match the chart configuration; adding a fallback to include the dict itself when column_name is missing fixes this. [logic error]
Severity Level: Major ⚠️
- ❌ x_axis omitted from preview query for ad-hoc expressions
- ⚠️ Vega-Lite/table previews mismatch when using SQL expressions
Suggested change
| if col_name and col_name not in columns: | |
| columns.insert(0, col_name) | |
| if col_name: | |
| if col_name not in columns: | |
| columns.insert(0, col_name) | |
| elif "sqlExpression" in x_axis_config and x_axis_config not in columns: | |
| columns.insert(0, x_axis_config) |
Steps of Reproduction ✅
1. Call generate_preview_from_form_data(form_data, dataset_id, preview_format) in
superset/mcp_service/chart/preview_utils.py (the call to _build_query_columns is at line
86).
2. Provide form_data where x_axis is an ad-hoc column dict without "column_name" but with
a SQL expression, e.g. form_data["x_axis"] = {"sqlExpression": "YEAR(date)"} (this format
is used by ad-hoc columns).
3. _build_query_columns (lines 39-53) hits the dict branch at line 49, extracts col_name
(line 50) which is None, and because no fallback exists the ad-hoc dict is ignored. The
resulting columns list does not include the x_axis expression.
4. The QueryContext created from these columns therefore omits the x-axis expression from
SELECT and GROUP BY. The preview (via ChartDataCommand and subsequent preview generators)
shows missing or incorrect x-axis, reproducing for ad-hoc x_axis usages.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/preview_utils.py
**Line:** 51:52
**Comment:**
*Logic Error: When the x-axis is configured as an ad-hoc column (a dict without `column_name` but with `sqlExpression`), the current logic ignores it entirely, so the generated query columns omit the x-axis dimension and the preview query may not match the chart configuration; adding a fallback to include the dict itself when `column_name` is missing fixes this.
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.|
|
||
| # Build query columns list: include both x_axis and groupby | ||
| x_axis_config = form_data.get("x_axis") | ||
| query_columns = groupby_columns.copy() |
Contributor
There was a problem hiding this comment.
Suggestion: The code assumes that the groupby value from form_data is always a list and calls .copy() on it directly; if an existing chart was saved with groupby as null or a scalar (string), this will raise an AttributeError at runtime in the fallback path, preventing chart data retrieval. [type error]
Severity Level: Major ⚠️
- ❌ get_chart_data MCP tool crashes on fallback charts.
- ⚠️ Chart data previews fail for older unsaved charts.
- ⚠️ Series/group_by chart previews may error inconsistently.
Suggested change
| query_columns = groupby_columns.copy() | |
| if isinstance(groupby_columns, list): | |
| query_columns = groupby_columns.copy() | |
| elif groupby_columns: | |
| # Handle older or malformed form_data where groupby is a scalar | |
| query_columns = [groupby_columns] | |
| else: | |
| query_columns = [] |
Steps of Reproduction ✅
1. Create or find a chart record where chart.query_context is NULL and chart.params
contains form_data with `"groupby": null` or `"groupby": "year"`. This falls into the
fallback path in get_chart_data (file superset/mcp_service/chart/tool/get_chart_data.py).
The fallback form_data is loaded at the line `form_data = utils_json.loads(chart.params)
if chart.params else {}` (the fallback block around lines ~172-180).
2. Invoke the MCP data tool for that chart by calling the get_chart_data tool function
(superset/mcp_service/chart/tool/get_chart_data.py:def get_chart_data). Call it with
request.identifier set to the chart's id so the code loads that chart (the ChartDAO lookup
happens earlier in the same function).
3. Execution reaches the fallback branch where groupby is read with `groupby_columns =
form_data.get("groupby", [])` (line 179). If the saved form_data had `groupby: null` (->
None) or `groupby: "year"` (-> str), groupby_columns becomes None or a string.
4. The code then executes `query_columns = groupby_columns.copy()` (line 183). This raises
an AttributeError ('NoneType' object has no attribute 'copy' or 'str' object has no
attribute 'copy'), causing the get_chart_data invocation to fail and return an error from
the inner except block that logs "Error retrieving chart data" and returns a ChartError.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_data.py
**Line:** 183:183
**Comment:**
*Type Error: The code assumes that the `groupby` value from `form_data` is always a list and calls `.copy()` on it directly; if an existing chart was saved with `groupby` as `null` or a scalar (string), this will raise an `AttributeError` at runtime in the fallback path, preventing chart data retrieval.
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.| def _build_query_columns(form_data: Dict[str, Any]) -> list[str]: | ||
| """Build query columns list from form_data, including both x_axis and groupby.""" | ||
| x_axis_config = form_data.get("x_axis") | ||
| groupby_columns: list[str] = form_data.get("groupby", []) |
Contributor
There was a problem hiding this comment.
Suggestion: _build_query_columns assumes that form_data["groupby"] is always a list, but if it is explicitly None or a scalar (e.g. a single string), calling .copy() will raise an AttributeError or produce an incorrect type, breaking table/vega previews for such charts. Normalizing the value to a list before copying avoids this runtime failure. [type error]
Severity Level: Major ⚠️
- ❌ Table preview generation may raise exceptions.
- ❌ Vega-Lite preview queries can be aborted.
- ⚠️ get_chart_preview tool returns ChartError instead.
Suggested change
| groupby_columns: list[str] = form_data.get("groupby", []) | |
| groupby_columns = form_data.get("groupby") or [] | |
| if not isinstance(groupby_columns, list): | |
| groupby_columns = [groupby_columns] |
Steps of Reproduction ✅
1. Load a chart preview using get_chart_preview tool at
superset/mcp_service/chart/tool/get_chart_preview.py: async
_get_chart_preview_internal(...) which eventually instantiates PreviewFormatGenerator and
calls a strategy (TablePreviewStrategy or VegaLitePreviewStrategy).
2. The TablePreviewStrategy.generate path calls _build_query_columns(form_data) (callsite
added at get_chart_preview.py:301 in the PR) passing form_data parsed from chart.params
(from ChartDAO or transient chart).
3. If the stored chart.params contains "groupby": null or "groupby": "year" (a scalar) — a
realistic payload when form_data JSON comes from external LLMs or older saved exports —
_build_query_columns executes groupby_columns: list[str] = form_data.get("groupby", [])
then columns = groupby_columns.copy() at get_chart_preview.py:64.
4. When groupby_columns is None, None.copy() raises AttributeError; when groupby_columns
is a string, .copy() is a method error (strings don't have copy), causing the preview path
to raise and the Table/Vega preview to fail. The failure appears in logs from
get_chart_preview and results in ChartError returned to the caller.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 62:62
**Comment:**
*Type Error: `_build_query_columns` assumes that `form_data["groupby"]` is always a list, but if it is explicitly `None` or a scalar (e.g. a single string), calling `.copy()` will raise an AttributeError or produce an incorrect type, breaking table/vega previews for such charts. Normalizing the value to a list before copying avoids this runtime failure.
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.
Comment on lines
+69
to
+71
| col_name = x_axis_config.get("column_name") | ||
| if col_name and col_name not in columns: | ||
| columns.insert(0, col_name) |
Contributor
There was a problem hiding this comment.
Suggestion: When x_axis is provided as a dict (for example, an ad‑hoc or SQL expression column configuration), _build_query_columns currently extracts only column_name and drops the rest of the configuration; if column_name is missing this silently omits the x‑axis from the query, so previews for such charts won't include the intended x‑axis dimension. Using the entire dict as the column specification ensures ad‑hoc x‑axis definitions are preserved in the query. [logic error]
Severity Level: Critical 🚨
- ❌ Vega-Lite previews missing ad-hoc x-axis dimension.
- ❌ Table previews omit intended x-axis column.
- ⚠️ Preview SQL GROUP BY incorrect for ad-hoc x_axis.
Suggested change
| col_name = x_axis_config.get("column_name") | |
| if col_name and col_name not in columns: | |
| columns.insert(0, col_name) | |
| if x_axis_config not in columns: | |
| columns.insert(0, x_axis_config) |
Steps of Reproduction ✅
1. Create or have a chart whose form_data.x_axis is an ad-hoc or SQL expression dict (for
example: {"sqlExpression": "DATE_TRUNC('year', date)", "label": "year"}) stored in
chart.params (ChartDAO result used in VegaLitePreviewStrategy or transient chart creation
in _get_chart_preview_internal).
2. Call get_chart_preview (superset/mcp_service/chart/tool/get_chart_preview.py), which
selects VegaLitePreviewStrategy.generate or TablePreviewStrategy.generate and calls
columns = _build_query_columns(form_data) (callsites in get_chart_preview.py:204 and
get_chart_preview.py:301 in the PR).
3. Inside _build_query_columns (defined at get_chart_preview.py:59), the dict
x_axis_config path looks only for "column_name" (get("column_name")) and if absent does
not add any column; the ad-hoc dict therefore is silently omitted from the columns list.
4. QueryContextFactory.create (superset/common/query_context_factory.py:create at ~line
39) receives a queries[0]["columns"] lacking the x_axis dict, so the generated SQL and
GROUP BY omit the intended x-axis expression, producing incorrect preview results (missing
dimension in SELECT/GROUP BY) visible in Table/Vega previews.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 69:71
**Comment:**
*Logic Error: When `x_axis` is provided as a dict (for example, an ad‑hoc or SQL expression column configuration), `_build_query_columns` currently extracts only `column_name` and drops the rest of the configuration; if `column_name` is missing this silently omits the x‑axis from the query, so previews for such charts won't include the intended x‑axis dimension. Using the entire dict as the column specification ensures ad‑hoc x‑axis definitions are preserved in the 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.
Contributor
Code Review Agent Run #b7c615Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Two bugs in the MCP service prevented series charts with
group_byfrom working:Bug 1:
XYChartConfigsilently ignored unknown fields. LLMs frequently sent"series"instead of"group_by", which was silently dropped by Pydantic (default behavior). The chart was created without any groupby dimension. Fixed by addingextra="forbid"to bothXYChartConfigandTableChartConfig, so unknown fields likeseriesnow return a clear validation error that tells the LLM to use the correct field name (group_by).Bug 2: Query context omitted the
x_axiscolumn. When building SQL queries for chart previews and data retrieval, onlygroupbycolumns were included in the query'scolumnslist — thex_axiscolumn was missing. This produced queries likeSELECT year, SUM(sales) FROM ... GROUP BY yearinstead ofSELECT territory, year, SUM(sales) FROM ... GROUP BY territory, year. Fixed in 4 locations:TablePreviewStrategyinget_chart_preview.pyVegaLitePreviewStrategyinget_chart_preview.pyget_chart_data.pygenerate_preview_from_form_datainpreview_utils.pyThe fix follows the pattern already used by
ASCIIPreviewStrategy, which correctly included both columns.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (Bug 1): LLM sends
"series": {"name": "year"}→ silently dropped, no groupby in form_dataAfter (Bug 1): LLM sends
"series"→ gets validation error → retries with"group_by"→ worksBefore (Bug 2): Query context missing x_axis column
After (Bug 2): Query context includes both x_axis and groupby
TESTING INSTRUCTIONS
territoryyearSUM(sales)"series"instead of"group_by"returns a validation errorpytest tests/unit_tests/mcp_service/chart/ -vADDITIONAL INFORMATION