codeant-ai-for-open-source[bot] commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3615935081
##########
superset/sql/execution/executor.py:
##########
@@ -87,12 +90,102 @@
StatementResult,
)
+ from superset.db_engine_specs.base import BaseEngineSpec
from superset.models.core import Database
from superset.result_set import SupersetResultSet
logger = logging.getLogger(__name__)
+def _raise_all_statements_stripped() -> NoReturn:
+ """Raise a clean error for a mutator that stripped a query down to
nothing."""
+ raise SupersetErrorException(
+ SupersetError(
+ message=__(
+ "The SQL query mutator removed all executable "
+ "statements from this query."
+ ),
+ error_type=SupersetErrorType.INVALID_SQL_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+
+def _has_executable_statements(sql: str, engine: str) -> bool:
+ """Best-effort check that mutated SQL still contains executable
statements."""
+ try:
+ return bool(SQLScript(sql, engine=engine).statements)
+ except SupersetParseError:
+ # A mutator may emit engine-specific SQL our parser can't handle; the
+ # database itself is the authority on validity in that case.
+ return True
+
+
+def build_statement_blocks(
+ parsed_script: SQLScript,
+ db_engine_spec: type[BaseEngineSpec],
+ database: Database,
+) -> tuple[SQLScript, list[str]]:
+ """
+ Build the SQL blocks to execute from a parsed script, applying
+ ``SQL_QUERY_MUTATOR`` according to ``MUTATE_AFTER_SPLIT``.
+
+ Some databases (like BigQuery and Kusto) do not persist state across
multiple
+ statements if they're run separately (especially when using `NullPool`),
so the
+ query runs as a single joined block when the engine spec requires it;
otherwise
+ each statement becomes its own block. Shared by the sync (``sql_lab``) and
+ async (``celery_task``) SQL Lab paths so the
+ ``run_multiple_statements_as_one`` × ``MUTATE_AFTER_SPLIT`` matrix behaves
+ identically in both.
+
+ Returns the (possibly re-parsed) script and the blocks to execute.
+
+ :raises SupersetErrorException: if the mutator strips the query down to
+ nothing executable (e.g. only comments/whitespace)
+ """
+ blocks: list[str]
+ if db_engine_spec.run_multiple_statements_as_one:
+ if app.config["MUTATE_AFTER_SPLIT"]:
+ # These engines never actually execute statements individually, so
+ # the per-block mutation call at execution time (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.
+ joined_block = ";\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
+ )
+ if not _has_executable_statements(joined_block,
db_engine_spec.engine):
+ _raise_all_statements_stripped()
+ blocks = [joined_block]
+ else:
+ blocks =
[parsed_script.format(comments=db_engine_spec.allows_sql_comments)]
+ else:
+ if not app.config["MUTATE_AFTER_SPLIT"]:
+ # `MUTATE_AFTER_SPLIT=False` means the mutator should see the
whole,
+ # un-split query, but this engine executes statements individually.
+ # Mutate the whole block up front and re-parse it, so the
+ # per-statement split below (and the per-block mutation call at
+ # execution time, which is a no-op here since its `is_split=True`
no
+ # longer matches the config) operate on the already-mutated SQL.
+ mutated_sql: str = database.mutate_sql_based_on_config(
+
parsed_script.format(comments=db_engine_spec.allows_sql_comments),
+ is_split=False,
+ )
+ parsed_script = SQLScript(mutated_sql,
engine=db_engine_spec.engine)
+ if not parsed_script.statements:
+ _raise_all_statements_stripped()
Review Comment:
**Suggestion:** Re-parsing the mutator output without handling parser
failures introduces a regression: if `SQL_QUERY_MUTATOR` emits engine-specific
SQL that Superset’s parser cannot parse (but the database can execute), this
now raises `SupersetParseError` and aborts execution for
`MUTATE_AFTER_SPLIT=False` on split engines. Catch parse errors here and fall
back to a safe execution path instead of failing before sending SQL to the
database. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SQL Lab queries fail when mutator emits unparsable SQL.
- ⚠️ MUTATE_AFTER_SPLIT=False split engines see unexpected failures.
- ⚠️ Behavior inconsistent with _has_executable_statements parse handling.
- ⚠️ Governance mutators become brittle for advanced engine syntax.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an engine whose spec does not join statements
(db_engine_spec.run_multiple_statements_as_one is False) and set
app.config["MUTATE_AFTER_SPLIT"] = False so build_statement_blocks enters
the branch at
`superset/sql/execution/executor.py:167-185`.
2. Register a SQL_QUERY_MUTATOR so that
`database.mutate_sql_based_on_config(...)` at
`superset/sql/execution/executor.py:174-176` returns SQL that is valid for
the database
but uses engine-specific syntax the `SQLScript` parser cannot handle (causing
`SupersetParseError` on parse).
3. From SQL Lab, execute a multi-statement query against this Database so
the parsed
script is built and `build_statement_blocks(...)` at
`superset/sql/execution/executor.py:124-186` is invoked by the shared
sync/async SQL Lab
execution paths described in the module docstring at lines 42-54 and in the
function
docstring at lines 130-145.
4. In the `MUTATE_AFTER_SPLIT=False` / split-engine branch, the code at
`superset/sql/execution/executor.py:178` runs `parsed_script =
SQLScript(mutated_sql,
engine=db_engine_spec.engine)` without a try/except; the mutator’s
engine-specific SQL
triggers `SupersetParseError`, which is not caught here (unlike
`_has_executable_statements` at lines 114-121), causing the request to fail
before any SQL
is sent to the database.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=527b1f0b3718480a9838c75c257396fc&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=527b1f0b3718480a9838c75c257396fc&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/executor.py
**Line:** 174:180
**Comment:**
*Logic Error: Re-parsing the mutator output without handling parser
failures introduces a regression: if `SQL_QUERY_MUTATOR` emits engine-specific
SQL that Superset’s parser cannot parse (but the database can execute), this
now raises `SupersetParseError` and aborts execution for
`MUTATE_AFTER_SPLIT=False` on split engines. Catch parse errors here and fall
back to a safe execution path instead of failing before sending SQL to the
database.
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=356b5173649d37127197ae854c0f2e8ac68fd581648fdaf6454e74529341dd3e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=356b5173649d37127197ae854c0f2e8ac68fd581648fdaf6454e74529341dd3e&reaction=dislike'>👎</a>
##########
superset/sql/execution/executor.py:
##########
@@ -140,8 +238,23 @@ def execute_sql_with_cursor(
# Apply SQL mutation
stmt_sql = database.mutate_sql_based_on_config(
statement,
- is_split=True,
+ is_split=is_split,
)
+ if not stmt_sql.strip():
+ # A `SQL_QUERY_MUTATOR` that strips a statement down to nothing
+ # would otherwise be sent to the database engine as an empty
+ # query, surfacing a confusing engine-specific error instead of
+ # a clean one.
+ raise SupersetErrorException(
+ SupersetError(
+ message=__(
+ "The SQL query mutator removed all executable "
+ "statements from this query."
+ ),
+ error_type=SupersetErrorType.INVALID_SQL_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
Review Comment:
**Suggestion:** The new empty-statement guard only checks whitespace, so a
mutator output containing only comments (for example `-- ...`) bypasses this
check and still gets executed as a non-executable statement, leading to the
same confusing engine-specific errors this block is trying to prevent. Use
executable-statement detection (comment-aware parsing) instead of plain
`strip()`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Comment-only mutator output still executed as statements.
- ⚠️ Users see engine-specific errors for non-executable blocks.
- ⚠️ Per-statement guard inconsistent with _has_executable_statements logic.
- ⚠️ Governance policies that drop statements get unclear feedback.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a SQL_QUERY_MUTATOR on the Database model so
`database.mutate_sql_based_on_config(...)` at
`superset/sql/execution/executor.py:239-242`
can transform some statements into a comment-only string (for example `"--
stripped by
mutator"`).
2. Use a database engine where
`db_engine_spec.run_multiple_statements_as_one` is False so
SQL Lab executes each statement separately; in this case
`execute_sql_with_cursor(...)` at
`superset/sql/execution/executor.py:189-222` is called with `is_split=True`
and the
individual statements list produced by `build_statement_blocks(...)`.
3. From SQL Lab, submit a multi-statement query where one statement is
modified by the
mutator into comment-only SQL; during execution, the loop at
`superset/sql/execution/executor.py:231-241` sets `stmt_sql` to that
comment-only value.
4. The guard at `superset/sql/execution/executor.py:243-257` checks only `if
not
stmt_sql.strip():`; since comment-only SQL is non-empty after `strip()`,
this condition is
False, so no `SupersetErrorException` is raised and the comment-only
statement is sent to
the database, which can return a confusing engine-specific error instead of
the intended
“removed all executable statements” error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=92645256ab864f5885507d04102bccd8&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=92645256ab864f5885507d04102bccd8&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/executor.py
**Line:** 243:257
**Comment:**
*Logic Error: The new empty-statement guard only checks whitespace, so
a mutator output containing only comments (for example `-- ...`) bypasses this
check and still gets executed as a non-executable statement, leading to the
same confusing engine-specific errors this block is trying to prevent. Use
executable-statement detection (comment-aware parsing) instead of plain
`strip()`.
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=85db3122457a20ded7779f8e98eac0068ba4d9280f6479546d03bc95cc978f95&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=85db3122457a20ded7779f8e98eac0068ba4d9280f6479546d03bc95cc978f95&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]
