Skip to content

Backport #839: Resolve the bare awsathena scheme to the REST dialect #903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laughingman7743 merged 2 commits into from
Sep 30, 2026
+36 −3
Merged

Backport #839: Resolve the bare awsathena scheme to the REST dialect #903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68b25aa
Backport #839: Resolve the bare awsathena scheme to the REST dialect
aminghadersohi Sep 30, 2026
bc5fc93
Backport #905: Align the compliance-suite dialect registry with the e…
laughingman7743 Sep 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/sqlalchemy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ awsathena+aiorest://:@athena.{region_name}.amazonaws.com:443/{schema_name}?s3_st

| Dialect | Driver | Schema | Cursor |
|-----------|--------|------------------|------------------------|
| awsathena | | awsathena | DefaultCursor |
| awsathena | rest | awsathena | DefaultCursor |

@laughingman7743 laughingman7743 Sep 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims, callers, operations). Base d6fe836, head 68b25aa.

Claims checked against evidence that could disprove them:

  • Before the change on 3.x, a bare URL creates AthenaDialect, and Engine.driver, URL.get_driver_name(), and dialect_description each raise AttributeError: reproduced by installing origin/3.x (d6fe836) into an isolated environment with SQLAlchemy 2.0.46.
  • TestAthenaDialect came to master with Fix metadata reflection throttling and reuse listed metadata #777: git log -S finds 6d791e2, which GitHub maps to Fix metadata reflection throttling and reuse listed metadata #777.
  • Every PyAthena and compliance-suite fixture uses an explicit driver: tests/__init__.py:6 and :10 use awsathena+rest:// / awsathena+aiorest://, and tests/pyathena/conftest.py:158 / :192 default to rest / aiorest. No other test or doc uses a bare awsathena:// URL.
  • The backport commit keeps the contributor as author and records the -x source and conflict resolution.
  • Docs: the edited table row is the only place on 3.x that describes the bare schema; the other rows and the async table have no bare entry.

Callers: isinstance(engine.dialect, AthenaDialect) still holds, the cursor and connect arguments are unchanged, and SQLAlchemy 1.4.54 resolves the same class as 2.0.46. Only type(engine.dialect) is AthenaDialect checks change, which the release note states.

Operations: no AWS request path changes.

Corrections made to the PR body: "agreed for both lines" now states that the maintainer decided on the port. The CI note now says, from .github/workflows/test.yaml:118-119, that pyproject.toml is a shared file, so Ready runs all three AWS suites, including Spark, on the newest Python version.

Result: CLEAN after those wording corrections. The round-one out-of-scope note about tests/sqlalchemy/__init__.py stands.

@laughingman7743 laughingman7743 Sep 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repair follow-up, round-two perspective (claims, callers, operations), same range 68b25aa..bc5fc93.

Claims added to the PR body and commit message:

Callers and operations: test harness only; no library behavior changes. CI selection is unchanged, because the pyproject.toml change from #839 already selects all three suites including Spark.

Result: CLEAN.

| awsathena | rest | awsathena+rest | DefaultCursor |
| awsathena | pandas | awsathena+pandas | {ref}`pandas-cursor` |
| awsathena | arrow | awsathena+arrow | {ref}`arrow-cursor` |
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ documentation = "https://pyathena.dev/"
issues = "https://github.com/pyathena-dev/PyAthena/issues"

[project.entry-points."sqlalchemy.dialects"]
awsathena = "pyathena.sqlalchemy.base:AthenaDialect"
awsathena = "pyathena.sqlalchemy.rest:AthenaRestDialect"

@laughingman7743 laughingman7743 Sep 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (implementation behavior). Base d6fe836, head 68b25aa.

Covered: all three changed files (pyproject.toml, docs/sqlalchemy.md, tests/pyathena/sqlalchemy/test_base.py); callers that resolve the bare scheme; AthenaRestDialect vs AthenaDialect on 3.x (driver, supports_statement_cache, import_dbapi/dbapi); SQLAlchemy 1.4.54 and 2.0.46 resolution; whether the test fails without the change.

