Copilot commented on code in PR #45561:
URL: https://github.com/apache/superset/pull/45561#discussion_r4336059645
##########
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:
`Query` is only imported under `TYPE_CHECKING`, but it’s used in
runtime-visible annotations (`query: Query | None` and `) -> Query`). If this
module does not have postponed annotation evaluation enabled (e.g., `from
__future__ import annotations`), this can raise `NameError` at import time. Fix
by either importing `Query` at runtime (outside `TYPE_CHECKING`) or by
converting these annotations to string literals (e.g., `"Query"` / `"Query |
None"`), or ensuring postponed evaluation is enabled in this file.
##########
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:
`query_id` is assigned only after a potentially-failing
`db.session.rollback()` call. If rollback raises, the function returns an error
result with `query_id=None` even though a persisted query record already
exists. To preserve the existing query ID for callers reliably, set `query_id =
query.id` before any metadata rollback/commit attempts.
##########
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:
`Query` is only imported under `TYPE_CHECKING`, but it’s used in
runtime-visible annotations (`query: Query | None` and `) -> Query`). If this
module does not have postponed annotation evaluation enabled (e.g., `from
__future__ import annotations`), this can raise `NameError` at import time. Fix
by either importing `Query` at runtime (outside `TYPE_CHECKING`) or by
converting these annotations to string literals (e.g., `"Query"` / `"Query |
None"`), or ensuring postponed evaluation is enabled in this file.
##########
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:
When `commit_query_unless_stopped(query)` returns `False`, the code assumes
`query` now reflects the winning `stopped` state (including
`error_message`/`end_time`) even though this function already mutated those
fields before the guarded commit. To make the behavior robust and
self-explanatory (and to avoid relying on implicit side effects of
`commit_query_unless_stopped`), explicitly refresh/expire `query` from the
session before reading `query.error_message` (and, if relevant,
`query.end_time/status`) and ensure any pending in-session mutations are
cleared so a later caller commit can’t flush stale failure metadata.
--
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]
