I3eka commented on code in PR #45561:
URL: https://github.com/apache/superset/pull/45561#discussion_r4335880497
##########
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 raise `NameError`. The module already has `from __future__
import annotations` at line 57, unchanged from master. Function-local variable
annotations are also not evaluated at runtime, even without that future import.
I verified the module imports with `Query` absent from its runtime globals, and
the execution tests exercise this line successfully. Keeping the type-only
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:
Postponed annotation evaluation is already enabled by `from __future__
import annotations` at line 57. I checked this directly:
`SQLExecutor._create_query_record.__annotations__["return"]` is the string
`"Query"`, and the module imports with no runtime `Query` binding. There is no
runtime lookup here, so no change is needed.
##########
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:
Confirmed and fixed in `85257270c9`. A regression using real SQLite sessions
reproduced the loss of another row's pending edit and a pending insert on SQL
failure. Finalization now rolls back first only when `db.session.is_active` is
false. The new tests verify that healthy pending work survives and that a real
failed metadata flush is still recovered. The OAuth-error and concurrent-Stop
regressions also pass.
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -763,6 +764,209 @@ def test_execute_custom_timeout(
)
[email protected]("failure", ["database", "timeout", "oauth"])
+def test_execute_failure_finishes_query_record(
+ mocker: MockerFixture,
+ database: Database,
+ app_context: None,
+ mock_query: MagicMock,
+ mock_db_session: MagicMock,
+ default_sql_config: None,
+ failure: str,
+) -> None:
+ """An execution failure must not leave its audit record running."""
+ from superset.errors import ErrorLevel, SupersetErrorType
+ from superset.exceptions import OAuth2Error, SupersetTimeoutException
+ from superset.sql.execution.executor import SQLExecutor
+
+ errors = {
+ "database": RuntimeError("Query failed"),
+ "timeout": SupersetTimeoutException(
+ error_type=SupersetErrorType.GENERIC_BACKEND_ERROR,
+ message="Query timed out",
+ level=ErrorLevel.ERROR,
+ ),
+ "oauth": OAuth2Error("Authentication required"),
+ }
+ mock_query.status = "running"
+ mocker.patch.object(SQLExecutor, "_create_query_record",
return_value=mock_query)
+ mocker.patch.object(SQLExecutor, "_try_get_cached_result",
return_value=None)
+ mocker.patch.object(SQLExecutor, "_execute_statements",
side_effect=errors[failure])
+ mocker.patch(
+ "superset.sql.execution.executor.now_as_float",
+ return_value=1234000.0,
+ create=True,
+ )
Review Comment:
`create=True` is intentional here: this regression also runs against
unmodified upstream, where the executor did not import `now_as_float`. It lets
the test reach the stale-query-status failure instead of failing while setting
up the patch. A wrong patch target cannot make this test pass: `assert
mock_query.end_time == 1234000.0` requires the patched clock value to reach the
audit record. Keeping it for that regression use case.
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -763,6 +764,209 @@ def test_execute_custom_timeout(
)
[email protected]("failure", ["database", "timeout", "oauth"])
+def test_execute_failure_finishes_query_record(
+ mocker: MockerFixture,
+ database: Database,
+ app_context: None,
+ mock_query: MagicMock,
+ mock_db_session: MagicMock,
+ default_sql_config: None,
+ failure: str,
+) -> None:
+ """An execution failure must not leave its audit record running."""
+ from superset.errors import ErrorLevel, SupersetErrorType
+ from superset.exceptions import OAuth2Error, SupersetTimeoutException
+ from superset.sql.execution.executor import SQLExecutor
+
+ errors = {
+ "database": RuntimeError("Query failed"),
+ "timeout": SupersetTimeoutException(
+ error_type=SupersetErrorType.GENERIC_BACKEND_ERROR,
+ message="Query timed out",
+ level=ErrorLevel.ERROR,
+ ),
+ "oauth": OAuth2Error("Authentication required"),
+ }
+ mock_query.status = "running"
+ mocker.patch.object(SQLExecutor, "_create_query_record",
return_value=mock_query)
+ mocker.patch.object(SQLExecutor, "_try_get_cached_result",
return_value=None)
+ mocker.patch.object(SQLExecutor, "_execute_statements",
side_effect=errors[failure])
+ mocker.patch(
+ "superset.sql.execution.executor.now_as_float",
+ return_value=1234000.0,
+ create=True,
+ )
+ expected_status = (
+ QueryStatus.TIMED_OUT if failure == "timeout" else QueryStatus.FAILED
+ )
+
+ if failure == "oauth":
+ with pytest.raises(OAuth2Error):
+ database.execute("SELECT 1")
+ else:
+ result = database.execute("SELECT 1")
+ assert result.status == expected_status
+ assert result.query_id == mock_query.id
+ assert result.error_message == mock_query.error_message
+
+ assert mock_query.status == expected_status.value
+ assert mock_query.error_message
+ assert mock_query.end_time == 1234000.0
+ mock_db_session.rollback.assert_called_once()
+ mock_db_session.commit.assert_called_once()
+
+
[email protected]("failure", ["database", "timeout", "oauth",
"oauth_redirect"])
[email protected]("metadata_failure", ["rollback", "commit"])
+def test_execute_preserves_error_when_failure_recording_fails(
+ mocker: MockerFixture,
+ database: Database,
+ app_context: None,
+ mock_query: MagicMock,
+ mock_db_session: MagicMock,
+ default_sql_config: None,
+ failure: str,
+ metadata_failure: str,
+ caplog: pytest.LogCaptureFixture,
+) -> None:
+ """A metadata outage must not replace the execution error or OAuth
redirect."""
+ from superset.errors import ErrorLevel, SupersetErrorType
+ from superset.exceptions import (
+ OAuth2Error,
+ OAuth2RedirectError,
+ SupersetTimeoutException,
+ )
+ from superset.sql.execution.executor import SQLExecutor
+
+ error = {
+ "database": RuntimeError("Query failed"),
+ "timeout": SupersetTimeoutException(
+ error_type=SupersetErrorType.GENERIC_BACKEND_ERROR,
+ message="Query timed out",
+ level=ErrorLevel.ERROR,
+ ),
+ "oauth": OAuth2Error("Authentication required"),
+ "oauth_redirect": OAuth2RedirectError(
+ "https://example.com/auth";, "tab-id",
"https://example.com/callback";
+ ),
+ }[failure]
+ mocker.patch.object(SQLExecutor, "_create_query_record",
return_value=mock_query)
+ mocker.patch.object(SQLExecutor, "_try_get_cached_result",
return_value=None)
+ mocker.patch.object(SQLExecutor, "_execute_statements", side_effect=error)
+ getattr(mock_db_session, metadata_failure).side_effect = RuntimeError(
+ "Metadata unavailable"
+ )
+
+ if isinstance(error, (OAuth2Error, OAuth2RedirectError)):
+ with pytest.raises(type(error)) as caught:
+ database.execute("SELECT 1")
+ assert caught.value is error
+ else:
+ result = database.execute("SELECT 1")
+ assert result.status == (
+ QueryStatus.TIMED_OUT if failure == "timeout" else
QueryStatus.FAILED
+ )
+ assert result.error_message == (
+ "Query exceeded the timeout limit"
+ if failure == "timeout"
+ else "sqlite error: Query failed"
+ )
+ if metadata_failure == "commit":
+ assert result.query_id == mock_query.id
+
+ assert mock_db_session.rollback.call_count == 2
+ assert "Failed to persist SQL query error" in caplog.text
Review Comment:
Confirmed as a test reliability issue. Running these eight cases with
`--log-level=ERROR` reproduced eight failures because the warning was absent
from `caplog.text`. Fixed in `85257270c9` by explicitly setting WARNING capture
for `superset.sql.execution.executor`. All eight cases now pass with the global
level still set to ERROR.
--
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]
