I3eka commented on code in PR #45561:
URL: https://github.com/apache/superset/pull/45561#discussion_r4384232656
##########
superset/sql/execution/executor.py:
##########
@@ -680,16 +683,24 @@ def execute(
QueryStatus.TIMED_OUT,
"Query exceeded the timeout limit",
start_time,
+ query=query,
)
except SupersetSecurityException as ex:
- return self._create_error_result(QueryStatus.FAILED, str(ex),
start_time)
- except (OAuth2RedirectError, OAuth2Error):
+ return self._create_error_result(
+ QueryStatus.FAILED, str(ex), start_time, query=query
+ )
+ except (OAuth2RedirectError, OAuth2Error) as ex:
# Let OAuth2 exceptions propagate so callers (MCP, API) can
# handle them with context-appropriate responses.
+ self._create_error_result(
+ QueryStatus.FAILED, str(ex), start_time, query=query
+ )
raise
Review Comment:
Rechecked on c8c079654469 after merging Apache master 1220ec01ef85.
`_create_error_result` catches metadata persistence failures and suppresses
recovery rollback failures. The OAuth handlers still re-raise the original
exception object; the regression covers both commit and rollback failures. No
further code change is needed for this case.
##########
superset/sql/execution/executor.py:
##########
@@ -1008,14 +1020,24 @@ 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
+ if query is not None:
+ # A failed metadata flush must be rolled back before recording
failure.
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ query.status = status.value
Review Comment:
Rechecked on c8c079654469. Failure finalization uses
`commit_query_unless_stopped`, whose conditional write preserves an accepted
Stop. The regression commits Stop through a second SQLAlchemy session between
the read and failure write, then verifies status, end time and cancellation
metadata even after a later caller commit. It passes.
##########
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 is a false positive on c8c079654469: the module already has `from
__future__ import annotations`, and a function-local variable annotation does
not evaluate `Query` at runtime. A fresh-process import succeeds with no
`Query` binding in the module globals. The executor tests also pass without
adding a runtime import.
##########
superset/sql/execution/executor.py:
##########
@@ -1166,7 +1203,7 @@ def _create_query_record(
catalog: str | None,
schema: str | None,
status: QueryStatus,
- ) -> Any:
+ ) -> Query:
Review Comment:
This is covered by the existing `from __future__ import annotations`. On
c8c079654469, a fresh-process check confirms that
`_create_query_record.__annotations__["return"]` is the string `"Query"`, while
`Query` is absent from module globals. No additional import or quoting is
required.
##########
superset/sql/execution/executor.py:
##########
@@ -1008,14 +1020,39 @@ 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.
+ db.session.rollback() # pylint:
disable=consider-using-transaction
+ query_id = query.id
Review Comment:
Rechecked on c8c079654469. The initial rollback runs only when
`db.session.is_active` is false. Real-session regressions cover both a healthy
transaction with unrelated pending changes and a failed flush requiring
rollback; they pass. The exception handler still attempts rollback if
finalization itself fails.
--
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]