Result: CLEAN for this PR's scope.

  • AthenaRestDialect only adds driver = "rest" and delegates import_dbapi to the base, so connect arguments and the cursor are unchanged; isinstance(dialect, AthenaDialect) still holds. Nothing in pyathena/ or tests/ compares type(dialect) except the new test.
  • The test fails against 3.x's entry point (AthenaDialect is not AthenaRestDialect) and passes after uv sync.
  • The conflict resolution adds only Resolve the bare awsathena scheme to the REST dialect #839's import and test; the added lines match Resolve the bare awsathena scheme to the REST dialect #839's diff apart from the new TestAthenaDialect class line.

Out of scope, pre-existing on master as well: tests/sqlalchemy/__init__.py:3 still calls registry.register("awsathena", "pyathena.sqlalchemy.base", "AthenaDialect"), which overrides the entry point inside the compliance-suite process. Importing tests.sqlalchemy first makes a bare URL create AthenaDialect again. There is no functional impact today: the suites run in separate pytest sessions and the compliance suite connects only with +rest / +aiorest. Aligning it belongs in a master change first, then a backport, so this PR keeps #839's delta unchanged.

@laughingman7743 laughingman7743 Sep 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repair follow-up, round-one perspective (implementation behavior). Range: git range-diff is not needed because the base is unchanged; the repair is the added commit 68b25aa..bc5fc93 on merge-base d6fe836.

The out-of-scope note above is now addressed by backporting #905 (commit d6b9303 on master's PR branch, -x).

  • tests/sqlalchemy/__init__.py registers the same eleven classes as the 3.x entry points: bare awsathena is AthenaRestDialect, and awsathena.polars is added.
  • test_compliance_suite_registry_matches_entry_points runs the file against an empty PluginLoader via monkeypatch, so it does not depend on the global registry or test order. It fails with 3.x's original file (both differences reported) and passes now.
  • Importing tests.sqlalchemy no longer changes what a bare URL creates.
  • The compliance suites collect 759 tests with and without --dburi async, the same as origin/3.x.
  • The applied lines match Align the compliance-suite dialect registry with the entry points #905's commit exactly; only the conflict context differs.

Result: CLEAN.

"awsathena.rest" = "pyathena.sqlalchemy.rest:AthenaRestDialect"
"awsathena.pandas" = "pyathena.sqlalchemy.pandas:AthenaPandasDialect"
"awsathena.arrow" = "pyathena.sqlalchemy.arrow:AthenaArrowDialect"
Expand Down
32 changes: 32 additions & 0 deletions tests/pyathena/sqlalchemy/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import importlib.metadata
import re
import runpy
import textwrap
import uuid
from datetime import date, datetime
from decimal import Decimal
from pathlib import Path
from urllib.parse import quote_plus

import numpy as np
Expand All @@ -15,7 +18,9 @@
from sqlalchemy.sql.ddl import CreateTable
from sqlalchemy.sql.schema import Column, MetaData, Table
from sqlalchemy.sql.selectable import TextualSelect
from sqlalchemy.util import PluginLoader

