Require SQLAlchemy 2.0 or later for the SQLAlchemy dialect - #858
Conversation
The sqlalchemy extra and the dev dependency group declared 1.0.0, but CI tests only the locked 2.0 release, the test tree needs 2.0 at import, and the dialect relies on 2.0-only type APIs. Raise the floor to 2.0.0 and drop the documentation qualifiers that implied 1.x support. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With SQLAlchemy 2.0 required, types.DOUBLE, types.Double, and TypeEngine._variant_mapping always exist. Use them directly and drop the version-dependent test branches. Keep get_double_type as a public helper that returns types.DOUBLE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| if isinstance(left_type, types.Float) or isinstance(right_type, types.Float): | ||
| division_type = get_double_type()() | ||
| division_type: TypeEngine[Any] = types.DOUBLE() |
There was a problem hiding this comment.
Self-review round one — implementation behavior (Claude Code)
Scope: git diff d6891e9e8f4204f5e8ac4d181e7e41814e52332d..349eaeb53b52bce5e3c26db7a50a2b34c1e16982. Covers pyproject.toml, uv.lock, README.md, docs/introduction.md, docs/sqlalchemy.md, array.py, base.py, compiler.py, types.py, test_base.py, and test_types.py.
Result: FINDINGS (1, fixed).
- Removed guards:
types.Double,types.DOUBLE, andTypeEngine._variant_mappingexist in SQLAlchemy 2.0.0.- Checked on the floor itself: in an isolated SQLAlchemy 2.0.0 / Python 3.10 environment, the dialect imports,
cast(x, Double())rendersDOUBLE,AthenaArray(Integer)results decode, and a variant cast resolves.
- True division:
types.DOUBLE()is the same classget_double_type()returned on 2.x, so the casts are unchanged (test_compiler.py: 158 passed).- The explicit
TypeEngine[Any]annotation is needed because mypy no longer seesAny.
get_double_type()stays in__all__and returnstypes.DOUBLE, so imports by users keep working.- Tests:
test_reflect_table_include_columnskeeps the 2.x branch (Inspector.reflect_table) as-is.test_numeric_type_variantsnarrows the accepted reflected DOUBLE types to the DOUBLE family;ischema_names["double"]istypes.DOUBLE.- Validation: Athena, 4 passed (
-n 1); no-AWS SQLAlchemy tests, 384 passed.
- Lock:
uv lockchanged only the two specifiers. - Finding, fixed in 349eaeb:
docs/sqlalchemy.md:192still carried "(SQLAlchemy 2.0+)" in the async section. It was redundant under the new floor and inconsistent with the other removed qualifiers.
There was a problem hiding this comment.
CI note (Claude Code)
Run 36252040653 at 349eaeb, attempt 1: all checks passed except test-sqla-async / run (3.11).
- In that job, all 577 tests passed (758 skipped). The job failed in session teardown with
INTERNALERROR. - The failure: the SQLAlchemy test plugin's
pytest_testnodedownran thetests/sqlalchemy/conftest.pydrop_dbhook, whoseDROP DATABASE test_b4798d487ea4 CASCADEfailed withSemanticException [Error 10072]: Database does not exist. - The log shows no crashed or replaced xdist worker.
- The same suites passed on the other nine
test-sqla/test-sqla-asyncjobs. - This PR does not touch the follower database hooks.
- Attempt 2 of the failed job passed. All 22 checks now pass; the 3 skipped entries belong to the run from the Draft period.
If the teardown drop failure recurs, the drop_db hook could tolerate a missing follower database; that would be a separate change.
|
|
||
| [project.optional-dependencies] | ||
| sqlalchemy = ["sqlalchemy>=1.0.0"] | ||
| sqlalchemy = ["sqlalchemy>=2.0.0"] |
There was a problem hiding this comment.
Self-review round two — claims, compatibility, evidence (Claude Code)
Scope: d6891e9e8f4204f5e8ac4d181e7e41814e52332d..349eaeb53b52bce5e3c26db7a50a2b34c1e16982, plus the PR body and commit messages.
Result: CLEAN.
Claims checked:
- Only the specifiers change in the lock:
git diff uv.lockshows the tworequires-dist/devspecifier lines only. - A 1.4 pin is rejected:
uv run --no-project --isolated --with 'sqlalchemy==1.4.54' --with '.[sqlalchemy]'reports the requirements as unsatisfiable. - SQLAlchemy 2.0.0 on Python 3.13: 2.0.0 fails to import on 3.13 with
AssertionError: ... directly inherits TypingOnly but has additional attributes, which comes from SQLAlchemy itself. That is why 2.0.0 was checked on Python 3.10. - No other declarations:
git grepfinds no remaining>=1.0.0or 1.x statements inREADME.md,docs/, orpyproject.toml. Nobenchmarks/pyproject declares SQLAlchemy.
Caller compatibility:
- Users on SQLAlchemy 1.x resolve to an earlier PyAthena, which the PR body calls out for the release notes.
get_double_typeremains importable.- No runtime behavior changes on 2.x: every removed branch was the 2.x path already.
Evidence limits: the full pyathena, sqla, and sqla-async suites are left to CI, and SQLAlchemy 2.1 is out of scope.
| @@ -37,18 +37,12 @@ | |||
|
|
|||
|
|
|||
| def get_double_type() -> type[Any]: | |||
There was a problem hiding this comment.
Independent review (relayed)
This is a static review of a detached snapshot at the head SHA; the reviewer ran nothing.
- Reviewer: Codex CLI 0.157.0, model
gpt-6-sol, session01a0de52-df73-7b70-bb91-8084abed18ca. - Invocation:
codex exec -s read-only --ephemeral. - Scope:
d6891e9e8f4204f5e8ac4d181e7e41814e52332d..349eaeb53b52bce5e3c26db7a50a2b34c1e16982. - The prompt contained the literal diff and constraints only (no PR number, description, or prior findings).
Covered surfaces:
- The base-to-head diff, and SQLAlchemy 2.x branch equivalence and API use.
- Callers and tests.
pyathena/,tests/,docs/,README.md,pyproject.toml, andbenchmarks/for remaining 1.x assumptions.- The public
types.__all__, anduv.lockconsistency.
The reviewer noted that the changed tests keep the 2.x reflection path and tighten the DOUBLE assertion.
Result: CLEAN — no actionable regression or remaining 1.x-specific requirement.
The review snapshot and PR worktree were unchanged afterwards (git status clean).
WHAT
pyproject.toml: thesqlalchemyextra and the dev dependency group now require>=2.0.0.uv.lock: updated. Only the two specifier lines change, and no package versions change.README.mdanddocs/introduction.mdlist>=2.0.0for the SQLAlchemy extra.docs/sqlalchemy.mdstates 2.0.0 or higher and drops the "(SQLAlchemy 2.0+)" / "With SQLAlchemy 2.0," qualifiers that implied 1.x support.compiler.py:hasattr(types, "Double")/getattr(types, "Double", get_double_type())becometypes.Double, and the true-division casts usetypes.DOUBLE().base.py:ischema_names["double"]istypes.DOUBLE.array.py:_ArrayTypeInspector.variantreadstype_._variant_mappingdirectly.get_double_type()kept: it is exported inpyathena.sqlalchemy.types.__all__, so it stays public, now always returnstypes.DOUBLE, and is no longer used internally.test_reflect_table_include_columnsdrops its 1.2/1.3 branches and usesInspector.reflect_table.test_numeric_type_variantsno longer accepts the 1.xFLOATfallback for DOUBLE columns.test_get_double_typeassertstypes.DOUBLE.WHY
Closes #845.
The declared
sqlalchemy>=1.0.0did not match what is tested or what works:TypeEngine._variant_mapping.Release note: installing this release with the
sqlalchemyextra requires SQLAlchemy 2.0.0 or later.With
sqlalchemy==1.4.54pinned,uvrejectspyathena[sqlalchemy]as unsatisfiable, so such environments stay on an earlier PyAthena release.TEST
Tested commit: c687ea4 (code and tests). 349eaeb changes only one line of
docs/sqlalchemy.md;just docs lintwas rerun on it.just lint: passed.just docs lint: 0 errors.uv run pytestovertest_compiler.py,test_array.py,test_temporal.py,test_types.py,test_map.py, andtest_struct.pyintests/pyathena/sqlalchemy/, with-k "not Integration": 384 passed.uv run --env-file .env pytest -n 1 tests/pyathena/sqlalchemy/test_base.py -k "test_reflect_table_include_columns or test_reflect_select or test_get_column_type or test_numeric_type_variants"against Athena: 4 passed.uv run --no-project --isolated --with 'sqlalchemy==1.4.54' --with '.[sqlalchemy]': resolution fails.cast(x, Double())rendersDOUBLE.AthenaArray(Integer)result processing returns[1].get_double_type()returnstypes.DOUBLE.Not run locally: the full
just test pyathena,just test sqla, andjust test sqla-asyncsuites (left to CI).SQLAlchemy 2.1 validation is out of scope (#845).
🤖 Generated with Claude Code