bito-code-review[bot] commented on code in PR #44737:
URL: https://github.com/apache/superset/pull/44737#discussion_r4140727913
##########
superset/db_engine_specs/impala.py:
##########
@@ -105,14 +107,7 @@ def get_schema_names(cls, inspector: Inspector) ->
set[str]:
@classmethod
def has_implicit_cancel(cls) -> bool:
- """
- Return True if the live cursor handles the implicit cancelation of the
query,
- False otherwise.
-
- :return: Whether the live cursor implicitly cancels the query
- :see: handle_cursor
- """
-
+ """Keep HTTP cancellation independent of the live cursor polling
loop."""
return False
Review Comment:
<!-- Bito Reply -->
The suggestion is valid and addresses a critical issue where Impala queries
could not be stopped because the cancellation ID was not available before
execution. By changing the method to return False, the system correctly falls
back to the explicit-ID path, which is now supported by the early-cancel flag
implementation in the associated changes.
**superset/db_engine_specs/impala.py**
```
@classmethod
def has_implicit_cancel(cls) -> bool:
"""Keep HTTP cancellation independent of the live cursor polling
loop."""
return False
```
##########
tests/unit_tests/db_engine_specs/test_impala_pending.py:
##########
@@ -0,0 +1,137 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from flask import Flask
+
+from superset.constants import QUERY_CANCEL_KEY, QUERY_EARLY_CANCEL_KEY
+from superset.db_engine_specs.impala import ImpalaEngineSpec
+from superset.sql.execution.executor import SQLExecutor
+from superset.sql_lab import cancel_query
+
+
[email protected](
+ "state", ["PENDING_STATE", "INITIALIZED_STATE", "RUNNING_STATE"]
+)
+def test_cancel_unfinished_operation(state: str) -> None:
+ """An early stop must reach the live operation in every unfinished
state."""
+ query = Mock(id=1, extra={QUERY_EARLY_CANCEL_KEY: True}, progress=0)
+ cursor = Mock()
+ cursor.status.side_effect = [state, "FINISHED_STATE"]
+ with patch("superset.db_engine_specs.impala.db") as db:
+ db.session.query.return_value.filter_by.return_value.one.return_value
= query
+ ImpalaEngineSpec.handle_cursor(cursor, query)
+ cursor.cancel_operation.assert_called_once_with()
+ cursor.close_operation.assert_called_once_with()
+ cursor.close.assert_called_once_with()
+ cursor.get_log.assert_not_called()
+
+
+def test_pending_operation_is_polled_without_progress() -> None:
+ """Pending work must not escape the cancel/progress polling loop."""
+ app = Flask(__name__)
+ app.config["DB_POLL_INTERVAL_SECONDS"] = {"impala": 0}
+ query = Mock(id=1, extra={}, progress=0)
+ cursor = Mock()
+ cursor.status.side_effect = [
+ "PENDING_STATE",
+ "INITIALIZED_STATE",
+ "RUNNING_STATE",
+ "FINISHED_STATE",
+ ]
+ cursor.get_log.return_value = "Query abc: 25% Complete"
+ with app.app_context(), patch("superset.db_engine_specs.impala.db") as db:
+ db.session.query.return_value.filter_by.return_value.one.return_value
= query
+ ImpalaEngineSpec.handle_cursor(cursor, query)
+ assert cursor.status.call_count == 4
+ cursor.get_log.assert_called_once_with()
+ assert query.progress == 25
+ cursor.cancel_operation.assert_not_called()
+
+
[email protected]("use_executor", [False, True])
+def test_stop_with_cancel_id_uses_http(use_executor: bool) -> None:
+ """Stop must reach Impala even when no worker is polling the live
cursor."""
+ app = Flask(__name__)
+ app.config["IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS"] = True
+ cancel_id = "0123456789abcdef:fedcba9876543210"
+ query = MagicMock(extra={QUERY_CANCEL_KEY: cancel_id})
Review Comment:
<!-- Bito Reply -->
The suggestion to add explicit type annotations to local variables and mocks
in the test file is appropriate. Applying these annotations improves code
readability and maintainability by clearly defining the expected types for
`app`, `cancel_id`, and `query` within the test scope.
**tests/unit_tests/db_engine_specs/test_impala_pending.py**
```
app: Flask = Flask(__name__)
app.config["IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS"] = True
cancel_id: str = "0123456789abcdef:fedcba9876543210"
query: MagicMock = MagicMock(extra={QUERY_CANCEL_KEY: cancel_id})
```
--
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]
