aminghadersohi commented on code in PR #45845:
URL: https://github.com/apache/superset/pull/45845#discussion_r4449191425
##########
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:
Fixed in 1cb704263a5b816912fa7b562f84b857ad140a38. I took the cancel-only
option: a clean-transport `fetch_data` time limit sends
`cancel_operation(reset_state=False)`, then abandons locally through the guard.
It does not send CloseOperation or CloseSession, including from subsequent
cursor finalization/connection release. `handle_cursor` retains full
cancellation/close cleanup.
Regression: `test_fetch_limit_cancels_without_close_rpcs` covers polling and
row-fetch limits, including the soft-limit and TimerTimeout interrupt paths.
With the final tests retained and impala.py restored from
f73c0a47fcf61c8fadf58cfaf5be48767c14f5aa, all 4 cases failed; restoring the
change makes all 4 pass. This negative control and positive run both used
thrift 0.25.0. The accompanying workflow regression also changed from 1 failed
to 1 passed (`5 failed` before / `5 passed` after overall).
Regression command:
```bash
python -m pytest -q tests/unit_tests/db_engine_specs/test_impala.py
tests/unit_tests/scripts/python_unit_transport_test.py -k
'test_fetch_limit_cancels_without_close_rpcs or
test_python_unit_workflow_installs_transport_extras' -m 'not manual and not
llm_compliance'
```
The scoped CI-flags run (Impala/Hive transport and mock suites,
pending-operation tests, Base, Presto, Trino, executor and workflow tests)
reported `1228 passed, 3 xfailed`, exit 0. The real-driver transport files
account for 265 Impala and 66 Hive passes, with zero skipped cases. The
clean-transport transport regression verifies CancelOperation is the last
request after the fetch limit; the handle_cursor cases still verify full
cleanup. Staged-file/branch-file pre-commit and explicit mypy checks each
exited 0.
Remaining trade-off: CancelOperation itself is not bounded by this handler.
The fetch_data docstring and configuration documentation recommend a driver
network timeout or a larger `ALERT_REPORTS_WORKING_TIME_OUT_LAG` for Impala
alerts/reports. No live Impala timing/admission-slot measurement was performed
in this round.
Sibling search: `rg -n 'def
fetch_data|TIME_LIMIT_EXCEPTIONS|_cancel_operation_on_time_limit|cancel_operation\(|close_operation\('
superset/db_engine_specs superset/utils/hive.py superset/sql/execution`, plus
`rg -n 'def handle_cursor|def fetch_data|cursor.cancel|_cancel_requested_query'
superset/db_engine_specs/{hive,presto,trino}.py`; I read each fetch override
and the polling/cleanup paths.
Siblings checked:
- superset/db_engine_specs/impala.py:181 — fixed in 1cb704263a
- superset/db_engine_specs/impala.py:413 — fixed in 1cb704263a
- superset/db_engine_specs/impala.py:169 — not affected: the separate local
polling deadline retains its existing cleanup; only the signal/time-limit
handler is shortened as requested.
- superset/db_engine_specs/impala.py:383 — not affected: full cleanup
remains available for execution-status polling and explicit Stop.
- superset/db_engine_specs/impala.py:397 — not affected: an interrupted
cleanup RPC abandons locally and is not retried.
- superset/db_engine_specs/impala.py:507 — not affected: handle_cursor
intentionally keeps full cancel/operation/session cleanup.
- superset/db_engine_specs/impala.py:490 — not affected: GetLog limits
propagate to the handle_cursor handler, whose full close is intentionally
retained.
- superset/db_engine_specs/impala.py:146 — not affected: execute re-raises
without adding a fetch cleanup sequence.
- superset/db_engine_specs/impala.py:290 — not affected: transport reset is
local cleanup, not another warehouse cancellation/close RPC.
- superset/db_engine_specs/hive.py:210 — not affected: catches
ProgrammingError only and does not add cancel/close RPCs to a time-limit
handler.
- superset/db_engine_specs/hive.py:493 — not affected: Hive polling cancels
explicit Stop; no equivalent fetch-time multi-RPC cleanup handler.
- superset/db_engine_specs/hive.py:527 — not affected: log time limits only
propagate; no added cancellation/close sequence.
- superset/db_engine_specs/presto.py:1415 — not affected: uses its own
polling/Stop cancellation, inherits Base fetch_data and has no Impala-style
fetch-time cleanup sequence.
- superset/db_engine_specs/trino.py:331 — not affected: uses its own
polling/Stop path, inherits Base fetch_data and has no Impala-style fetch-time
cleanup sequence.
- superset/db_engine_specs/trino.py:406 — not affected: handles explicit
requested cancellation, not fetch time-limit cleanup.
- superset/db_engine_specs/base.py:1632 — not affected: maps fetch errors
without sending cancellation/close RPCs.
- superset/db_engine_specs/base.py:1678 — not affected: abstract
native-pagination entry point, not a time-limit cleanup handler.
- superset/db_engine_specs/base.py:2103 — not affected: delegates to the
engine callback; guard abandonment prevents downstream cleanup RPCs.
- superset/utils/hive.py:90 — not affected: interrupted generated RPCs
abandon locally before driver-internal cleanup; no additional
cancellation/close RPCs are sent.
- superset/sql/execution/executor.py:244 — not affected: truncation probes
delegate to the engine fetch method and re-raise time limits.
- superset/db_engine_specs/bigquery.py:416 — not affected: result
sampling/error fallback has no cancel/close sequence in a time-limit handler.
- superset/db_engine_specs/crate.py:99 — not affected: delegates fetching
and transforms timestamps without time-limit cleanup RPCs.
- superset/db_engine_specs/datastore.py:319 — not affected: delegates
fetching and transforms rows without time-limit cleanup RPCs.
- superset/db_engine_specs/drill.py:187 — not affected: handles a specific
RuntimeError without cancellation/close RPCs.
- superset/db_engine_specs/exasol.py:165 — not affected: delegates fetching
and converts rows without time-limit cleanup RPCs.
- superset/db_engine_specs/mssql.py:194 — not affected: delegates fetching
and converts rows without time-limit cleanup RPCs.
- superset/db_engine_specs/ocient.py:322 — not affected: removes a local
query-ID mapping on error without cancellation/close RPCs.
- superset/db_engine_specs/elasticsearch.py:274 — not affected: separate
native-pagination endpoint, not an Impala fetch-time cancellation handler.
- superset/db_engine_specs/elasticsearch.py:392 — not affected: separate
OpenDistro pagination endpoint, not an Impala fetch-time cancellation handler.
I have not resolved the thread or 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:
Fixed in 1cb704263a5b816912fa7b562f84b857ad140a38 using the workflow-install
option. The Python unit-test job installs the hive/impala extras immediately
after its existing backend setup, before pytest:
```bash
uv pip install --system -e ".[hive,impala]"
```
Neither requirements/development.in nor requirements/development.txt changed
(`git diff f73c0a47fcf61c8fadf58cfaf5be48767c14f5aa..HEAD --
requirements/development.in requirements/development.txt` is empty).
Regression: `test_python_unit_workflow_installs_transport_extras` requires
that unconditional install before the unit-test step. Retaining the added test
but restoring the workflow from f73c0a47fcf61c8fadf58cfaf5be48767c14f5aa
produces 1 failure; restoring the change produces 1 pass. It was included in
the regression selection reported as `5 failed` before / `5 passed` after, with
the four cancel-only regression cases.
I installed the same extras locally with `uv pip install --python
/tmp/45845-r9-venv/bin/python -e '.[hive,impala]'` (exit 0). Import metadata
and the loaded module paths confirm impyla 0.24.0, thrift 0.25.0, thrift-sasl
0.4.3 and PyHive 0.7.0 in that environment. The CI-flags scoped pytest run and
its JUnit report show:
| File | Passed | Skipped |
|---|---:|---:|
| test_impala_transport.py | 265 | 0 |
| test_hive_transport.py | 66 | 0 |
| test_impala.py | 68 | 0 |
| test_hive.py | 29 | 0 |
The broader scope including sibling engines, pending-operation, executor and
workflow regressions reported `1228 passed, 3 xfailed`, exit 0.
Staged-file/branch-file pre-commit and explicit mypy checks exited 0. This is
local validation; it is not a claim that the newly triggered GitHub CI run has
finished.
Sibling search: `rg -n 'setup-backend|requirements/development|pytest'
.github/workflows/superset-python-unittest.yml
.github/workflows/python-nightly-canary.yml
.github/actions/setup-backend/action.yml`, and driver `importorskip` sites in
both transport files.
Siblings checked:
- .github/workflows/superset-python-unittest.yml:68 — fixed in 1cb704263a
- tests/unit_tests/db_engine_specs/test_impala_transport.py:49 — fixed in
1cb704263a
- tests/unit_tests/db_engine_specs/test_hive_transport.py:42 — fixed in
1cb704263a
- .github/actions/setup-backend/action.yml:73 — not affected: the shared
development-requirements install is intentionally unchanged; the unit-test job
adds its own optional drivers afterward.
- .github/workflows/python-nightly-canary.yml:48 — not affected: separate
canary job is outside the requested unit-job-only installation change.
- requirements/development.in — not affected: unchanged; no lockfile
recompile in this approach.
- requirements/development.txt — not affected: unchanged; no generated-pin
edits.
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]
