aminghadersohi commented on code in PR #45232:
URL: https://github.com/apache/superset/pull/45232#discussion_r4267805409
##########
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:
Valid. If a Stop committed after `check_stopped()` but while `conn.commit()`
ran, the mutation/CTAS was durable but the post-commit re-check returned the
stopped payload and `_commit_query_unless_stopped` refused to write SUCCESS.
Fixed in fbbeb9f178015b29b915dce58299c7a92a981b8e: once `conn.commit()`
succeeds, later stop checks are skipped (`_never_stopped`), and
`_store_results_in_backend` / `_publish_query_result` take `durable=True` and
write the terminal status unconditionally. A Stop that lands before the commit
still prevents the commit, as before.
Regression test:
`tests/unit_tests/sql/execution/test_celery_task.py::test_stop_during_mutation_commit_does_not_mask_committed_work`
(real SQLite metadata DB; the Stop is committed from an independent session
inside `conn.commit()`; parametrized for no results backend, backend write
success, and backend write failure). It fails on ce10e4f068 (status `stopped`)
and passes on fbbeb9f178.
Sibling paths checked:
- `superset/sql/execution/celery_task.py` `_execute_sql_statements`: fixed
(the only target-DB commit in the new path).
- `superset/sql/execution/executor.py` `execute_sql_with_cursor`: checks
stop before each statement and does not commit the target connection, not
affected.
- `superset/sql_lab.py` legacy `execute_sql_statements` (`conn.commit()`
then refresh/STOPPED check): same pattern, but that code is unchanged from
master by this PR; not fixed here.
--
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]
