ci: run SQLAlchemy and Spark tests when shared core modules change - #906
Conversation
The changes job selected the SQLAlchemy and Spark suites only from their
own package and test paths, so a ready pull request that changed only a
top-level module both import (such as pyathena/util.py or
pyathena/common.py) or a shared test fixture skipped them.
Treat the top-level modules of pyathena and pyathena.aio, and the shared
test fixtures (tests/__init__.py, tests/pyathena/{conftest,tables,util}.py,
tests/pyathena/aio/conftest.py, tests/resources/), as shared by both
suites. Changes limited to the result-set or filesystem packages still
skip them.
Closes #896
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| spark="$shared|^pyathena/(aio/)?spark/|^tests/pyathena/(aio/)?spark/" | ||
| # The top-level modules and the shared test fixtures, which both the | ||
| # SQLAlchemy and the Spark code and tests import. | ||
| core='^pyathena/(aio/)?[^/]+\.py$|^tests/(__init__|pyathena/(conftest|tables|util)|pyathena/aio/conftest)\.py$|^tests/resources/' |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN
- Scope:
git diff 8679f04a36f6ffd7190b892c32b1938672ab502f..fcb75ee0ebb5778a1f4e7fa66e1dbd62d465c163, one file,.github/workflows/test.yaml(changesjob comment and thecore/sqla/sparkpatterns). - Behavior:
coreis single-quoted and expanded inside the double-quotedsqla/spark, so\.and the$anchors reachgrep -Eunchanged, as forshared. ERE alternation with anchors in the middle (...py$|^tests/...) matches per changed-file line, the same as before. The output keys and the non-pull-request branch are unchanged. - Coverage traced from imports, runtime imports included.
pyathena/(aio/)?(sqlalchemy|spark)/import only top-level modules (pyathena,common,cursor,error,formatter,model,util,aio.connection,aio.cursor,aio.util), their own subpackages, and the result-set cursors (pyathena/(aio/)?(pandas|arrow|polars|s3fs)/cursor). The result-set cursors stay excluded, as the maintainer decided on the issue scope. No top-level module imports a subpackage; the only match is a docstring example atpyathena/connection.py:553. Their tests import onlytests,tests.pyathena.conftest,tests.pyathena.aio.conftest, andtests.pyathena.util.tests/pyathena/conftest.pyimportstests.pyathena.tables, andtests/pyathena/util.pyreadstests/resources/queries/. All are incore. - Simplicity: one variable reused by both patterns; no change to the grep/output logic.
- Verification: a local simulation extracts these assignments and evaluates 35 paths (listed in the PR description): all as expected. Against master's patterns, exactly the 13 new core and fixture paths differ.
just lintandactionlintpass. - Pre-existing, out of scope: the files API reports only the new name of a renamed file (
previous_filenameis ignored). A file moved out ofpyathena/sqlalchemy/is still caught by its new path only if that path matches. This behavior is unchanged by this PR.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| sqla="$shared|^pyathena/(aio/)?sqlalchemy/|^tests/sqlalchemy/|^tests/pyathena/(aio/)?sqlalchemy/|^setup\.cfg$" | ||
| spark="$shared|^pyathena/(aio/)?spark/|^tests/pyathena/(aio/)?spark/" | ||
| # The SQLAlchemy and Spark packages load the top-level modules, | ||
| # directly or transitively, and their tests use the shared fixtures. |
There was a problem hiding this comment.
Self-review round two (claims and operational effects): FINDINGS, repaired
- Scope: full claim audit of
git diff 8679f04a36f6ffd7190b892c32b1938672ab502f..fcb75ee0ebb5778a1f4e7fa66e1dbd62d465c163(PR body, commit message, and workflow comments), plus the repairfcb75ee0ebb5778a1f4e7fa66e1dbd62d465c163..3deba1dd43a172a3b394fe66510445dc167c849d. - Finding (repaired in 3deba1d): the
corecomment said both packages and their tests import the top-level modules. I measured by importingpyathena.sqlalchemy.base,pyathena.sqlalchemy.rest,pyathena.aio.sqlalchemy.base, and the three Spark cursors, then listingsys.modules. The SQLAlchemy dialects load every top-level module exceptpyathena/async_cursor.py. Spark loads a subset directly (common,converter,error,formatter,glue,model,options,parser,util,aio/util) and the rest throughconnect()in its tests. The comment now says they load the modules "directly or transitively", and the PR body states the measurement and theasync_cursor.pyexception. Matchingasync_cursor.pytoo is a deliberate superset (issue option 1). - Claims checked:
strtoboolfeeds SQLAlchemy URL options (pyathena/sqlalchemy/base.py:288-297,pandas.py/arrow.py/polars.py:57-58).retry_api_callis used by Spark (pyathena/spark/common.py,pyathena/aio/spark/cursor.py).- The schedule, dispatch, and Release runs select every suite: the non-
pull_requestbranch sets both outputs to true. - Result-set and filesystem changes still skip both suites: simulated.
- Operator (AWS cost): of the 44 non-docs PRs merged into master since 2026-06-01, the new filter selects SQLAlchemy for 36 (23 before) and Spark for 29 (14 before). Reject synchronous iteration of the asyncio cursors and result sets #899 and Honor the arraysize argument of the SQL cursors #901 are among the newly selected, and they touched modules the SQLAlchemy dialects load. The per-run cost of those suites is not measured; the PR body says so.
- Existing callers: the job outputs and job conditions are unchanged. Draft and fork PRs still skip the
changesjob. - 3.x:
origin/3.xhas the same fixtures excepttests/pyathena/tables.py, which is harmless in the pattern. The backport follows separately.
Python loads tests/pyathena/__init__.py and tests/pyathena/aio/__init__.py when importing the SQLAlchemy and Spark tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| spark="$shared|^pyathena/(aio/)?spark/|^tests/pyathena/(aio/)?spark/" | ||
| # The SQLAlchemy and Spark packages load the top-level modules, | ||
| # directly or transitively, and their tests use the shared fixtures. | ||
| core='^pyathena/(aio/)?[^/]+\.py$|^tests/(pyathena/(aio/)?)?(__init__|conftest)\.py$|^tests/pyathena/(tables|util)\.py$|^tests/resources/' |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS (1 pre-existing gap, none introduced)
- Reviewer: Codex CLI 0.157.1, model
gpt-6-sol(OpenAI), reasoning effort high,--sandbox read-only,--ephemeral, session01a0f0f5-dd9e-7c71-ac29-4442453b524d. Static, source-only review: no tests or workflows were run. - Scope:
8679f04a36f6ffd7190b892c32b1938672ab502f..3deba1dd43a172a3b394fe66510445dc167c849don a detached snapshot. The prompt omitted the PR number, description, commit messages, and prior findings. The snapshot was unchanged after the review (git statusclean, HEAD 3deba1d). - Covered: the filter and its Bash/
grep -Eevaluation,test-suite.yaml, suite imports and fixtures underpyathena/andtests/,justfile,pyproject.toml,setup.cfg. Reported that the new alternatives are anchored and quoted correctly, that the multi-line file list is handled line by line, and that non-PR events still set both outputs totrue. - Finding (pre-existing, retained): the pattern omitted
tests/pyathena/__init__.pyandtests/pyathena/aio/__init__.py. Python loads them when it imports the SQLAlchemy and Spark test modules. A ready PR changing onlytests/pyathena/aio/__init__.pygotsqla=falseandspark=false. - Verification: both files exist and are empty today; the finding is valid. Folded into this PR, since it is the same class of gap and costs nothing (the files rarely change).
There was a problem hiding this comment.
Repaired in 020dc45: core now matches ^tests/(pyathena/(aio/)?)?(__init__|conftest)\.py$|^tests/pyathena/(tables|util)\.py$|^tests/resources/.
Self-review of the repair (3deba1dd..020dc45b, one line):
- Behavior (round-one perspective): the simulation now covers 37 paths, including both initializers, and every case matches the expected output. Each fixture path that matched before still matches.
tests/pyathena/filesystem/conftest.pyandtests/pyathena/test_cursor.pystayfalse. The new pattern would also match a futuretests/conftest.py, which pytest loads for every suite, so that is intended.actionlintandjust lintpass. - Claims (round-two perspective): the comment ("their tests use the shared fixtures") still holds, and the PR body lists the fixture files. The body is updated to name the initializers. There is no cost change: the two files are empty and last changed in 3abb567.
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN
- Reviewer: Codex CLI 0.157.1, model
gpt-6-sol, reasoning effort high,--sandbox read-only,--ephemeral, session01a0f0fb-1879-7370-b9e2-a3721582c1b5. Static review only. - Scope: repair
3deba1dd43a172a3b394fe66510445dc167c849d..020dc45b7842155239b43f4905b23690d0e38ae6(range-diff against merge-base 8679f04; the old head's object was verified present). The prompt gave the reported gap and the intended scope, without the author's conclusions. The snapshot was unchanged afterwards (clean, HEAD 020dc45). - Result: the repaired
corematches both package initializers and keeps every path the previous expression matched. The only addition beyond them is a futuretests/conftest.py, a shared fixture location. It does not matchtests/pyathena/filesystem/,tests/pyathena/(aio/)pandas/, ortests/pyathena/test_*.py. The surrounding comment remains accurate.
Replace the grep over the pull request files API with dorny/paths-filter v4.0.3, which reads the same API, so the path lists are YAML globs instead of long regular expressions. The selection is unchanged for every tracked file; a renamed file now also matches by its previous path. Events other than pull_request still run every suite, and the Python version selection stays in its own step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| - id: filter | ||
| - id: paths | ||
| if: github.event_name == 'pull_request' | ||
| uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3 |
There was a problem hiding this comment.
Self-review round one, expanded scope (paths-filter migration): CLEAN
- Scope: full pass over
git diff 8679f04a36f6ffd7190b892c32b1938672ab502f..7439d76ea0e9b78797eca35bb82cff109f39e678. The migration commit (020dc45b..7439d76e) replaces thegh api+grep -Eselection with dorny/paths-filter, so earlier narrow-repair scoping does not apply. - Action behavior, read from
src/main.tsandsrc/filter.tsat the pinned SHA (ceb8a2b8, tag v4.0.3):- For
pull_requestwith a token (defaultgithub.token), it callspulls.listFileswith pagination, the same API as before, and needs no checkout. - A renamed file is split into an added new path and a deleted previous path.
- Filters are loaded with js-yaml, nested anchor lists are flattened, and each pattern is matched with picomatch (
dot: true). With the defaultsomequantifier, the output is'true'when any changed file matches any pattern.
- For
- Selection: a local mirror of that logic (js-yaml 4, picomatch 2.3.1) gives the expected result for 48 cases. Against the previous
grep -Epatterns at 020dc45, it gives identicalsqla/sparkresults for all 278 tracked files. The only difference is a hypothetical newtests/conftest.py, which the previoustests/(pyathena/(aio/)?)?conftestregex matched and the explicit list does not; no such file exists. - Outputs:
sqla/sparkaregithub.event_name != 'pull_request' || steps.paths.outputs.<name> == 'true'. Schedule, dispatch, and Release (workflow_callinherits the caller'spushevent) skip the step and evaluate totrue. Pull requests use the action's result. Job outputs are strings, so consumers'== 'true'/!= 'true'comparisons are unchanged. Draft and fork pull requests still skip the whole job. - Permissions: the job's
permissions: pull-requests: readreplaces the workflow-levelid-token: write. So the third-party action gets only a read token and no OIDC, and it is pinned by full SHA (pinact run --checkpasses). - Failure path: an API failure fails the step and therefore the job, so the test jobs do not run. This is the same as the previous
gh apiunderbash -e. - Runtime: the action uses
node24, asactions/checkoutv6.0.2 already does in this repository. - Pre-existing, unchanged: the files API lists at most 3,000 files per pull request, the same as
gh apibefore. - Checks:
just lint,just scripts(actionlint + 108 script tests), andpinact run --checkpassed.
| sqla: ${{ steps.filter.outputs.sqla }} | ||
| spark: ${{ steps.filter.outputs.spark }} | ||
| python-versions: ${{ steps.filter.outputs.python-versions }} | ||
| sqla: ${{ github.event_name != 'pull_request' || steps.paths.outputs.sqla == 'true' }} |
There was a problem hiding this comment.
Self-review round two, expanded scope (claims and operational effects): CLEAN (PR body rewritten)
- Scope: claims in the 7439d76 commit message, the rewritten PR body, and the workflow comments, over
8679f04a36f6ffd7190b892c32b1938672ab502f..7439d76ea0e9b78797eca35bb82cff109f39e678. - Claims checked:
- "reads the same pull request files API":
getChangedFilesFromApicallspulls.listFiles(src/main.ts:204-217at v4.0.3). - "selection is unchanged for every tracked file": per-file comparison over
git ls-files(278 files) is identical. - "a renamed file now also matches by its previous path":
src/main.ts:228-238. - "only the job's
pull-requests: readtoken": job-level permissions replace the workflow-level ones. - "other events still run every suite": the output expressions.
- "reads the same pull request files API":
- Operator: a new third-party action dependency. It is maintained (v4.0.3 published 2026-08-05, last push 2026-09-27), the repository allows all actions, and pinact tracks the SHA pin. The AWS cost effect is unchanged from the
grep -Eversion, because the selection is identical. - Evidence limits (stated in the PR body): the AWS run that passed was on 020dc45, the
grep -Eversion. The action itself runs only on a ready pull request, so its first real run is this PR's Ready run; its log and outputs must be checked before merge. A push at 7439d76 while Ready started a run; returning the PR to Draft cancelled it before thechangesjob finished, so no AWS job ran. - Workflow comment above
changes:: still accurate, because the behavior it describes did not change.
| - id: filter | ||
| - id: paths | ||
| if: github.event_name == 'pull_request' | ||
| uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3 |
There was a problem hiding this comment.
Independent review, expanded scope (relayed): CLEAN
- Reviewer: Codex CLI 0.157.1, model
gpt-6-sol, reasoning effort high,--sandbox read-only,--ephemeral, session01a0f4c7-89b2-7371-8671-28eec96b9bbe. Static review only: no workflow or tests run. - Scope: full pass over
8679f04a36f6ffd7190b892c32b1938672ab502f..7439d76ea0e9b78797eca35bb82cff109f39e678on a detached snapshot. Because the sandbox has no network, the package also containedaction.yml,src/main.ts, andsrc/filter.tsof dorny/paths-filter at the pinned commitceb8a2b8(v4.0.3), outside the repository tree. The prompt omitted the PR number, description, commit messages, and prior findings. The snapshot's HEAD was unchanged afterwards; the only untracked entry was that reference directory. - Covered: the workflow diff, the output consumers in
test.yaml/test-suite.yaml, the Release caller, and the pinned action source. - Result:
- The globs and flattened YAML anchors select the stated paths, dotfiles match, and the comments inside
filtersparse as YAML comments. sqla/sparkbecome"true"/"false"as downstream jobs expect. When the step is skipped for a non-PR event, both are"true".- Python version selection is unchanged.
- The action is SHA-pinned and gets a token limited to pull-request read access, and changed filenames never reach a shell.
- An API error fails
changes, so the dependent AWS jobs do not run.
- The globs and flattened YAML anchors select the stated paths, dotfiles match, and the comments inside
- Pre-existing note:
paths-ignoreat.github/workflows/test.yaml:16-18means a PR that changes only docs or Markdown starts no run. So "A ready pull request always runs the PyAthena suite" (line 68) applies only to runs that start. Deferred as pre-existing and unchanged: the comment aboveon:(lines 11-14) already states that docs-only pull requests start no run.
Backport #906: Run SQLAlchemy and Spark tests when shared core modules change
WHAT
The Test workflow's
changesjob now also runs the SQLAlchemy tests (the compliance suites andtests/pyathena/(aio/)sqlalchemy/) and the Spark tests of a ready pull request when it changes:pyathenaorpyathena.aio(pyathena/*.py,pyathena/aio/*.py, such asutil.py,common.py,connection.py,cursor.py,formatter.py,aio/util.py), ortests/__init__.py,tests/pyathena/__init__.py,tests/pyathena/aio/__init__.py,tests/pyathena/conftest.py,tests/pyathena/aio/conftest.py,tests/pyathena/tables.py,tests/pyathena/util.py, ortests/resources/.Changes limited to subpackages such as
pyathena/filesystem/or the result-set packages (pyathena/(aio/)?(pandas|arrow|polars|s3fs)/), or to their tests, still skip both, as before.The job now selects the suites with dorny/paths-filter v4.0.3 (pinned by SHA) instead of a
gh apicall plusgrep -E. The path lists are YAML globs, so they are easier to read and extend.pull_requestevents the action reads the same pull request files API, and it receives only the job'spull-requests: readtoken.WHY
Closes #896.
The SQLAlchemy and Spark packages load every top-level module except
pyathena/async_cursor.py, directly or transitively. This was checked by importingpyathena.sqlalchemy.base,pyathena.sqlalchemy.rest,pyathena.aio.sqlalchemy.base, and the three Spark cursors, then listingsys.modules. Their tests use these fixtures. The filter matched only their own package and test paths, so a regression from, for example,strtoboolinpyathena/util.pyorretry_api_callsurfaced only in the weekly run or the Release workflow.This follows the issue's option 1. The move to paths-filter was requested during the review of this PR. The
3.xbackport follows after this merges.Cost impact: 44 non-docs pull requests were merged into master since 2026-06-01. Applied to their changed files, this filter selects the SQLAlchemy tests for 36 of them (23 before) and the Spark tests for 29 (14 before). The newly selected ones include #899 (
pyathena/aio/common.py,pyathena/aio/result_set.py) and #901 (pyathena/result_set.py), modules that the SQLAlchemy dialects load. The per-run AWS cost of those suites was not measured.TEST
Tested commit: 7439d76.
just lint,just scripts(actionlint and the script tests), andpinact run --check: passed.filtersblock from the workflow with js-yaml 4. It flattens the anchors and matches with picomatch 2.3.1 usingdot: true, the same assrc/filter.tsat v4.0.3. 48 cases all match the expected outputs:true:pyathena/util.py,pyathena/aio/util.py,pyathena/common.py,pyathena/connection.py,pyathena/cursor.py,pyathena/aio/cursor.py,pyathena/__init__.py, the seven fixture.pyfiles,tests/resources/queries/create_database.sql.jinja2, both test workflows,justfile,pyproject.toml,uv.lockfalse:pyathena/filesystem/s3.py,pyathena/(aio/)pandas/cursor.py,pyathena/arrow/result_set.py,pyathena/py.typedtests/pyathena/test_cursor.py,tests/pyathena/aio/test_cursor.py,tests/pyathena/pandas/test_cursor.py,tests/pyathena/filesystem/conftest.pydocs/index.md,README.md,benchmarks/pyathena_bench/fleet.py,NOTICE,.github/workflows/release.yaml,cloudformation/github_actions/aws.yamlpyathena/(aio/)sqlalchemy/base.py,tests/sqlalchemy/conftest.py,tests/pyathena/(aio/)sqlalchemy/test_base.py,setup.cfg. Spark only:pyathena/(aio/)spark/cursor.py,tests/pyathena/spark/test_spark_cursor.py,tests/pyathena/aio/spark/test_cursor.py.docs/index.md+pyathena/filesystem/s3.py+pyathena/util.py→ both;pyathena/pandas/cursor.py+pyathena/spark/cursor.py→ Spark only; renamepyathena/sqlalchemy/old.py→pyathena/pandas/new.py→ SQLAlchemy.grep -Epatterns (020dc45): both give the samesqla/sparkresult for each of the 278 tracked files. Thegrep -Eversion matched 15 more of the paths above than master's patterns did: the new core and fixture paths.grep -Eversion, all suites selected becausetest.yamlchanged):test,test-sqla, andtest-sqla-asyncpassed. Ready run on 7439d76 (paths-filter version), run 36794585149:listFiles(pull_number: 906)and received[modified] .github/workflows/test.yaml.shared = true,core = false,sqla = true,spark = true, andpython-versionswas set.test,test-sqla, andtest-sqla-asyncpassed.pyathena/util.py. I skipped the extra AWS run for cost reasons; the simulation covers that case.🤖 Generated with Claude Code