Migrate exasol provider to pyexasol 2 - #69431
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
|
@youdie006 This PR has been converted to draft because it does not yet meet our Pull Request quality criteria. Issues found:
What to do next:
Converting a PR to draft is not a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the Airflow Slack. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
pyexasol 2.x adds stricter typing to ExaConnection.execute and export_to_pandas; cast the hook's parameters/sql to the driver's expected types at the call sites. No public pyexasol symbols were removed.
b134946 to
18649ae
Compare
|
Thanks for the triage @potiuk. I've rebased onto current Local checks on the rebased branch:
Marked ready for review. Let me know if anything else comes up. |
potiuk
left a comment
There was a problem hiding this comment.
Thanks for picking this up — removing the <2 cap is genuinely wanted (#69123) and the rebase onto current main is appreciated.
My concern is that this satisfies mypy rather than actually migrating. Every incompatibility surfaced by pyexasol 2.x's stricter types is silenced with a cast(...) that asserts something the signatures say isn't true — so the runtime mismatch the <2 cap was protecting against is still there, just no longer visible to the type checker. Details inline.
There's also a release-mechanics gap: pyexasol>=2 is a hard floor on a provider that supports apache-airflow>=2.11.0, so anyone currently on pyexasol 1.x breaks on upgrade. That's a breaking change for a provider sitting at 4.10.4 — it needs a major version bump and an entry under the Changelog header in providers/exasol/docs/changelog.rst explaining the required user action. (No newsfragment — providers don't use those.)
And nothing in providers/exasol/tests/ changed. A major dependency migration with zero test movement is hard to gain confidence in; at minimum the parameter-passing paths below want coverage against the 2.x API.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
| 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, |
There was a problem hiding this 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
| """ | ||
| 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) |
There was a problem hiding this 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
| # 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", |
There was a problem hiding this 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
…hange docs + tests Replace the cast(...) hacks with runtime handling at the boundary, per review: - _serialize_query_params: convert Mapping parameters to a plain dict and reject positional sequences with a clear TypeError (Exasol binds named parameters only) instead of casting an unsupported value to dict. - _execute_statements: handle the sql list branch the way run() does (execute sequentially, fetch from the last cursor) so get_records/get_first keep the base str | list[str] contract without casting sql to str. - run() and _get_pandas_df route parameters through _serialize_query_params. Also: - pyproject: keep the upper bound with pyexasol>=2,<3. - Bump the provider to 5.0.0 (major) and document the breaking change in changelog.rst under the Changelog header. - Add tests for the parameter-passing paths and list-sql handling.
|
Thanks for the careful review @potiuk — reworked in 761fb2b.
Upper bound. Restored: Version + changelog. Bumped to Tests. Added coverage for the mapping/positional parameter paths and the list-
|
Closes #69123.
Migrates the Exasol provider to
pyexasol2 and addresses @potiuk's review.Dependency
providers/exasol/pyproject.toml:pyexasol>=0.26.0,<2→pyexasol>=2,<3(keep an upper bound).Honest type handling (no more
cast)pyexasol 2 ships a
py.typedmarker, soExaConnection.execute/export_to_pandasare now typed as(query: str, query_params: dict | None). Instead of casting the hook's broadersql/parametersto the driver types (which lies to mypy), the hook now handles the mismatch at runtime:_serialize_query_params: converts aMappingto a plaindictand raises a clearTypeErrorfor a positional sequence. Exasol binds named parameters only, so a positional sequence could never have worked — it now fails at the boundary with a helpful message instead of deep inside the driver._execute_statements: resolvessqlthe wayrun()does — astris one statement, alist[str]is executed sequentially and records are fetched from the last cursor. This keeps the baseget_records/get_firststr | list[str]contract (a list has been valid input since Fix UnboundLocalError when sql is empty list in ExasolHook #23812) without castingsqltostr.run()and_get_pandas_dfrouteparametersthrough the same helper.Breaking change + version
Breaking changesnote under theChangelogheader:pyexasol1.x support is dropped, and a positionalparameterssequence now raises.ExasolHook's public API (run/get_records/get_first/ pandas export arguments) is otherwise unchanged.Tests
providers/exasol/tests/unit/exasol/hooks/test_exasol.pyfor the parameter-passing paths (mapping forwarded as adict; positional sequence rejected) and the list-sqlhandling.test_run_with_parametersnow uses a mapping, since its previous positional tuple is rejected by design.Verification
ruff format --checkis clean and there are no newruff checkfindings on the changed files; the boundary helpers were unit-checked in isolation. I could not stand up the full Airflow toolchain in my environment this round, so the integrated mypy and provider unit tests run in CI here — I am watching them.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines
Drafted-by: Claude Code (Opus 4.8) (no human review before posting)
Important
Maintainer triage note for @youdie006 - by
@potiuk- 2026-07-11 14:59 UTCHelpful heads-up from the maintainers — please address before this PR can be reviewed (see our Pull Request quality criteria):
The ball is in your court — you've been assigned to this PR. Fix the above, then mark it Ready for review.
Automated triage — may be imperfect; a maintainer takes the next look.