codeant-ai-for-open-source[bot] commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3655394375
##########
superset/versioning/queries.py:
##########
@@ -381,10 +382,10 @@ def resolve_version_uuid(
transaction in Python because there's no portable SQL form for a
UUIDv5 derivation across PostgreSQL / MySQL / SQLite (Postgres has
``uuid_generate_v5``; the other two do not). The iteration count is
- bounded by the configured retention window worth of edits — the
- retention task ages older shadow rows out — so the
+ bounded by ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` worth of
+ edits — the retention task ages older shadow rows out — so the
practical N is at most a few hundred. If retention is ever
- disabled on a heavily-edited entity, this loop is the
+ disabled (``= 0``) on a heavily-edited entity, this loop is the
Review Comment:
**Suggestion:** This documentation claims iteration is bounded by the
retention window, but the retention task preserves an entire transaction
whenever any unrelated parent, child, or M2M shadow row anchored there is live.
Consequently, old closed rows for an entity can remain indefinitely when their
transactions are retained by another live shadow, so `resolve_version_uuid` can
still iterate over an unbounded history. [docstring mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Version UUID resolution can scan history beyond retention.
- ⚠️ Large entities can incur slower version lookups.
- ⚠️ The docstring gives an incorrect performance guarantee.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the entity-versioning write listeners configured in
`superset/initialization/__init__.py:716` to create an old transaction
containing a
version row for entity A and a live shadow row for another versioned entity.
2. Create newer versions of entity A so its earlier rows become closed,
while the
unrelated live shadow row remains anchored to the old transaction.
3. Run the scheduled `prune_old_versions()` task at
`superset/tasks/version_history_retention.py:530`;
`_delete_for_transactions()` at line
263 preserves any transaction containing a live parent, child, or M2M shadow
row, so the
old transaction and entity A's closed rows are retained despite exceeding
the configured
retention window.
4. Call `resolve_version_uuid()` at `superset/versioning/queries.py:366` for
entity A.
After the optional lookup at lines 394-396, its transaction loop can still
traverse those
retained historical rows, contradicting the bounded-iteration claim at lines
385-388; with
retention disabled, the same function explicitly permits unrestricted
history.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6f625d4456e8434fb23946ed694d9f2b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6f625d4456e8434fb23946ed694d9f2b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/versioning/queries.py
**Line:** 385:388
**Comment:**
*Docstring Mismatch: This documentation claims iteration is bounded by
the retention window, but the retention task preserves an entire transaction
whenever any unrelated parent, child, or M2M shadow row anchored there is live.
Consequently, old closed rows for an entity can remain indefinitely when their
transactions are retained by another live shadow, so `resolve_version_uuid` can
still iterate over an unbounded history.
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%2F41075&comment_hash=a679d1ddcd426382aae90923635179afa89daeff0a77b1d68a516ddad218b2a6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=a679d1ddcd426382aae90923635179afa89daeff0a77b1d68a516ddad218b2a6&reaction=dislike'>👎</a>
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -210,21 +210,7 @@ async def execute_sql(request: ExecuteSqlRequest, ctx:
Context) -> ExecuteSqlRes
"template_params supplied but ENABLE_TEMPLATE_PROCESSING is
off"
)
- # Log successful execution
- if response.success:
- await ctx.info(
- "SQL execution completed successfully: rows_returned=%s, "
- "execution_time=%s"
- % (
- response.row_count,
- response.execution_time,
- )
- )
- else:
- await ctx.info(
- "SQL execution failed: error=%s, error_type=%s"
- % (response.error, response.error_type)
- )
+ await _log_execution_result(response, ctx)
return response
Review Comment:
**Suggestion:** Calling `_log_execution_result` directly on the success path
makes SQL execution depend on the MCP notification channel. If `ctx.info` fails
because the client disconnects or the transport errors after the database query
has already completed, this raises from `execute_sql` and reports a successful
query as a failed execution. Handle logging failures independently so telemetry
cannot change the query result. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Completed SQL can be reported as failed.
- ⚠️ Callers may retry already-executed queries.
- ⚠️ MCP transport failures obscure database execution results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke the MCP SQL execution tool, which enters `execute_sql()` at
`superset/mcp_service/sql_lab/tool/execute_sql.py:114`, with a valid
read-only SQL query
and an active MCP context.
2. Allow the database query and response construction to complete
successfully, producing
an `ExecuteSqlResponse` with `response.success` set to true.
3. Disconnect the MCP client or otherwise make the notification transport
fail before
`ctx.info()` is called by `_log_execution_result()` at
`superset/mcp_service/sql_lab/tool/execute_sql.py:247-260`.
4. The `await _log_execution_result(response, ctx)` call at line 213
propagates the
notification exception into `execute_sql()`, where the broad exception
handler at lines
236-244 reports execution failure and re-raises even though the SQL already
completed
successfully.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b81dabb72e644994a056be9db8ac0d78&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b81dabb72e644994a056be9db8ac0d78&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/sql_lab/tool/execute_sql.py
**Line:** 213:215
**Comment:**
*Possible Bug: Calling `_log_execution_result` directly on the success
path makes SQL execution depend on the MCP notification channel. If `ctx.info`
fails because the client disconnects or the transport errors after the database
query has already completed, this raises from `execute_sql` and reports a
successful query as a failed execution. Handle logging failures independently
so telemetry cannot change the query result.
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%2F41075&comment_hash=d76355f55724a72d5585583194bf4470162b42faa54cd217970cd4c6b8c0a211&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=d76355f55724a72d5585583194bf4470162b42faa54cd217970cd4c6b8c0a211&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]
