Skip to content

ci: run SQLAlchemy and Spark tests when shared core modules change - #906

Merged
laughingman7743 merged 4 commits into
masterfrom
ci/896-shared-core-path-filter
Oct 1, 2026
Merged

laughingman7743 merged 4 commits into
masterfrom
ci/896-shared-core-path-filter

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

WHAT

The Test workflow's changes job now also runs the SQLAlchemy tests (the compliance suites and tests/pyathena/(aio/)sqlalchemy/) and the Spark tests of a ready pull request when it changes:

  • a top-level module of pyathena or pyathena.aio (pyathena/*.py, pyathena/aio/*.py, such as util.py, common.py, connection.py, cursor.py, formatter.py, aio/util.py), or
  • a shared test fixture: tests/__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, or tests/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 api call plus grep -E. The path lists are YAML globs, so they are easier to read and extend.

  • For pull_request events the action reads the same pull request files API, and it receives only the job's pull-requests: read token.
  • A renamed file now matches by both its new and its previous path; before, only the new path was checked.
  • Other events (schedule, dispatch, Release) skip the action and still run every suite.
  • The Python version selection is unchanged, in its own step.

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 importing pyathena.sqlalchemy.base, pyathena.sqlalchemy.rest, pyathena.aio.sqlalchemy.base, and the three Spark cursors, then listing sys.modules. Their tests use these fixtures. The filter matched only their own package and test paths, so a regression from, for example, strtobool in pyathena/util.py or retry_api_call surfaced 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.x backport 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), and pinact run --check: passed.
  • Local simulation of the action's matching: a throwaway script (not committed) loads the filters block from the workflow with js-yaml 4. It flattens the anchors and matches with picomatch 2.3.1 using dot: true, the same as src/filter.ts at v4.0.3. 48 cases all match the expected outputs:
    • both 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 .py files, tests/resources/queries/create_database.sql.jinja2, both test workflows, justfile, pyproject.toml, uv.lock
    • both false:
      • pyathena/filesystem/s3.py, pyathena/(aio/)pandas/cursor.py, pyathena/arrow/result_set.py, pyathena/py.typed
      • tests/pyathena/test_cursor.py, tests/pyathena/aio/test_cursor.py, tests/pyathena/pandas/test_cursor.py, tests/pyathena/filesystem/conftest.py
      • docs/index.md, README.md, benchmarks/pyathena_bench/fleet.py, NOTICE, .github/workflows/release.yaml, cloudformation/github_actions/aws.yaml
    • SQLAlchemy only: pyathena/(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.
    • Multi-file and rename cases: docs/index.md + pyathena/filesystem/s3.py + pyathena/util.py → both; pyathena/pandas/cursor.py + pyathena/spark/cursor.py → Spark only; rename pyathena/sqlalchemy/old.py → pyathena/pandas/new.py → SQLAlchemy.
  • Equivalence with the previous grep -E patterns (020dc45): both give the same sqla/spark result for each of the 278 tracked files. The grep -E version matched 15 more of the paths above than master's patterns did: the new core and fixture paths.
  • AWS run on 020dc45 (grep -E version, all suites selected because test.yaml changed): test, test-sqla, and test-sqla-async passed. Ready run on 7439d76 (paths-filter version), run 36794585149:
    • The step called listFiles(pull_number: 906) and received [modified] .github/workflows/test.yaml.
    • It reported shared = true, core = false, sqla = true, spark = true, and python-versions was set.
    • test, test-sqla, and test-sqla-async passed.
  • Not run live: a ready pull request that changes only pyathena/util.py. I skipped the extra AWS run for cost reasons; the simulation covers that case.

🤖 Generated with Claude Code

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>
Comment thread .github/workflows/test.yaml Outdated
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/'

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): CLEAN

  • Scope: git diff 8679f04a36f6ffd7190b892c32b1938672ab502f..fcb75ee0ebb5778a1f4e7fa66e1dbd62d465c163, one file, .github/workflows/test.yaml (changes job comment and the core/sqla/spark patterns).
  • Behavior: core is single-quoted and expanded inside the double-quoted sqla/spark, so \. and the $ anchors reach grep -E unchanged, as for shared. 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 at pyathena/connection.py:553. Their tests import only tests, tests.pyathena.conftest, tests.pyathena.aio.conftest, and tests.pyathena.util. tests/pyathena/conftest.py imports tests.pyathena.tables, and tests/pyathena/util.py reads tests/resources/queries/. All are in core.
  • 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 lint and actionlint pass.
  • Pre-existing, out of scope: the files API reports only the new name of a renamed file (previous_filename is ignored). A file moved out of pyathena/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>
Comment thread .github/workflows/test.yaml Outdated
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.

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 and operational effects): FINDINGS, repaired

  • Scope: full claim audit of git diff 8679f04a36f6ffd7190b892c32b1938672ab502f..fcb75ee0ebb5778a1f4e7fa66e1dbd62d465c163 (PR body, commit message, and workflow comments), plus the repair fcb75ee0ebb5778a1f4e7fa66e1dbd62d465c163..3deba1dd43a172a3b394fe66510445dc167c849d.
  • Finding (repaired in 3deba1d): the core comment said both packages and their tests import the top-level modules. I measured by importing pyathena.sqlalchemy.base, pyathena.sqlalchemy.rest, pyathena.aio.sqlalchemy.base, and the three Spark cursors, then listing sys.modules. The SQLAlchemy dialects load every top-level module except pyathena/async_cursor.py. Spark loads a subset directly (common, converter, error, formatter, glue, model, options, parser, util, aio/util) and the rest through connect() in its tests. The comment now says they load the modules "directly or transitively", and the PR body states the measurement and the async_cursor.py exception. Matching async_cursor.py too is a deliberate superset (issue option 1).
  • Claims checked:
    • strtobool feeds SQLAlchemy URL options (pyathena/sqlalchemy/base.py:288-297, pandas.py/arrow.py/polars.py:57-58).
    • retry_api_call is used by Spark (pyathena/spark/common.py, pyathena/aio/spark/cursor.py).
    • The schedule, dispatch, and Release runs select every suite: the non-pull_request branch 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 changes job.
  • 3.x: origin/3.x has the same fixtures except tests/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>
Comment thread .github/workflows/test.yaml Outdated
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/'

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): 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, session 01a0f0f5-dd9e-7c71-ac29-4442453b524d. Static, source-only review: no tests or workflows were run.
  • Scope: 8679f04a36f6ffd7190b892c32b1938672ab502f..3deba1dd43a172a3b394fe66510445dc167c849d on a detached snapshot. The prompt omitted the PR number, description, commit messages, and prior findings. The snapshot was unchanged after the review (git status clean, HEAD 3deba1d).
  • Covered: the filter and its Bash/grep -E evaluation, test-suite.yaml, suite imports and fixtures under pyathena/ and tests/, 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 to true.
  • Finding (pre-existing, retained): the pattern omitted tests/pyathena/__init__.py and tests/pyathena/aio/__init__.py. Python loads them when it imports the SQLAlchemy and Spark test modules. A ready PR changing only tests/pyathena/aio/__init__.py got sqla=false and spark=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).

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.

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.py and tests/pyathena/test_cursor.py stay false. The new pattern would also match a future tests/conftest.py, which pytest loads for every suite, so that is intended. actionlint and just lint pass.
  • 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.

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): CLEAN

  • Reviewer: Codex CLI 0.157.1, model gpt-6-sol, reasoning effort high, --sandbox read-only, --ephemeral, session 01a0f0fb-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 core matches both package initializers and keeps every path the previous expression matched. The only addition beyond them is a future tests/conftest.py, a shared fixture location. It does not match tests/pyathena/filesystem/, tests/pyathena/(aio/)pandas/, or tests/pyathena/test_*.py. The surrounding comment remains accurate.

@laughingman7743
laughingman7743 marked this pull request as ready for review September 30, 2026 06:26
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>
@laughingman7743
laughingman7743 marked this pull request as draft October 1, 2026 00:02
- id: filter
- id: paths
if: github.event_name == 'pull_request'
uses: dorny/paths-filter@ceb8a2b8f2d89434be7ff52d3de7ec3738c5cc9d # v4.0.3

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, expanded scope (paths-filter migration): CLEAN

  • Scope: full pass over git diff 8679f04a36f6ffd7190b892c32b1938672ab502f..7439d76ea0e9b78797eca35bb82cff109f39e678. The migration commit (020dc45b..7439d76e) replaces the gh api + grep -E selection with dorny/paths-filter, so earlier narrow-repair scoping does not apply.
  • Action behavior, read from src/main.ts and src/filter.ts at the pinned SHA (ceb8a2b8, tag v4.0.3):
    • For pull_request with a token (default github.token), it calls pulls.listFiles with 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 default some quantifier, the output is 'true' when any changed file matches any pattern.
  • 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 -E patterns at 020dc45, it gives identical sqla/spark results for all 278 tracked files. The only difference is a hypothetical new tests/conftest.py, which the previous tests/(pyathena/(aio/)?)?conftest regex matched and the explicit list does not; no such file exists.
  • Outputs: sqla/spark are github.event_name != 'pull_request' || steps.paths.outputs.<name> == 'true'. Schedule, dispatch, and Release (workflow_call inherits the caller's push event) skip the step and evaluate to true. 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: read replaces the workflow-level id-token: write. So the third-party action gets only a read token and no OIDC, and it is pinned by full SHA (pinact run --check passes).
  • 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 api under bash -e.
  • Runtime: the action uses node24, as actions/checkout v6.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 api before.
  • Checks: just lint, just scripts (actionlint + 108 script tests), and pinact run --check passed.

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' }}

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, 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": getChangedFilesFromApi calls pulls.listFiles (src/main.ts:204-217 at 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: read token": job-level permissions replace the workflow-level ones.
    • "other events still run every suite": the output expressions.
  • 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 -E version, because the selection is identical.
  • Evidence limits (stated in the PR body): the AWS run that passed was on 020dc45, the grep -E version. 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 the changes job 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

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, expanded scope (relayed): CLEAN

  • Reviewer: Codex CLI 0.157.1, model gpt-6-sol, reasoning effort high, --sandbox read-only, --ephemeral, session 01a0f4c7-89b2-7371-8671-28eec96b9bbe. Static review only: no workflow or tests run.
  • Scope: full pass over 8679f04a36f6ffd7190b892c32b1938672ab502f..7439d76ea0e9b78797eca35bb82cff109f39e678 on a detached snapshot. Because the sandbox has no network, the package also contained action.yml, src/main.ts, and src/filter.ts of dorny/paths-filter at the pinned commit ceb8a2b8 (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 filters parse as YAML comments.
    • sqla/spark become "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.
  • Pre-existing note: paths-ignore at .github/workflows/test.yaml:16-18 means 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 above on: (lines 11-14) already states that docs-only pull requests start no run.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 1, 2026 00:07
@laughingman7743
laughingman7743 merged commit 9b9421d into master Oct 1, 2026
16 of 18 checks passed
@laughingman7743
laughingman7743 deleted the ci/896-shared-core-path-filter branch October 1, 2026 00:25
laughingman7743 added a commit that referenced this pull request Oct 1, 2026
Backport #906: Run SQLAlchemy and Spark tests when shared core modules change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI skips SQLAlchemy and Spark tests when only shared core modules change

1 participant