I3eka commented on code in PR #45561:
URL: https://github.com/apache/superset/pull/45561#discussion_r4377044616
##########
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:
Checked against 8698a1afe6; this does not raise NameError. The module
already has `from __future__ import annotations`, and function-local variable
annotations do not evaluate `Query` at runtime. A fresh-process import succeeds
while `Query` is absent from the module's runtime globals. No additional
runtime import is needed.
##########
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:
Checked against 8698a1afe6; the module already enables postponed
annotations. In a fresh process, the module imports without a runtime `Query`
binding and `SQLExecutor._create_query_record.__annotations__["return"]` is the
string `"Query"`. Keeping the TYPE_CHECKING import is correct here.
##########
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 8698a1afe6. The initial rollback is limited to an inactive
session, and now also recovers a failed Query INSERT before a Query object
reaches the caller. SQLite regressions verify that a healthy session's
unrelated pending edit and insert survive ordinary execution failure, while a
genuinely failed flush is recovered.
##########
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:
Removed `create=True` in 8698a1afe6. The earlier version tolerated the
missing symbol when demonstrating the failure against unmodified master; the
current test targets this branch, where the imported attribute must exist. A
misspelled patch target now fails immediately.
##########
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:
Rechecked on 8698a1afe6. The test explicitly sets WARNING capture for
`superset.sql.execution.executor` before executing. All eight
persistence-failure cases also pass with pytest started with
`--log-level=ERROR`.
##########
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:
Rechecked on 8698a1afe6; this is covered by the existing `from __future__
import annotations`. A fresh-process import succeeds without `Query` in the
module globals, and the method annotations remain strings. The local variable
annotation does not evaluate the type either, so no runtime import is required.
--
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]
