codeant-ai-for-open-source[bot] commented on code in PR #36739:
URL: https://github.com/apache/superset/pull/36739#discussion_r2634831997
##########
superset-core/src/superset_core/api/models.py:
##########
@@ -139,7 +143,11 @@ def execute_async(
Returns immediately with a handle for tracking progress and retrieving
results from the background worker.
- :param sql: SQL query to execute
+ The SQL must be written in the dialect of the target database (e.g.,
+ PostgreSQL syntax for PostgreSQL databases, Snowflake syntax for
+ Snowflake, etc.). No automatic cross-dialect translation is performed.
+
+ :param sql: SQL query to execute (in the target database's dialect)
Review Comment:
**Suggestion:** Duplicate parameter documentation in
`Database.execute_async`: the new docstring block adds another `:param sql:`
entry for the async executor, which will duplicate parameter documentation and
may trigger doc warnings or inconsistencies; remove the `:param sql:` line from
this added block. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same as the previous suggestion but for execute_async: the inserted block
contains a `:param sql:` entry that appears redundant given the rest of the
docstring. Removing the duplicate parameter line avoids duplicated docs and
is the right minimal change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/api/models.py
**Line:** 149:150
**Comment:**
*Possible Bug: Duplicate parameter documentation in
`Database.execute_async`: the new docstring block adds another `:param sql:`
entry for the async executor, which will duplicate parameter documentation and
may trigger doc warnings or inconsistencies; remove the `:param sql:` line from
this added block.
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>
##########
superset-core/src/superset_core/api/models.py:
##########
@@ -92,7 +92,11 @@ def execute(
"""
Execute SQL synchronously.
- :param sql: SQL query to execute
+ The SQL must be written in the dialect of the target database (e.g.,
+ PostgreSQL syntax for PostgreSQL databases, Snowflake syntax for
+ Snowflake, etc.). No automatic cross-dialect translation is performed.
+
+ :param sql: SQL query to execute (in the target database's dialect)
Review Comment:
**Suggestion:** Duplicate parameter documentation: the inserted block in
`Database.execute` includes a second `:param sql:` entry inside the docstring,
which will produce duplicated `sql` parameter entries in generated
documentation and can confuse doc consumers or doc-build tools; remove this
duplicated `:param sql:` line from the added block (leave the actual parameter
doc where it already exists below). [possible bug]
**Severity Level:** Critical 🚨
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion correctly points out that the added block in Database.execute
contains a `:param sql:` line which duplicates the parameter documentation
already present in the function docstring. This can create duplicate entries
in generated docs and trigger warnings from doc-build tools. Removing the
duplicate `:param sql:` from the inserted paragraph is a safe, focused
docfix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/api/models.py
**Line:** 98:99
**Comment:**
*Possible Bug: Duplicate parameter documentation: the inserted block in
`Database.execute` includes a second `:param sql:` entry inside the docstring,
which will produce duplicated `sql` parameter entries in generated
documentation and can confuse doc consumers or doc-build tools; remove this
duplicated `:param sql:` line from the added block (leave the actual parameter
doc where it already exists below).
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>
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -92,3 +138,55 @@ async def execute_sql(request: ExecuteSqlRequest, ctx:
Context) -> ExecuteSqlRes
)
)
raise
+
+
+def _convert_to_response(result: QueryResult) -> ExecuteSqlResponse:
+ """Convert QueryResult to ExecuteSqlResponse."""
+ if result.status != QueryStatus.SUCCESS:
+ return ExecuteSqlResponse(
+ success=False,
+ error=result.error_message,
+ error_type=result.status.value,
+ )
+
+ # Build statement info list
+ statements = [
+ StatementInfo(
+ original_sql=stmt.original_sql,
+ executed_sql=stmt.executed_sql,
+ row_count=stmt.row_count,
+ execution_time_ms=stmt.execution_time_ms,
+ )
+ for stmt in result.statements
+ ]
+
+ # Get first statement's data for backward compatibility
+ first_stmt = result.statements[0] if result.statements else None
Review Comment:
**Suggestion:** Robustness bug: the code iterates over and indexes
`result.statements` assuming it's a sequence; if `result.statements` is None
this will raise a TypeError; normalize to an empty list before
iterating/indexing. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
stmts = result.statements or []
statements = [
StatementInfo(
original_sql=stmt.original_sql,
executed_sql=stmt.executed_sql,
row_count=stmt.row_count,
execution_time_ms=stmt.execution_time_ms,
)
for stmt in stmts
]
# Get first statement's data for backward compatibility
first_stmt = stmts[0] if stmts else None
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
If result.statements can be None, both the list comprehension and the
indexing will raise. Normalizing to an empty list (stmts = result.statements or
[]) is a small, defensive change that avoids a runtime TypeError and preserves
current behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/sql_lab/tool/execute_sql.py
**Line:** 153:164
**Comment:**
*Possible Bug: Robustness bug: the code iterates over and indexes
`result.statements` assuming it's a sequence; if `result.statements` is None
this will raise a TypeError; normalize to an empty list before
iterating/indexing.
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>
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -92,3 +138,55 @@ async def execute_sql(request: ExecuteSqlRequest, ctx:
Context) -> ExecuteSqlRes
)
)
raise
+
+
+def _convert_to_response(result: QueryResult) -> ExecuteSqlResponse:
+ """Convert QueryResult to ExecuteSqlResponse."""
+ if result.status != QueryStatus.SUCCESS:
+ return ExecuteSqlResponse(
+ success=False,
+ error=result.error_message,
+ error_type=result.status.value,
+ )
+
+ # Build statement info list
+ statements = [
+ StatementInfo(
+ original_sql=stmt.original_sql,
+ executed_sql=stmt.executed_sql,
+ row_count=stmt.row_count,
+ execution_time_ms=stmt.execution_time_ms,
+ )
+ for stmt in result.statements
+ ]
+
+ # Get first statement's data for backward compatibility
+ first_stmt = result.statements[0] if result.statements else None
+ rows: list[dict[str, Any]] | None = None
+ columns: list[ColumnInfo] | None = None
+ row_count: int | None = None
+ affected_rows: int | None = None
+
+ if first_stmt and first_stmt.data is not None:
+ # SELECT query - convert DataFrame
+ df = first_stmt.data
+ rows = df.to_dict(orient="records")
+ columns = [ColumnInfo(name=col, type=str(df[col].dtype)) for col in
df.columns]
+ row_count = len(df)
+ elif first_stmt:
+ # DML query
+ affected_rows = first_stmt.row_count
+
+ return ExecuteSqlResponse(
+ success=True,
+ rows=rows,
+ columns=columns,
+ row_count=row_count,
+ affected_rows=affected_rows,
+ execution_time=(
+ result.total_execution_time_ms / 1000
+ if result.total_execution_time_ms
Review Comment:
**Suggestion:** Logic bug: `result.total_execution_time_ms` is tested using
truthiness so a value of 0 will be treated as missing and return None; compare
explicitly to None instead to preserve zero execution times. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
if result.total_execution_time_ms is not None
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current truthy check treats 0 as missing and will return None for a
valid 0ms execution time.
Changing the condition to an explicit "is not None" preserves zero values
and fixes a real logic bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/sql_lab/tool/execute_sql.py
**Line:** 188:188
**Comment:**
*Logic Error: Logic bug: `result.total_execution_time_ms` is tested
using truthiness so a value of 0 will be treated as missing and return None;
compare explicitly to None instead to preserve zero execution times.
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>
##########
superset/mcp_service/utils/schema_utils.py:
##########
@@ -508,10 +508,17 @@ def sync_wrapper(request: Any, *args: Any, **kwargs: Any)
-> Any:
new_params = []
for name, param in orig_sig.parameters.items():
# Skip ctx parameter - FastMCP tools don't expose it to clients
- if param.annotation is FMContext or (
- hasattr(param.annotation, "__name__")
- and param.annotation.__name__ == "Context"
- ):
+ # Check for Context type, forward reference string, or parameter
named 'ctx'
+ is_context = (
+ param.annotation is FMContext
+ or (
+ hasattr(param.annotation, "__name__")
+ and param.annotation.__name__ == "Context"
+ )
+ or param.annotation == "Context" # Forward reference string
Review Comment:
**Suggestion:** The forward-reference string check only matches the bare
"Context" string; dotted forward references like "fastmcp.Context" (or other
module-qualified forward refs) will be missed—treat string annotations that end
with ".Context" as matches as well. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
or (isinstance(param.annotation, str) and (param.annotation
== "Context" or param.annotation.endswith(".Context")))
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Forward-reference strings can be module-qualified (e.g., "fastmcp.Context")
and the current equality check only matches the bare "Context". Checking for
strings that end with ".Context" or otherwise handling dotted forward refs is a
simple, correct improvement to avoid leaking ctx for annotated forward refs.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/utils/schema_utils.py
**Line:** 518:518
**Comment:**
*Possible Bug: The forward-reference string check only matches the bare
"Context" string; dotted forward references like "fastmcp.Context" (or other
module-qualified forward refs) will be missed—treat string annotations that end
with ".Context" as matches as well.
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]
