codeant-ai-for-open-source[bot] commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3578689541
##########
superset/sql_lab.py:
##########
@@ -474,12 +474,55 @@ def execute_sql_statements( # noqa: C901
for statement in parsed_script.statements:
apply_limit(query, statement)
- # some databases (like BigQuery and Kusto) do not persist state across
mmultiple
+ # some databases (like BigQuery and Kusto) do not persist state across
multiple
# statements if they're run separately (especially when using `NullPool`),
so we run
# the query as a single block.
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 further down (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
+ )
+ ]
+ 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 further down, 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)
Review Comment:
**Suggestion:** Re-parsing the output of `SQL_QUERY_MUTATOR` introduces a
new hard dependency on Superset's SQL parser for mutated SQL. Custom mutators
are allowed to inject engine-specific statements, and those can be valid for
the database but not parseable by `SQLScript`, which will now fail before
execution for `MUTATE_AFTER_SPLIT=False` on split engines. Handle parse
failures (or avoid re-parsing mutator output) so valid database SQL produced by
the mutator still executes. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SQL Lab queries fail for certain SQL_QUERY_MUTATOR configurations.
- ⚠️ Governance mutators using vendor syntax become harder to deploy.
- ⚠️ Error surfaced as parse failure, not database execution.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a SQL mutator by setting `SQL_QUERY_MUTATOR` in the Superset
config (see
`superset/config.py:2005-2011` and `superset/models/core.py:706-724`) so
that it prepends
or injects database-specific statements (e.g. vendor commands) that are
valid for the
target database but not fully supported by Superset’s sqlglot-based parser
(causing
`SQLStatement._parse` in `superset/sql/parse.py:588-667` to raise
`SupersetParseError`
when given that mutated script).
2. Ensure `MUTATE_AFTER_SPLIT` remains `False` (the default as defined in
`superset/config.py:2004-2014`) and use a database engine whose
`db_engine_spec.run_multiple_statements_as_one` is `False` (the typical case
for engines
that execute split statements individually), so that
`execute_sql_statements` in
`superset/sql_lab.py:391-420` will enter the `else` branch at
`superset/sql_lab.py:59-87`
for split engines with `MUTATE_AFTER_SPLIT=False`.
3. From SQL Lab, run a simple multi-statement query (for example via the
Celery task
`get_sql_results` in `superset/sql_lab.py:6-28`, which calls
`execute_sql_statements`),
where the original `rendered_query` is fully parseable by
`SQLScript(rendered_query,
engine=db_engine_spec.engine)` at `superset/sql_lab.py:42` so the initial
parsing and
security checks succeed before any mutation is applied.
4. In the split-engine, `MUTATE_AFTER_SPLIT=False` branch at
`superset/sql_lab.py:60-71`,
observe that `database.mutate_sql_based_on_config(...)` is called with
`is_split=False`,
causing your configured `SQL_QUERY_MUTATOR` to run on the whole un-split
query and produce
`mutated_sql` that includes the engine-specific statements introduced by the
mutator.
5. Immediately afterward, the code re-parses the mutator output via
`parsed_script =
SQLScript(mutated_sql, engine=db_engine_spec.engine)` at
`superset/sql_lab.py:71`, which
calls `SQLStatement.split_script` and `_parse` in
`superset/sql/parse.py:588-746`; because
this new `mutated_sql` contains syntax the parser cannot handle even though
the database
would accept it, `SQLScript` construction raises `SupersetParseError`
instead of returning
a script.
6. The exception bubbles up to `get_sql_results` in
`superset/sql_lab.py:17-34`, which
catches `Exception` and passes it to `handle_query_error(ex, query)`,
resulting in SQL Lab
surfacing a parse error and aborting execution; this is a regression from
the pre-PR
behavior where the mutator’s output (returned by
`mutate_sql_based_on_config`) was sent
directly to the engine without being re-parsed, so the same vendor-specific
SQL would have
executed successfully.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3cfe225f96db407796110cac7afe5582&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=3cfe225f96db407796110cac7afe5582&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_lab.py
**Line:** 506:510
**Comment:**
*Api Mismatch: Re-parsing the output of `SQL_QUERY_MUTATOR` introduces
a new hard dependency on Superset's SQL parser for mutated SQL. Custom mutators
are allowed to inject engine-specific statements, and those can be valid for
the database but not parseable by `SQLScript`, which will now fail before
execution for `MUTATE_AFTER_SPLIT=False` on split engines. Handle parse
failures (or avoid re-parsing mutator output) so valid database SQL produced by
the mutator still executes.
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=c8cfb19e63be78e1aae3035400dc9a29af0702b8ed7616dda6be6fa009203f1c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=c8cfb19e63be78e1aae3035400dc9a29af0702b8ed7616dda6be6fa009203f1c&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]
