aminghadersohi commented on code in PR #45283:
URL: https://github.com/apache/superset/pull/45283#discussion_r4281406719
##########
superset/sql/execution/executor.py:
##########
@@ -1427,15 +1428,22 @@ def _get_async_query_result(query_id: int) -> Any:
)
@staticmethod
+ @transaction()
def _cancel_async_query(query_id: int, database: Database) -> bool:
- """Cancel an async query."""
+ """Cancel an async query and persist intent without replacing a
result."""
from superset.models.sql_lab import Query as QueryModel
+ from superset.sql.execution.utils import set_query_stopped
+ from superset.utils.dates import now_as_float
query =
db.session.query(QueryModel).filter_by(id=query_id).one_or_none()
if not query:
return False
- return SQLExecutor._cancel_query(database, query)
+ with db.session.no_autoflush:
+ if not SQLExecutor._cancel_query(database, query):
+ return False
+ set_query_stopped(query, now_as_float())
+ return True
Review Comment:
Fixed in 7de1b2c156d2d26f7b4c66c658345d305e4e0fe8: `_cancel_async_query`
returns `set_query_stopped(...)`, and the handle forwards that boolean.
Regression:
`test_engine_early_cancel_preserves_concurrently_committed_terminal_metadata`
asserts `is False` for SUCCESS/FAILED/TIMED_OUT. I restored the dropped return
locally: 6 async-handle cases failed; with the committed implementation
restored, `pytest tests/unit_tests/daos/test_query_dao.py` passed all 46 tests.
Siblings checked:
- superset/sql/execution/executor.py:1275 — fixed in
7de1b2c156d2d26f7b4c66c658345d305e4e0fe8
- superset/sql/execution/executor.py:1314 — not affected: cached-result
handles already return False
- superset/daos/query.py:65 — not affected: returns a Query rather than a
cancellation boolean; terminal metadata preservation is covered by the DAO
variants of the same regression
- superset-core/src/superset_core/queries/types.py:164 — not affected:
interface method is replaced by the executor closure
##########
superset/queries/api.py:
##########
@@ -275,12 +276,21 @@ def stop_query(self) -> FlaskResponse:
$ref: '#/components/responses/401'
404:
$ref: '#/components/responses/404'
+ 422:
+ description: >
+ Query could not be stopped; includes query if already complete
500:
$ref: '#/components/responses/500'
"""
try:
body = self.stop_query_schema.load(request.json)
- QueryDAO.stop_query(body["client_id"])
+ query = QueryDAO.stop_query(body["client_id"])
+ if query.status != QueryStatus.STOPPED:
Review Comment:
Fixed the integration coverage and upgrade note in
ae40a754f3b05f1e25cb32d933ee57e733e84ae8. `test_stop_query` explicitly inserts
RUNNING; `test_stop_query_already_finished` asserts the complete 422 body and
unchanged SUCCESS state. `UPDATING.md:27` documents the public API change under
Next.
Regression: restoring the SUCCESS fixture made `test_stop_query` fail;
restoring the pre-7de1b2c1 API handler made `test_stop_query_already_finished`
fail (200 instead of 422). After restoring the changes, `pytest
tests/integration_tests/queries/api_tests.py -k stop_query` passed all 3 tests
against the local SQLite integration database. The PR's
`test_stop_reports_concurrently_finished_query` also passes in the 46-test DAO
suite.
Validation: staged-file pre-commit (including mypy) passed; sibling unit
suites passed 466 tests. Branch-wide pre-commit also ran, but frontend
type-checking failed with TS6305 missing built package declarations and other
TypeScript errors; I did not change unrelated files.
Siblings checked:
- tests/integration_tests/queries/api_tests.py:553 — fixed in
ae40a754f3b05f1e25cb32d933ee57e733e84ae8
- tests/integration_tests/queries/api_tests.py:535 — not affected: missing
query still returns 404
- superset-frontend/src/SqlLab/actions/sqlLab.ts:617 — not affected: already
refreshes terminal query state on 422 instead of reporting cancellation
- superset-frontend/src/SqlLab/actions/sqlLab.test.ts:651 — not affected:
generic 200 tests cover a successful stop; terminal-state 422 cases are
separately covered; action suite passed 86 tests
- tests/unit_tests/daos/test_query_dao.py:328 — not affected: already
asserts 422 for SUCCESS/FAILED/TIMED_OUT, including concurrent completion
- tests/unit_tests/dao/queries_test.py:166 — not affected: direct DAO call
asserts preserved FAILED state, not HTTP 200
- superset/mcp_service/ — not affected: no stop callers found by `rg -n
'query/stop|stop_query|stopQuery' superset/mcp_service superset/extensions
superset-core/src superset-extensions*`
- superset/extensions/ — not affected: same search found no stop callers
- superset-extensions-cli/ — not affected: same search found no stop callers
--
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]
