You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/01/07 04:32:02 UTC
[PR] fix(mcp): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
aminghadersohi opened a new pull request, #36937:
URL: https://github.com/apache/superset/pull/36937
## Summary
- **Fixed `ctx` error**: Added missing `@parse_request(GetChartDataRequest)` decorator to `get_chart_data` tool, which was causing the error: `missing 1 required positional argument: 'ctx'` when calling from MCP clients
- **Added raw data option**: New `include_raw_data` parameter that retrieves all columns from the underlying dataset instead of the chart's aggregated view, solving the issue where charts like `big_number` return only a single aggregated row
## Test plan
- [ ] Test `get_chart_data` tool via MCP client to verify no `ctx` error
- [ ] Test `get_chart_data` with `include_raw_data=true` to get full dataset rows
- [ ] Test `get_chart_data` with `include_raw_data=false` (default) to get chart's normal aggregated view
## Additional Information
Reported issues:
1. `get_chart_data` was returning: `missing 1 required positional argument: 'ctx'`
2. Users were getting "limited data" / "single aggregated values" instead of full data
Root causes:
1. The `@parse_request` decorator was missing (other tools like `get_chart_info` have it) - this decorator handles FastMCP's context injection
2. The query was using the chart's `form_data` which includes aggregations (`metrics`), causing charts like `big_number` to return only one aggregated row
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3717279933
CodeAnt AI is reviewing your PR.
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3717312390
## Addressing Review Feedback
### β
Fixed: Unbounded row fetch risk
Added `MAX_ROW_LIMIT = 10000` constant and capped all row limit calculations using `min(request.limit or 100, MAX_ROW_LIMIT)`. This prevents excessive memory/DB load when clients request large datasets.
### β
Added: Unit tests for new feature
Added comprehensive unit tests in `tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py` covering:
- `GetChartDataRequest` schema validation including the new `include_raw_data` field
- `MAX_ROW_LIMIT` constant and row limit capping logic
- All 13 tests pass
### βΉοΈ Re: Backward-incompatible schema change (False Positive)
The CodeAnt review flagged `DataColumn` model fields as potentially backward-incompatible, but this is **not related to this PR**. The `DataColumn` schema was not modified in this PR - it's pre-existing code. My changes only touched:
1. `GetChartDataRequest` schema - added new optional field `include_raw_data` with default `False` (backward compatible)
2. `get_chart_data` function - added `@parse_request` decorator and raw data logic
The flagged lines (R1117-R1130) are far outside my diff which only modified lines 978-985 in schemas.py.
--
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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3719129439
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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): use chart.query_context for get_chart_data like the API does [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3719990162
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 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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3719130145
## Addressed Vitor's Feedback
Thanks @vitor.avila for the detailed feedback! You were absolutely right.
### The Problem
The original implementation was manually constructing a query from `form_data` parts (`groupby`, `metrics`, `filters`), which doesn't produce the same results as the chart visualization. For a line chart with annual aggregation, this was returning aggregated totals instead of the actual data points.
### The Fix
Now `get_chart_data` uses **`chart.query_context`** (the saved query context) exactly like the `/api/v1/chart/{id}/data` endpoint does:
```python
# Before (wrong approach):
query_spec = {
"columns": form_data.get("groupby", []),
"metrics": form_data.get("metrics", []),
...
}
# After (correct approach - same as API):
query_context_json = json.loads(chart.query_context)
query_context = ChartDataQueryContextSchema().load(query_context_json)
```
The `query_context` contains all the information needed to reproduce the chart's data exactly as shown in the visualization.
### Changes in this commit:
1. Uses `chart.query_context` for data retrieval (same as `/api/v1/chart/{id}/data`)
2. Falls back to `form_data` construction only if `query_context` is unavailable
3. Removed `include_raw_data` option (was solving the wrong problem)
4. Row limit override still works and is capped at 10000 for safety
### Testing
Unfortunately I don't have MCP running locally with a Preset instance to test against. Could you verify this fix works correctly with your line chart example?
--
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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3717284288
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | d73b5f8306f1fcaea332c576d8d41e88971704c3 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/695de1c3415e5600088ea250 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-36937--superset-docs-preview.netlify.app](https://deploy-preview-36937--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> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-36937--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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #36937:
URL: https://github.com/apache/superset/pull/36937#discussion_r2668795281
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -36,22 +36,30 @@
PerformanceMetadata,
)
from superset.mcp_service.utils.cache_utils import get_cache_status_from_result
+from superset.mcp_service.utils.schema_utils import parse_request
logger = logging.getLogger(__name__)
+# Maximum rows to prevent unbounded fetches and excessive memory/DB load
+MAX_ROW_LIMIT = 10000
+
Review Comment:
It's nice to have an override (like maybe when I'm talking to the MCP I might say what are the top xxx from my chart so I don't need to bring all rows) but if not provided, I wonder if we should just respect what's in the chart.
--
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): use chart.query_context for get_chart_data like the API does [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3720452096
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3717280054
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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #36937:
URL: https://github.com/apache/superset/pull/36937#discussion_r2668786065
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -36,22 +36,30 @@
PerformanceMetadata,
)
from superset.mcp_service.utils.cache_utils import get_cache_status_from_result
+from superset.mcp_service.utils.schema_utils import parse_request
logger = logging.getLogger(__name__)
+# Maximum rows to prevent unbounded fetches and excessive memory/DB load
+MAX_ROW_LIMIT = 10000
+
Review Comment:
Should we just respect the row limit configured at the chart level?
--
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): use chart.query_context for get_chart_data like the API does [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #36937:
URL: https://github.com/apache/superset/pull/36937
--
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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3717283976
## Nitpicks π
<table>
<tr><td>π <strong>No security issues identified</strong></td></tr>
<tr><td>β‘ <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/36937/files#diff-b7dc9fa8d3969426f0aa364ad6169dfc9401554a5a568769f8dbf046e0d3e36fR169-R174'><strong>Unbounded row fetch risk</strong></a><br>When `include_raw_data` is true the code uses `request.limit or 100` as the
`row_limit`. If clients pass a very large `limit` (or none is provided)
this may return large datasets and cause heavy DB load / memory use.
Consider enforcing a configurable hard cap or validating the requested limit.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36937/files#diff-c53108521a676f135ad7846c809dddc3eb33240abb7d123d0948cd5f7b2e9765R1117-R1130'><strong>Backward-incompatible schema change</strong></a><br>The `DataColumn` model now requires `sample_values`, `null_count`, and
`unique_count` (no defaults). Making these fields required can cause
validation errors where code paths / tests or existing responses construct
chart data without those fields. This may break consumers that expect the
previous, lighter-weight schema.<br>
</td></tr>
</table>
--
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): add missing @parse_request decorator to get_chart_data and add raw data option [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 #36937:
URL: https://github.com/apache/superset/pull/36937#discussion_r2667037276
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -138,19 +143,61 @@ async def get_chart_data( # noqa: C901
# Create a proper QueryContext using the factory with cache control
factory = QueryContextFactory()
- query_context = factory.create(
- datasource={"id": chart.datasource_id, "type": chart.datasource_type},
- queries=[
- {
+
+ # Build query based on whether raw data is requested
+ if request.include_raw_data:
+ await ctx.info(
+ "Raw data mode enabled - retrieving underlying dataset columns"
+ )
+ # Get the datasource to access its columns using DAO pattern
+ from superset.daos.dataset import DatasetDAO
+
+ datasource = (
+ DatasetDAO.find_by_id(chart.datasource_id)
+ if chart.datasource_type == "table"
+ else None
+ )
+
+ if datasource:
+ # Get all column names from the datasource
+ raw_columns_list = [col.column_name for col in datasource.columns]
+ await ctx.debug(
+ "Found %s columns in datasource" % len(raw_columns_list)
+ )
+ query_spec = {
+ "filters": form_data.get("filters", []),
+ "columns": raw_columns_list, # All columns, no aggregation
+ "metrics": [], # No metrics = no aggregation
+ "row_limit": request.limit or 100,
+ "order_desc": True,
+ "cache_timeout": request.cache_timeout,
+ }
+ else:
+ await ctx.warning(
+ "Could not load datasource for raw data, "
+ "falling back to chart query"
+ )
+ query_spec = {
"filters": form_data.get("filters", []),
"columns": form_data.get("groupby", []),
"metrics": form_data.get("metrics", []),
"row_limit": request.limit or 100,
"order_desc": True,
- # Apply cache control from request
"cache_timeout": request.cache_timeout,
}
- ],
+ else:
+ query_spec = {
+ "filters": form_data.get("filters", []),
+ "columns": form_data.get("groupby", []),
+ "metrics": form_data.get("metrics", []),
+ "row_limit": request.limit or 100,
Review Comment:
**Suggestion:** Logic bug: using "request.limit or 100" treats falsy values (0) as missing and can produce inconsistent behavior compared with other uses of `request.limit`; use an explicit None check to preserve a provided 0 and avoid surprising fallbacks. [logic error]
**Severity Level:** Minor β οΈ
```suggestion
"row_limit": request.limit if request.limit is not None else 100,
"order_desc": True,
"cache_timeout": request.cache_timeout,
}
else:
await ctx.warning(
"Could not load datasource for raw data, "
"falling back to chart query"
)
query_spec = {
"filters": form_data.get("filters", []),
"columns": form_data.get("groupby", []),
"metrics": form_data.get("metrics", []),
"row_limit": request.limit if request.limit is not None else 100,
"order_desc": True,
"cache_timeout": request.cache_timeout,
}
else:
query_spec = {
"filters": form_data.get("filters", []),
"columns": form_data.get("groupby", []),
"metrics": form_data.get("metrics", []),
"row_limit": request.limit if request.limit is not None else 100,
```
<details>
<summary><b>Why it matters? β </b></summary>
This is a real logic bug: using `x or 100` will treat valid 0 limits as missing and substitute 100. The improved check (`request.limit if request.limit is not None else 100`) preserves an explicit 0 and aligns behaviour with callers that intentionally pass 0. Fixing it prevents surprising unintended defaults.
</details>
<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_data.py
**Line:** 171:193
**Comment:**
*Logic Error: Logic bug: using "request.limit or 100" treats falsy values (0) as missing and can produce inconsistent behavior compared with other uses of `request.limit`; use an explicit None check to preserve a provided 0 and avoid surprising fallbacks.
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.
```
</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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3717298217
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/36937?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 68.13%. Comparing base ([`9aff89c`](https://app.codecov.io/gh/apache/superset/commit/9aff89c1b4b783c7cd8809ea8d46a273c12f6f4e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d73b5f8`](https://app.codecov.io/gh/apache/superset/commit/d73b5f8306f1fcaea332c576d8d41e88971704c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #36937 +/- ##
===========================================
+ Coverage 0 68.13% +68.13%
===========================================
Files 0 639 +639
Lines 0 47657 +47657
Branches 0 5204 +5204
===========================================
+ Hits 0 32470 +32470
- Misses 0 13903 +13903
- Partials 0 1284 +1284
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/36937/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/36937/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.09% <ΓΈ> (?)` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/36937/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.22% <ΓΈ> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/36937/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.69% <ΓΈ> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/36937/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.10% <ΓΈ> (?)` | |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/36937/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.94% <ΓΈ> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/36937/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (?)` | |
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 Sentry](https://app.codecov.io/gh/apache/superset/pull/36937?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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3717285795
CodeAnt AI finished reviewing your 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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3719416330
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 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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #36937:
URL: https://github.com/apache/superset/pull/36937#discussion_r2669190191
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -36,22 +36,30 @@
PerformanceMetadata,
)
from superset.mcp_service.utils.cache_utils import get_cache_status_from_result
+from superset.mcp_service.utils.schema_utils import parse_request
logger = logging.getLogger(__name__)
+# Maximum rows to prevent unbounded fetches and excessive memory/DB load
+MAX_ROW_LIMIT = 10000
+
Review Comment:
Good point! I've updated the logic to respect the chart's configured row_limit by default:
# Use request.limit if specified, otherwise use chart's configured
# row_limit, with MAX_ROW_LIMIT as safety cap
chart_row_limit = form_data.get("row_limit", 1000)
row_limit = min(request.limit or chart_row_limit, MAX_ROW_LIMIT)
So now:
- Default: Uses the chart's configured row_limit
- Override: request.limit takes precedence (e.g., "show me top 10 from this chart")
- Safety cap: MAX_ROW_LIMIT (10000) still applies to prevent unbounded fetches
--
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): add missing @parse_request decorator to get_chart_data and add raw data option [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36937:
URL: https://github.com/apache/superset/pull/36937#issuecomment-3717429161
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 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