sadpandajoe opened a new pull request, #45070:
URL: https://github.com/apache/superset/pull/45070
### SUMMARY
Clicking **Stop** on a running SQL Lab query against ClickHouse had no
effect: the query kept running on the server (consuming resources) and/or the
user saw `SupersetCancelQueryException: Could not cancel query`. Root cause:
`superset/db_engine_specs/clickhouse.py` never implemented any
query-cancellation hooks, so every cancel request fell through to
`BaseEngineSpec`'s inert defaults (`get_cancel_query_id` → `None`,
`cancel_query` → `False`), regardless of whether a real cancel attempt was even
possible.
This PR implements real cancellation for `ClickHouseConnectEngineSpec` (the
`clickhouse-connect`-driver-based engine spec, i.e. the one new/current
installs use):
- `get_cancel_query_id()` mints a client-side UUID synchronously, before any
statement executes.
- That UUID is threaded through to the actual statement as ClickHouse's own
`query_id` transport setting (`cursor.execute(sql, settings={"query_id":
...})`), making it the query's real server-side identifier.
- `cancel_query()` issues `KILL QUERY WHERE query_id = '<uuid>' SYNC` on a
fresh connection (never the busy one running the long query) and inspects the
returned `kill_status` to distinguish a confirmed kill from an unconfirmed
result, rather than assuming success whenever the statement doesn't raise.
- A pre-dispatch check in `execute_with_cursor()` refuses to send a
statement whose query was already marked stopped, closing the specific window
where ClickHouse's query-level `KILL` (unlike Postgres/MySQL's connection-level
kill) has no side effect that would otherwise prevent the statement from
running anyway.
### SCOPE
This is intentionally scoped to `ClickHouseConnectEngineSpec` only. The
legacy `clickhouse-sqlalchemy`-driver-based `ClickHouseEngineSpec` is unchanged
— its cursor's `execute()` has no `settings` parameter, so applying the same
override there would break every query. Stop remains a no-op on the legacy
driver; a real fix for it would need a different mechanism and is out of scope
here.
### KNOWN LIMITATIONS (disclosed, not silently omitted)
- A narrow check-then-commit race remains between this PR's own pre-dispatch
status refresh and the moment the statement is actually sent — the same general
class of TOCTOU window already present throughout `execute_sql_statements()`
for every engine (would need real DB-level row locking to close fully; not
attempted here, consistent with the rest of this file's existing disclosed
limitations).
- The async/Celery execution path (`superset/sql/execution/celery_task.py`)
has a separate, pre-existing gap where an exception raised during dispatch can
convert an already-STOPPED query to FAILED instead of preserving STOPPED —
confirmed this already affects Postgres/MySQL's existing cancellation today and
is not newly introduced by this PR. Left unfixed here as a distinct,
pre-existing issue in shared execution code.
- `KILL QUERY` requires the connected ClickHouse user to hold that privilege
under RBAC-configured deployments; without it, cancellation fails and the
existing `SupersetCancelQueryException` surfaces to the user (a clear message,
not a silent no-op), matching the behavior other engines already have in the
same situation.
### TESTING STRATEGY
- Unit tests covering the new
`get_cancel_query_id`/`cancel_query`/`execute_with_cursor` logic, including the
`kill_status` branches and the pre-dispatch stop check.
- An integration test using this repo's existing `testcontainers`-based
harness against a real `clickhouse-server` container: starts a genuinely
long-running query, confirms via `system.processes` that it's actually running,
cancels it, and confirms both that the API reports it stopped and that
`system.processes` drops to zero (i.e. the query is actually gone from the
server, not just reported as stopped).
- Verified the new regression tests actually discriminate a real cancel from
a no-op by temporarily reverting the fix and confirming the tests fail for the
right reason, then restoring it and confirming they pass.
--
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]
