rusackas commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3580498376
##########
superset/sql/execution/executor.py:
##########
@@ -101,6 +101,7 @@ def execute_sql_with_cursor(
log_query_fn: Any | None = None,
check_stopped_fn: Any | None = None,
execute_fn: Any | None = None,
+ is_split: bool = True,
Review Comment:
Not a regression — before this PR, `execute_sql_with_cursor` always called
`mutate_sql_based_on_config(statement, is_split=True)` hardcoded, with no
parameter at all. The new `is_split: bool = True` default just preserves that
exact prior behavior for the SQLExecutor sync path (which does not pass it
explicitly). It's the SQL Lab callers in `sql_lab.py` that now pass the correct
`is_split` explicitly, which is the actual fix.
##########
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:
This is inherent to the fix, not avoidable — `MUTATE_AFTER_SPLIT=False`
means the mutator runs on the whole un-split query, but split engines still
need per-statement blocks, so re-splitting the mutator's output is required
either way. If a mutator emits SQL our parser can't handle, `SQLScript(...)`
already raises `SupersetParseError`, which is a `SupersetErrorException` with a
clean message — not a raw crash — so this is already handled gracefully.
--
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]
