I3eka commented on code in PR #45561:
URL: https://github.com/apache/superset/pull/45561#discussion_r4336288758
##########
superset/sql/execution/executor.py:
##########
@@ -577,6 +579,7 @@ def execute(
opts: QueryOptionsType = options or QueryOptionsType()
start_time = time.time()
+ query: Query | None = None
Review Comment:
This does not reproduce: the module already enables postponed annotations
[at line
57](https://github.com/apache/superset/blob/8a9f0ed4f129554e080975ab406219555733eee5/superset/sql/execution/executor.py#L57).
I checked a fresh import: `Query` is absent from the runtime globals, and the
`_create_error_result` query annotation is the string `"Query | None"`. The
import succeeds without a `NameError`. No code change is needed here.
##########
superset/sql/execution/executor.py:
##########
@@ -1166,7 +1204,7 @@ def _create_query_record(
catalog: str | None,
schema: str | None,
status: QueryStatus,
- ) -> Any:
+ ) -> Query:
Review Comment:
The return annotation is also covered by the existing [`from __future__
import
annotations`](https://github.com/apache/superset/blob/8a9f0ed4f129554e080975ab406219555733eee5/superset/sql/execution/executor.py#L57).
A fresh import confirms
`SQLExecutor._create_query_record.__annotations__["return"] == "Query"`; the
class is not resolved at import time. Keeping `Query` under `TYPE_CHECKING` is
valid here.
##########
superset/sql/execution/executor.py:
##########
@@ -1008,14 +1020,40 @@ def _create_error_result(
:param start_time: Start time for calculating execution duration
:param partial_results: Optional list of StatementResult from
successful
statements before the failure
+ :param query: Persisted audit record, if execution reached that stage
:returns: QueryResult with error status
"""
- from superset_core.queries.types import QueryResult as QueryResultType
+ from superset_core.queries.types import (
+ QueryResult as QueryResultType,
+ QueryStatus,
+ )
+
+ from superset.sql.execution.utils import commit_query_unless_stopped
+
+ query_id = None
+ if query is not None:
+ try:
+ # A failed metadata flush must be rolled back before recording
failure.
+ if not db.session.is_active:
+ db.session.rollback() # pylint:
disable=consider-using-transaction
+ query_id = query.id
+ query.status = status.value
+ query.error_message = error_message
+ query.end_time = now_as_float()
Review Comment:
Confirmed and fixed in
[8a9f0ed4f1](https://github.com/apache/superset/pull/45561/commits/8a9f0ed4f129554e080975ab406219555733eee5).
The handler captures `inspect(query).identity[0]` before metadata recovery.
Moving `query.id` alone would still fail when that attribute is expired and
reading it requires the unavailable metadata database.
The regressions cover failed rollback on a real inactive SQLite session and
failed loading of expired attributes. They verify that the original SQL error
and persisted ID survive. Four checks failed before the fix; all 18
failure-handling checks pass afterward. The full SQL execution suite passes
locally: 772 passed, 3 skipped.
##########
superset/sql/execution/executor.py:
##########
@@ -1008,14 +1020,40 @@ def _create_error_result(
:param start_time: Start time for calculating execution duration
:param partial_results: Optional list of StatementResult from
successful
statements before the failure
+ :param query: Persisted audit record, if execution reached that stage
:returns: QueryResult with error status
"""
- from superset_core.queries.types import QueryResult as QueryResultType
+ from superset_core.queries.types import (
+ QueryResult as QueryResultType,
+ QueryStatus,
+ )
+
+ from superset.sql.execution.utils import commit_query_unless_stopped
+
+ query_id = None
+ if query is not None:
+ try:
+ # A failed metadata flush must be rolled back before recording
failure.
+ if not db.session.is_active:
+ db.session.rollback() # pylint:
disable=consider-using-transaction
+ query_id = query.id
+ query.status = status.value
+ query.error_message = error_message
+ query.end_time = now_as_float()
+ if not commit_query_unless_stopped(query):
+ status = QueryStatus.STOPPED
+ error_message = query.error_message
Review Comment:
The refresh already happens in
[`commit_query_unless_stopped`](https://github.com/apache/superset/blob/8a9f0ed4f129554e080975ab406219555733eee5/superset/sql/execution/utils.py#L178-L193):
when the guarded update loses to Stop, it calls `db.session.refresh(query,
with_for_update=True)` inside `no_autoflush` before returning `False`. This
reloads the winning row and discards the pending failure fields.
[`test_execute_failure_preserves_concurrent_stop`](https://github.com/apache/superset/blob/8a9f0ed4f129554e080975ab406219555733eee5/tests/unit_tests/sql/execution/test_executor.py#L1019-L1103)
accepts Stop through a separate SQLite session, performs another caller commit
after failure handling, then verifies status, end time, error message, and the
cancellation flag through a fresh session. All three cases (SQL error, timeout,
OAuth error) pass. A second refresh in this caller would duplicate the shared
helper's work.
--
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]
