I3eka commented on code in PR #45561:
URL: https://github.com/apache/superset/pull/45561#discussion_r4377047055
##########
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:
Rechecked on 8698a1afe6. Postponed annotations are already enabled at the
top of this module. The fresh-import check confirms that the return annotation
is `"Query"` and the error-handler argument annotation is `"Query | None"`;
neither requires evaluating Query during import.
##########
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:
Rechecked on 8698a1afe6. The persisted ID is captured with
`inspect(query).identity[0]` before rollback or attribute access. This also
avoids the expired-attribute load that `query.id` could trigger. The
real-session regressions still retain the ID when rollback fails or expired
attributes cannot be reloaded.
##########
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:
Checked the helper and reran the regressions on 8698a1afe6. On its False
path, `commit_query_unless_stopped()` already performs `refresh(query,
with_for_update=True)` inside `no_autoflush`, replacing the staged failure
fields with the durable Stop row. Another refresh would duplicate that work.
The missing transaction close was real and is now fixed; the regression checks
there is no active transaction before a later caller commit and verifies that
no stale failure metadata is written.
--
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]