from pyathena.sqlalchemy.rest import AthenaRestDialect
from pyathena.sqlalchemy.types import (
TINYINT,
AthenaArray,
Expand Down Expand Up @@ -50,6 +55,33 @@ def unique_s3tables_table_name(base: str) -> str:
return f"{base}_{uuid.uuid4().hex[:8]}"


class TestAthenaDialect:
def test_bare_scheme_uses_rest_driver(self):
# The bare awsathena entry point resolves to the REST dialect, like
# awsathena+rest. Requires the package to be reinstalled (uv sync) so the
# installed entry point metadata matches pyproject.toml.
url = "awsathena://athena.us-west-2.amazonaws.com:443/default?s3_staging_dir=s3://bucket/path/"
bare = create_engine(url)
assert type(bare.dialect) is AthenaRestDialect

@laughingman7743 laughingman7743 Sep 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review, relayed. Reviewer: Codex CLI 0.157.1, model gpt-6-sol, reasoning effort high, session 01a0f0d1-3da1-72c2-bd28-d4c093959025. Static review in a read-only sandbox of a detached snapshot at head 68b25aa (base d6fe836); it received the literal diff without the PR description, commit message, or prior findings, and ran no tests. The snapshot and the PR worktree were unchanged afterwards.

Covered: the three changed files; sync and async dialect classes and entry points; test fixtures, SQLAlchemy registration, CI test selection, and repository conventions. It found that the new test fails against the old installed entry point, the docs row matches the new sync driver, async URLs keep +aiorest, and SQLAlchemy 1.x gains no new runtime path.

Result: FINDINGS (1).

  • tests/sqlalchemy/__init__.py:3 still registers bare awsathena as AthenaDialect, overriding the new entry point in any process that imports tests.sqlalchemy. Collecting both test packages in one pytest process makes this bare-URL test resolve to the base class and fail. The reviewer classifies it as introduced by this diff: the registration existed before but became inconsistent with the entry point.

Author verification: confirmed. Importing tests.sqlalchemy first makes a bare URL create AthenaDialect. CI and the just test recipes run the suites in separate processes, so they are unaffected, but the repository sets no testpaths, so a bare pytest from the root collects both packages together. The same line is on master after #839. It stays open here: a fix belongs on master first and then in this backport, and that is pending the maintainer's decision.

@laughingman7743 laughingman7743 Sep 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up, relayed. Reviewer: Codex CLI 0.157.1, model gpt-6-sol, reasoning effort high, session 01a0f0fb-0a86-7de1-a945-130e9dd7cc0b. Static review in a read-only sandbox of a detached snapshot at bc5fc93, given the bounded diff 68b25aa..bc5fc93 and this thread's finding, without the PR description or author conclusions; no tests run. The snapshot and the PR worktree were unchanged afterwards.

Covered: the two changed files, all eleven dialect entry points in pyproject.toml, the test package imports, and the pytest, xdist, and CI invocation paths.

Result: CLEAN. The finding above is resolved: bare awsathena now registers AthenaRestDialect, and tests/sqlalchemy/__init__.py matches every PyAthena entry point. The new test executes the file against a fresh registry, so a wrong or missing registration fails the comparison. No ordering, xdist, or working-directory defect was found.

assert bare.driver == "rest"
assert bare.url.get_driver_name() == "rest"
assert bare.dialect.dialect_description == "awsathena+rest"

def test_compliance_suite_registry_matches_entry_points(self, monkeypatch):
# tests/sqlalchemy registers the dialects for the compliance suite, and a
# registration overrides the installed entry point in that process.
loader = PluginLoader("sqlalchemy.dialects")
monkeypatch.setattr("sqlalchemy.dialects.registry", loader)
runpy.run_path(str(Path(__file__).parents[2] / "sqlalchemy" / "__init__.py"))
entry_points = {
entry_point.name: entry_point.load()
for entry_point in importlib.metadata.entry_points(group="sqlalchemy.dialects")
if entry_point.value.startswith("pyathena.")
}
assert entry_points
assert {name: load() for name, load in loader.impls.items()} == entry_points


class TestSQLAlchemyAthena:
@pytest.mark.parametrize(
"engine",
Expand Down
3 changes: 2 additions & 1 deletion tests/sqlalchemy/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
from sqlalchemy.dialects import registry

registry.register("awsathena", "pyathena.sqlalchemy.base", "AthenaDialect")
registry.register("awsathena", "pyathena.sqlalchemy.rest", "AthenaRestDialect")
registry.register("awsathena.rest", "pyathena.sqlalchemy.rest", "AthenaRestDialect")
registry.register("awsathena.pandas", "pyathena.sqlalchemy.pandas", "AthenaPandasDialect")
registry.register("awsathena.arrow", "pyathena.sqlalchemy.arrow", "AthenaArrowDialect")
registry.register("awsathena.polars", "pyathena.sqlalchemy.polars", "AthenaPolarsDialect")
registry.register("awsathena.s3fs", "pyathena.sqlalchemy.s3fs", "AthenaS3FSDialect")
registry.register("awsathena.aiorest", "pyathena.aio.sqlalchemy.rest", "AthenaAioRestDialect")
registry.register("awsathena.aiopandas", "pyathena.aio.sqlalchemy.pandas", "AthenaAioPandasDialect")
Expand Down
Loading