You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/10/07 23:45:35 UTC
(superset) 01/07: test(clickhouse): add regression coverage for Stop not cancelling queries
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-clickhouse-stop-query-cancel
in repository https://gitbox.apache.org/repos/asf/superset.git
commit ee764cfd5c32a44c82af7c12ecc98363d62f13c3
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Wed Oct 7 21:56:40 2026 +0000
test(clickhouse): add regression coverage for Stop not cancelling queries
SQL Lab's Stop button is a no-op for ClickHouse: ClickHouseBaseEngineSpec
never implements query cancellation, so the request silently falls
through to BaseEngineSpec's inert defaults while the query keeps running
on the server. Adds a real-server testcontainers test that drives the
same get_cancel_query_id -> execute_with_cursor -> cancel_query sequence
sql_lab.py uses and checks system.processes directly, plus fast mocked
unit tests for the same behavior.
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
.../db_engine_specs/test_clickhouse.py | 77 ++++++++++++
.../unit_tests/db_engine_specs/test_clickhouse.py | 136 +++++++++++++++++++++
2 files changed, 213 insertions(+)
diff --git a/tests/testcontainers/db_engine_specs/test_clickhouse.py b/tests/testcontainers/db_engine_specs/test_clickhouse.py
index d66d800114a..e595c50d8b7 100644
--- a/tests/testcontainers/db_engine_specs/test_clickhouse.py
+++ b/tests/testcontainers/db_engine_specs/test_clickhouse.py
@@ -41,6 +41,8 @@ the real app instance `tests/conftest.py` already builds for the rest of the
test run rather than constructing a second one.
"""
+import threading
+import time
from collections.abc import Iterator
import pytest
@@ -171,3 +173,78 @@ def test_group_by_all_groups_correctly(engine: Engine) -> None:
).fetchall()
assert [tuple(row) for row in rows] == [("a", 15), ("b", 7)]
+
+
+def test_cancel_query_actually_stops_the_query_server_side(engine: Engine) -> None:
+ """
+ SQL Lab's Stop button must make the query actually stop running on the
+ ClickHouse server -- not just make Superset give up waiting on it. Drives
+ the real `get_cancel_query_id` -> `execute_with_cursor` -> `cancel_query`
+ sequence `superset/sql_lab.py` uses, against a real server, with a second,
+ independent connection/cursor for the cancellation itself (mirroring
+ `superset/sql_lab.py::cancel_query()`'s own `engine.raw_connection()`).
+ A regression that merely raises client-side without reaching a real
+ `KILL QUERY` on the server would leave the query in `system.processes`
+ and this test would catch that even though the client-side call returned.
+ """
+ from tests.integration_tests.test_app import app
+
+ with app.app_context():
+ from superset.db_engine_specs.clickhouse import ClickHouseConnectEngineSpec
+
+ from superset.models.sql_lab import Query
+
+ query = Query()
+ query.id = 1
+
+ worker_conn = engine.raw_connection()
+ worker_cursor = worker_conn.cursor()
+
+ cancel_query_id = ClickHouseConnectEngineSpec.get_cancel_query_id(
+ worker_cursor, query
+ )
+ assert cancel_query_id is not None
+ query.set_extra_json_key("cancel_query", cancel_query_id)
+
+ # ~20s synthetic long-running query (sleep(1) x 20 rows)
+ sql = "SELECT sleep(1) FROM numbers(20)"
+ elapsed_seconds: list[float] = []
+ errors: list[BaseException] = []
+ start = time.monotonic()
+
+ def run_query() -> None:
+ try:
+ ClickHouseConnectEngineSpec.execute_with_cursor(worker_cursor, sql, query)
+ except Exception as ex: # pylint: disable=broad-except
+ errors.append(ex)
+ finally:
+ elapsed_seconds.append(time.monotonic() - start)
+
+ worker_thread = threading.Thread(target=run_query)
+ worker_thread.start()
+ time.sleep(3)
+
+ cancel_conn = engine.raw_connection()
+ cancel_cursor = cancel_conn.cursor()
+ assert (
+ ClickHouseConnectEngineSpec.cancel_query(cancel_cursor, query, cancel_query_id)
+ is True
+ )
+
+ worker_thread.join(timeout=25)
+ assert not worker_thread.is_alive(), (
+ "worker query did not stop after cancel_query() returned True"
+ )
+ assert not errors, f"worker query raised unexpectedly: {errors[0]!r}"
+ # The real assertion: fewer than the full 20s elapsed, proving the query
+ # was actually killed server-side rather than running to completion while
+ # Superset's cancel_query() call merely returned True.
+ assert elapsed_seconds[0] < 15
+
+ check_conn = engine.raw_connection()
+ check_cursor = check_conn.cursor()
+ check_cursor.execute(
+ "SELECT count() FROM system.processes WHERE query_id = %(query_id)s",
+ {"query_id": cancel_query_id},
+ )
+ assert check_cursor.fetchone()[0] == 0
diff --git a/tests/unit_tests/db_engine_specs/test_clickhouse.py b/tests/unit_tests/db_engine_specs/test_clickhouse.py
index ac22b949cef..2b7b38bfc54 100644
--- a/tests/unit_tests/db_engine_specs/test_clickhouse.py
+++ b/tests/unit_tests/db_engine_specs/test_clickhouse.py
@@ -765,3 +765,139 @@ def test_multivalue_contains_any_numeric_coercion_sql() -> None:
# element type) and confirm the emitted array literal is numeric.
expr = spec.array_contains_any(column("scores"), [5, 6])
assert _compile(expr) == "hasAny(scores, array(5, 6))"
+
+
+def test_get_cancel_query_id_mints_a_uuid_without_a_round_trip() -> None:
+ """
+ No SQL should be issued to obtain the id: ClickHouse never hands one
+ back, the id is entirely client-chosen (verified against a real server
+ in tests/testcontainers/db_engine_specs/test_clickhouse.py).
+ """
+ import uuid
+
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseConnectEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ cursor_mock = Mock()
+ cancel_query_id = spec.get_cancel_query_id(cursor_mock, Query())
+
+ assert cancel_query_id is not None
+ assert uuid.UUID(cancel_query_id).version == 4
+ cursor_mock.execute.assert_not_called()
+
+
+def test_execute_with_cursor_forwards_recorded_cancel_id_as_query_id_setting() -> None:
+ """
+ `execute_with_cursor` is the only place that can thread the id recorded
+ by `get_cancel_query_id` into the driver -- the base `execute`/
+ `execute_with_cursor` pair (superset/db_engine_specs/base.py) never
+ forwards per-query kwargs to `cursor.execute()`. Without this, the id
+ Superset records in `query.extra` would never reach the server, and
+ `KILL QUERY WHERE query_id = ...` would have nothing real to kill.
+ """
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseConnectEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ query = Query()
+ query.id = 1
+ query.set_extra_json_key("cancel_query", "d14b9e5b-47ec-478e-bc7c-f8b56be98576")
+ cursor_mock = Mock()
+
+ spec.execute_with_cursor(cursor_mock, "SELECT 1", query)
+
+ cursor_mock.execute.assert_called_once_with(
+ "SELECT 1",
+ settings={"query_id": "d14b9e5b-47ec-478e-bc7c-f8b56be98576"},
+ )
+
+
+def test_execute_with_cursor_omits_settings_when_no_cancel_id_recorded() -> None:
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseConnectEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ query = Query()
+ query.id = 1
+ cursor_mock = Mock()
+
+ spec.execute_with_cursor(cursor_mock, "SELECT 1", query)
+
+ cursor_mock.execute.assert_called_once_with("SELECT 1", settings=None)
+
+
+def test_cancel_query_issues_kill_query_for_the_recorded_id() -> None:
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseConnectEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ cursor_mock = Mock()
+ cancel_query_id = "d14b9e5b-47ec-478e-bc7c-f8b56be98576"
+
+ assert spec.cancel_query(cursor_mock, Query(), cancel_query_id) is True
+ cursor_mock.execute.assert_called_once_with(
+ f"KILL QUERY WHERE query_id = '{cancel_query_id}'"
+ )
+
+
+def test_cancel_query_rejects_non_uuid_ids() -> None:
+ """
+ Defense-in-depth against injection via a tampered/forged cancel id --
+ mirrors every other engine's own id-shaped validation (e.g. Postgres/
+ MySQL's `^\\d+$` for their integer ids).
+ """
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseConnectEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ cursor_mock = Mock()
+
+ assert (
+ spec.cancel_query(cursor_mock, Query(), "1; DROP TABLE system.processes")
+ is False
+ )
+ cursor_mock.execute.assert_not_called()
+
+
+def test_cancel_query_returns_false_on_driver_error() -> None:
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseConnectEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ cursor_mock = Mock()
+ cursor_mock.execute.side_effect = Exception("boom")
+
+ assert (
+ spec.cancel_query(cursor_mock, Query(), "d14b9e5b-47ec-478e-bc7c-f8b56be98576")
+ is False
+ )
+
+
+def test_legacy_clickhouse_engine_spec_has_no_cancel_support() -> None:
+ """
+ The legacy `clickhouse-sqlalchemy` driver's cursor (`ClickHouseEngineSpec`,
+ the non-recommended "clickhouse" engine) has a different `execute()`
+ signature with no `settings` kwarg, and only generates its own query id
+ internally inside `execute()` -- it isn't available beforehand the way
+ `has_query_id_before_execute` (the base default both engines use)
+ requires. Passing a `settings` kwarg to that driver's cursor would raise
+ a TypeError, so the cancel overrides below live on
+ `ClickHouseConnectEngineSpec` only; this locks in that the legacy engine
+ still falls through to the inert base defaults rather than inheriting
+ something that would break it.
+ """
+ from superset.db_engine_specs.clickhouse import ( # noqa: N813
+ ClickHouseEngineSpec as spec,
+ )
+ from superset.models.sql_lab import Query
+
+ cursor_mock = Mock()
+ assert spec.get_cancel_query_id(cursor_mock, Query()) is None
+ assert spec.cancel_query(cursor_mock, Query(), "123") is False