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: [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]
