EnxDev commented on code in PR #44714:
URL: https://github.com/apache/superset/pull/44714#discussion_r4137507568
##########
superset/db_engine_specs/base.py:
##########
@@ -1520,10 +1520,16 @@ def fetch_data(cls, cursor: Any, limit: int | None =
None) -> list[tuple[Any, ..
if cls.arraysize:
cursor.arraysize = cls.arraysize
try:
+ # Statements that return no result set (DDL, DML) leave the cursor
+ # description empty (``None`` per PEP 249), and several DB-API
+ # drivers (mysql-connector, ibm_db, pyexasol, impyla, ...) raise on
+ # a fetch in that state instead of returning no rows.
+ description = cursor.description
Review Comment:
Nit, doesn't bite today. #42127 moved the `description` read in `get_df`
after the fetch because Spark Thrift serves placeholder metadata until the
fetch waits, and it relied on this function reading it after `fetchall()` too.
The mutator map below now keys off the pre-fetch value. Re-reading
`cursor.description` after `fetchall()` for the mutators would keep that
ordering. Only MySQL, Postgres, Presto and Trino define mutators and none of
them has the placeholder issue, which is why nothing breaks yet.
##########
superset/db_engine_specs/impala.py:
##########
@@ -128,6 +128,27 @@ def execute(
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex) from ex
+ @classmethod
+ 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
+
+ deadline = time.monotonic() + app.config["SQLLAB_TIMEOUT"]
Review Comment:
`Database.get_df` and `SQLExecutor` call `fetch_data` right after
`execute_async` with no `handle_cursor` in between, so for charts, reports and
the executor this loop is where the whole Impala query runs. `fetchall()` used
to wait as long as the query took, so an Impala chart that needs 45s now gets
cancelled with a TimeoutError at the 30s `SQLLAB_TIMEOUT` default.
Could the wait go back to unbounded here (the webserver and Celery limits
already cover it), or take its limit from the caller? #44737 only fixes the SQL
Lab polling, so charts keep the cap either way, and the new docs section would
need to follow.
--
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]
