potiuk commented on code in PR #69431:
URL: https://github.com/apache/airflow/pull/69431#discussion_r3680696154
##########
providers/exasol/src/airflow/providers/exasol/hooks/exasol.py:
##########
@@ -188,7 +188,10 @@ def get_records(
sql statements to execute
:param parameters: The parameters to render the SQL query with.
"""
- with closing(self.get_conn()) as conn, closing(conn.execute(sql,
parameters)) as cur:
+ with (
+ closing(self.get_conn()) as conn,
+ closing(conn.execute(cast("str", sql), cast("dict | None",
parameters))) as cur,
Review Comment:
`get_records` is declared `sql: str | list[str]`, so `cast("str", sql)`
tells mypy the list case cannot happen when the signature explicitly allows it.
To be fair this isn't a regression — `conn.execute(sql, ...)` already passed
a list straight through to pyexasol before this PR. But that pre-existing
inconsistency is exactly what the stricter 2.x types just surfaced, and the
cast re-buries it. Either narrow the signature to `str` (if lists were never
really supported here), or handle the list branch explicitly the way `run()`
does with `sql_list`.
Same applies to `get_first` just below.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/exasol/src/airflow/providers/exasol/hooks/exasol.py:
##########
@@ -145,7 +145,7 @@ def _get_pandas_df(
``pyexasol.ExaConnection.export_to_pandas``.
"""
with closing(self.get_conn()) as conn:
- df = conn.export_to_pandas(sql, query_params=parameters, **kwargs)
+ df = conn.export_to_pandas(sql, query_params=cast("dict | None",
parameters), **kwargs)
Review Comment:
`parameters` is `Iterable | Mapping[str, Any] | None`, so `cast("dict |
None", ...)` is a stronger claim than the signature supports: a tuple or list
of positional params satisfies `Iterable` and would reach pyexasol as a
non-dict.
If pyexasol 2.x genuinely only accepts a mapping, the honest fix is to
tighten the parameter type (and convert or reject sequences at the boundary)
rather than assert the narrower type at the call site.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/exasol/pyproject.toml:
##########
@@ -62,9 +62,7 @@ dependencies = [
"apache-airflow>=2.11.0",
"apache-airflow-providers-common-compat>=1.12.0",
"apache-airflow-providers-common-sql>=1.32.0",
- # Capped to 1.x: pyexasol 2.x ships stricter types that break the exasol
hook.
- # Remove the cap after migrating; tracked at
https://github.com/apache/airflow/issues/69123
- "pyexasol>=0.26.0,<2",
+ "pyexasol>=2",
Review Comment:
`pyexasol>=2` drops the upper bound entirely — worth keeping one
(`pyexasol>=2,<3`) so the next major doesn't repeat this exercise unannounced.
Also, the removed comment pointed at the tracking issue; once this lands,
#69123 should be closed as part of the PR (`closes: #69123` in the description).
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
