I3eka commented on code in PR #45729:
URL: https://github.com/apache/superset/pull/45729#discussion_r4402840393
##########
superset/db_engine_specs/base.py:
##########
@@ -816,6 +816,10 @@ def get_extended_aggregation_func(
# owner cancel a query whose caller has given up.
has_query_id_during_execute = False
+ # Execution owners may call cursor.cancel() from another thread when the
+ # driver exposes it. This does not enable SQL Lab's cross-process stop
path.
+ supports_cancel_on_live_cursor = False
Review Comment:
Applied in a29f77c730819674ae56a73f1d780ea7d00144c5 together with the worker
change. `BaseEngineSpec.can_cancel_live_cursor(driver, cursor)` preserves the
explicit capability flag and callable check, while allowing engine specs to
restrict cancellation by driver.
##########
superset/db_engine_specs/mssql.py:
##########
@@ -96,6 +96,7 @@ class MssqlEngineSpec(BaseEngineSpec):
max_column_name_length = 128
allows_cte_in_subquery = False
supports_multivalues_insert = True
+ supports_cancel_on_live_cursor = True
Review Comment:
Applied in a29f77c730819674ae56a73f1d780ea7d00144c5. `MssqlEngineSpec` now
requires `driver == "pyodbc"` and delegates to `super()` for the capability and
cursor checks. The tests also verify that Azure Synapse and Sybase remain
disabled through the new hook.
##########
tests/unit_tests/mcp_service/test_worker.py:
##########
@@ -418,6 +418,103 @@ def
test_query_cancellation_capture_resets_stale_id_on_failed_recapture() -> Non
assert call.cancel_query is None
[email protected]("native_cancel", [False, True])
+def test_sql_server_cancellation_uses_only_the_live_cursor(native_cancel:
bool) -> None:
+ """SQL Server cancellation requires a driver with a native cursor API."""
+ from superset.db_engine_specs.mssql import MssqlEngineSpec
+ from superset.mcp_service.worker import _active_call, warehouse_cursor
+
+ call = Mock(lock=threading.RLock(), expired=threading.Event())
+ database = Mock(id=42, db_engine_spec=MssqlEngineSpec)
+ cursor = Mock(spec=["execute", "cancel"] if native_cancel else ["execute"])
+ token = _active_call.set(call)
+ try:
+ with warehouse_cursor(database, cursor, None, None):
+ if native_cancel:
Review Comment:
Added the suggested `pytds` regression in
a29f77c730819674ae56a73f1d780ea7d00144c5. It fails before the driver check and
passes after the fix. The parameterized test also covers pyodbc with and
without `cancel()`, and rejects a mocked non-pyodbc cursor even when that
method exists. All 113 tests in the focused MSSQL, SQL-execution, MCP worker
and migration-head suites passed.
--
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]
