aminghadersohi commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4449119864
##########
superset/db_engine_specs/impala.py:
##########
@@ -130,56 +143,289 @@ def execute(
) -> None:
try:
cursor.execute_async(query)
+ except TIME_LIMIT_EXCEPTIONS:
+ raise
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex) from ex
@classmethod
+ @guard_cursor
def fetch_data(cls, cursor: Any, limit: int | None = None) ->
list[tuple[Any, ...]]:
"""Wait for asynchronous operations using the public cursor API."""
- if callable(getattr(cursor, "execute_async", None)):
- from impala.error import Error
+ try:
+ if callable(getattr(cursor, "execute_async", None)):
+ from impala.error import Error
+
+ deadline = time.monotonic() + app.config["SQLLAB_TIMEOUT"]
+ try:
+ while cursor.is_executing():
+ if time.monotonic() >= deadline:
+ cls._cancel_operation(cursor)
+ raise TimeoutError(
+ "Timed out waiting for the Impala operation"
+ )
+ time.sleep(0.1)
+ if cursor.execution_failed():
+ # Fetching surfaces the driver's detailed asynchronous
error,
+ # including errors on statements that produce no
result set.
+ cursor.fetchall()
+ except Error as ex:
+ raise cls.get_dbapi_mapped_exception(ex) from ex
+ return super().fetch_data(cursor, limit)
+ except TIME_LIMIT_EXCEPTIONS:
+ cls._cancel_operation_on_time_limit(cursor)
+ raise
Review Comment:
Remaining: the fetch_data time-limit handler still performs the full
cancel/close-operation/close-session sequence; the proposed cancel-only change
and its regression test have not been made.
I stopped at the dependency-recompile condition in this round's
instructions. Adding the hive/impala extras and running the lockfile-header
command returned exit 0, but changed unrelated pins: firebird-base 2.0.3 ->
1.8.0, firebird-driver 2.0.3 -> 1.10.11, googleapis-common-protos 1.66.0 ->
1.76.0, grpcio-status 1.60.1 -> 1.84.0, proto-plus 1.25.0 -> 1.29.0, protobuf
5.29.6 -> 6.33.6. It also removed backports-tarfile 1.2.0 and backports-zstd
1.6.0.
I restored both dependency files without manually editing generated pins. No
commit or push was made; the head remains
f73c0a47fcf61c8fadf58cfaf5be48767c14f5aa. Regression before/after tests, scoped
pytest, pre-commit and the sibling-path audit were not run in this round
because of that stop condition. The thread remains open for follow-up; I have
not requested review.
##########
tests/unit_tests/db_engine_specs/test_impala_transport.py:
##########
@@ -0,0 +1,916 @@
+# 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.
+"""
+Time-limit cancellation over real impyla Thrift transports.
+
+A local HiveServer2 stand-in serves impyla's binary, SASL PLAIN and HTTP
+transports. The time limit is raised right after a reply header is read, as a
+signal landing at that point would, so the rest of the reply is left in the
+client's buffers.
+"""
+
+from __future__ import annotations
+
+import signal
+import socket
+import struct
+import threading
+from collections.abc import Iterator
+from contextlib import closing
+from dataclasses import dataclass, field
+from typing import Any
+from unittest.mock import Mock, patch
+
+import pytest
+from celery.exceptions import SoftTimeLimitExceeded
+
+from superset.constants import QUERY_EARLY_CANCEL_KEY
+from superset.db_engine_specs.impala import ImpalaEngineSpec as spec # noqa:
N813
+from superset.errors import ErrorLevel, SupersetErrorType
+from superset.exceptions import SupersetTimeoutException
+from superset.sql.execution.executor import _LimitedCursor
+
+hs2 = pytest.importorskip("impala.hiveserver2")
+ttypes = pytest.importorskip("impala._thrift_gen.TCLIService.ttypes")
+TCLIService = pytest.importorskip("impala._thrift_gen.TCLIService.TCLIService")
Review Comment:
Remaining: the hive/impala extras are not yet included in the committed
development requirements, so this CI coverage gap remains.
I added both extras and used the repository's preparation step (`grep
--invert-match './superset-core' requirements/base.txt >
requirements/base-constraint.txt`) followed by the exact lockfile-header
command:
```bash
uv pip compile requirements/development.in -c
requirements/base-constraint.txt -o requirements/development.txt
```
It returned exit 0 and selected impyla 0.24.0, thrift 0.25.0 and thrift-sasl
0.4.3. However, it also changed these unrelated existing pins:
- firebird-base: 2.0.3 -> 1.8.0
- firebird-driver: 2.0.3 -> 1.10.11
- googleapis-common-protos: 1.66.0 -> 1.76.0
- grpcio-status: 1.60.1 -> 1.84.0
- proto-plus: 1.25.0 -> 1.29.0
- protobuf: 5.29.6 -> 6.33.6
It removed backports-tarfile 1.2.0 and backports-zstd 1.6.0 as well. There
was no resolver error; unrelated pin churn triggered the explicit stop
condition for this round. I preserved the generated diff locally, restored the
input and lock files, and did not hand-edit the lockfile. Transport tests on
the generated thrift version were not run; their counts are unknown for this
round. No commit or push was made; the head remains
f73c0a47fcf61c8fadf58cfaf5be48767c14f5aa. I have not resolved the thread or
requested review.
--
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]
