codeant-ai-for-open-source[bot] commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3580559890
##########
superset/sql/execution/celery_task.py:
##########
@@ -137,8 +141,52 @@ def _prepare_statement_blocks(
# Build statement blocks for execution
if db_engine_spec.run_multiple_statements_as_one:
- blocks =
[parsed_script.format(comments=db_engine_spec.allows_sql_comments)]
+ if app.config["MUTATE_AFTER_SPLIT"]:
+ # These engines never actually execute statements individually, so
the
+ # per-block mutation call in `execute_sql_with_cursor` (whose
`is_split`
+ # is always `False` here) would never fire. Mutate each statement
here,
+ # before joining them into the single block this engine requires,
so
+ # `MUTATE_AFTER_SPLIT=True` still applies the mutator per
statement.
+ blocks = [
+ ";\n".join(
+ database.mutate_sql_based_on_config(
+
statement.format(comments=db_engine_spec.allows_sql_comments),
+ is_split=True,
+ )
+ for statement in parsed_script.statements
+ )
+ ]
Review Comment:
**Suggestion:** When `run_multiple_statements_as_one` and
`MUTATE_AFTER_SPLIT` are both true, per-statement mutation output is joined
without validating that anything executable remains. If the mutator returns
only comments/whitespace (or empty strings), the code still builds a single
block and execution can silently run an empty SQL block (or fail with a
backend-specific error) instead of raising the intended clean
`INVALID_SQL_ERROR`. Add a post-mutation validation step in this branch
(re-parse the joined SQL and ensure it has executable statements) before
returning `blocks`. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Async SQL Lab queries with mutator lack clean invalid-SQL errors.
⚠️ Run-as-one engines may execute empty or invalid statements.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Database whose engine spec sets
run_multiple_statements_as_one=True (for
example BigQuery or Kusto, as referenced in
superset/sql/execution/celery_task.py:37-39)
and enable async execution for SQL Lab on this database.
2. In the Superset app config, set SQL_QUERY_MUTATOR to a function that,
when called with
is_split=True, returns only comments, whitespace, or an empty string for the
test query,
and set MUTATE_AFTER_SPLIT=True; this wiring is used by
Database.mutate_sql_based_on_config at superset/models/core.py:706-724 where
the mutator
is invoked only when is_split equals app.config["MUTATE_AFTER_SPLIT"].
3. From SQL Lab, submit a multi-statement query on that database using async
execution
(SQLExecutor.execute_async delegates to the Celery task documented in
superset/sql/execution/celery_task.py:18-22 and implemented as
execute_sql_task at lines
382-388), which calls _execute_sql_statements at lines 77-81 with the
rendered_query
containing RLS/limit changes.
4. Inside _execute_sql_statements
(superset/sql/execution/celery_task.py:77-105), the
transformed SQL is parsed into a SQLScript and passed to
_prepare_statement_blocks(rendered_query, db_engine_spec, database). Because
db_engine_spec.run_multiple_statements_as_one is True and
app.config["MUTATE_AFTER_SPLIT"]
is True, _prepare_statement_blocks enters the branch at lines 44-59, calls
database.mutate_sql_based_on_config for each parsed_script.statement with
is_split=True
(lines 52-56), and joins the per-statement outputs with ";\n" into a single
block in
blocks[0] even if every mutated statement is now only comments/whitespace or
effectively
empty.
5. _execute_sql_statements then calls execute_sql_with_cursor
(superset/sql/execution/executor.py:96-190) with statements=blocks and
is_split set to not
db_engine_spec.run_multiple_statements_as_one (celery_task.py:99-105, so
is_split=False
for run-as-one engines). In execute_sql_with_cursor,
mutate_sql_based_on_config is called
with is_split=False (executor.py:145-149) and therefore bypasses
SQL_QUERY_MUTATOR when
MUTATE_AFTER_SPLIT=True, and database.db_engine_spec.execute is invoked on
the empty or
comment-only block. Because the validation guard that re-parses mutated SQL
and raises
SupersetErrorException with INVALID_SQL_ERROR when no executable statements
remain exists
only in the opposite branch of _prepare_statement_blocks (lines 63-90 for
MUTATE_AFTER_SPLIT=False) and not in this run-as-one+mutator-per-statement
branch, the
query either silently executes a non-executable block or surfaces a
backend-specific error
instead of the intended clean INVALID_SQL_ERROR, demonstrating the missing
validation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a09a8812a82c489fb46c23b64ac418ee&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=a09a8812a82c489fb46c23b64ac418ee&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/sql/execution/celery_task.py
**Line:** 144:158
**Comment:**
*Incomplete Implementation: When `run_multiple_statements_as_one` and
`MUTATE_AFTER_SPLIT` are both true, per-statement mutation output is joined
without validating that anything executable remains. If the mutator returns
only comments/whitespace (or empty strings), the code still builds a single
block and execution can silently run an empty SQL block (or fail with a
backend-specific error) instead of raising the intended clean
`INVALID_SQL_ERROR`. Add a post-mutation validation step in this branch
(re-parse the joined SQL and ensure it has executable statements) before
returning `blocks`.
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%2F41127&comment_hash=78950673fac4957366de6fe69cc64b6c410ec8a3ac985db2ba9c68f80657880e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=78950673fac4957366de6fe69cc64b6c410ec8a3ac985db2ba9c68f80657880e&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]
