aminghadersohi commented on code in PR #45232:
URL: https://github.com/apache/superset/pull/45232#discussion_r4268008357
##########
superset/sql/execution/celery_task.py:
##########
@@ -481,25 +544,44 @@ def _execute_sql_statements(
)
) from ex
- # Check if stopped
- if not execution_results:
- payload.update({"status": QueryStatus.STOPPED.value})
- return payload
+ # Cancelling can clear cursor.description, leaving a nonempty result
+ # list that looks like successful DML. Check intent, not just results.
+ stopped = not execution_results or check_stopped()
# Commit for mutations
- if parsed_script.has_mutation() or query.select_as_cta:
+ if not stopped and (parsed_script.has_mutation() or
query.select_as_cta):
conn.commit() # pylint: disable=consider-using-transaction
Review Comment:
Follow-up: my earlier reply in this thread overstated the fix. fbbeb9f178
only covered the success path and the `backend.set` returns-False branch. Two
things were still wrong:
1. In the Celery path, after a stop lost the race to a committed mutation or
CTAS, any later exception (`results_backend.set` raising, payload serialization
raising, `SQLLAB_PAYLOAD_MAX_MB` rejecting the results, the connection failing
to close) reached `execute_sql_task` -> `_handle_query_error`, which still
preserved the stop and ended STOPPED.
2. Legacy `superset/sql_lab.py` `execute_sql_statements` had the same race,
and I had left it out of scope. Its post-commit stop check, its results-backend
failure branch and its final status write all reported a committed mutation or
CTAS as STOPPED.
Fixed in 4b9f9365d49e371f8fbd2f0b86e98983e59d0ed4. Both executors call an
`on_commit` callback once `conn.commit()` succeeds. `execute_sql_task` and
`get_sql_results` pass `durable=True` to their error handlers in that case, so
the handlers skip the stop check and write FAILED. In the legacy success path,
the three post-commit checks no longer return STOPPED once the commit happened,
so the query ends SUCCESS or FAILED.
Regression tests (each parametrized for mutation and CTAS):
-
`tests/unit_tests/sql/execution/test_celery_task.py::test_failure_after_stop_raced_commit_reports_failed_not_stopped`:
runs `execute_sql_task` with a real SQLite metadata DB; the stop is committed
from an independent session inside `conn.commit()`. Cases:
`backend_set_raises`, `serialize_raises`, `payload_too_large`,
`connection_close_raises` (8 fail on fbbeb9f178 with status `stopped`), and
`backend_write_false` (2 passed already, kept as a control). All 10 pass on
4b9f9365d4.
-
`tests/unit_tests/sql_lab_test.py::test_get_sql_results_keeps_committed_work_when_stop_races_commit`:
the same setup through `get_sql_results`. Cases: no failure (expects SUCCESS),
`backend_write_false`, `backend_set_raises`, `serialize_raises`,
`payload_too_large`, `returned_payload_too_large`, `connection_close_raises`.
All 14 fail on fbbeb9f178 with status `stopped` and pass on 4b9f9365d4.
Siblings checked:
- superset/sql/execution/celery_task.py:415 — fixed in 4b9f9365d4
(`execute_sql_task` -> `_handle_query_error` for post-commit exceptions)
- superset/sql/execution/celery_task.py:593 — fixed in fbbeb9f178
(post-commit stop re-check; also `_finalize_successful_query` at :164)
- superset/sql/execution/celery_task.py:296 — fixed in fbbeb9f178
(`_store_results_in_backend` write-false branch; the control cases above cover
it)
- superset/sql/execution/celery_task.py:618 — fixed in fbbeb9f178
(`_publish_query_result` durable write)
- superset/sql_lab.py:275 — fixed in 4b9f9365d4 (`get_sql_results` ->
`handle_query_error` for post-commit exceptions, including the inline-results
size check at :883 that runs after the SUCCESS commit)
- superset/sql_lab.py:695 — fixed in 4b9f9365d4 (post-commit stop check)
- superset/sql_lab.py:803 — fixed in 4b9f9365d4 (results-backend
write-failure branch)
- superset/sql_lab.py:859 — fixed in 4b9f9365d4 (final SUCCESS write)
- superset/sql_lab.py:650 — not affected: the per-block `handle_query_error`
runs before `conn.commit()`, so nothing is committed yet
- superset/sql/execution/executor.py:525 — not affected:
`SQLExecutor.execute` does not commit the target connection and has no stop
check
--
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]
