Use case
The Test workflow's changes job (#837) runs the SQLAlchemy tests and the Spark tests of a ready pull request only when files under their own packages change (.github/workflows/test.yaml:125-127):
shared='^(\.github/workflows/test(-suite)?\.yaml|justfile|pyproject\.toml|uv\.lock)$'
sqla="$shared|^pyathena/(aio/)?sqlalchemy/|^tests/sqlalchemy/|^tests/pyathena/(aio/)?sqlalchemy/|^setup\.cfg$"
spark="$shared|^pyathena/(aio/)?spark/|^tests/pyathena/(aio/)?spark/"
Both packages depend on modules outside those paths. Their direct imports on master:
- SQLAlchemy (
pyathena/sqlalchemy/, pyathena/aio/sqlalchemy/): pyathena, pyathena.cursor, pyathena.aio.cursor, pyathena.aio.connection, pyathena.converter, pyathena.error, pyathena.formatter, pyathena.model, pyathena.util.
- Spark (
pyathena/spark/, pyathena/aio/spark/): pyathena, pyathena.common, pyathena.error, pyathena.model, pyathena.util, pyathena.aio.util.
Through connect() and the cursor base classes, they also depend on pyathena/connection.py and pyathena/common.py.
So a ready pull request that changes only one of those modules does not run the SQLAlchemy compliance suites, tests/pyathena/(aio/)sqlalchemy/, or the Spark tests. For example, a change to strtobool in pyathena/util.py can break SQLAlchemy URL option parsing, and a change to retry_api_call can break Spark session calls, without CI noticing before merge. The weekly schedule and the Release workflow run every suite, so such a regression surfaces only after merge or at release time.
This was found by the independent review of the 3.x backport (#893), which copies master's workflow unchanged.
Proposed change
Run the SQLAlchemy and Spark tests when shared core modules change, not only their own packages. Options:
- Add the core modules to both patterns. For example, treat
^pyathena/[^/]+\.py$ and ^pyathena/aio/[^/]+\.py$ (top-level modules such as util.py, common.py, connection.py, cursor.py, formatter.py, converter.py, model.py, error.py, aio/util.py) as shared for both.
- Invert the rule: skip the SQLAlchemy or Spark tests only when every changed file is under a package they do not import (for example
pyathena/filesystem/, pyathena/pandas/, docs/).
Option 1 is the smaller change and keeps the cost savings for pull requests limited to the result-set packages or to other features. Apply the same change to the 3.x branch afterwards, whose workflow matches master's after #893.
Validation plan (if implementing)
- Check the
changes job output on a Draft-to-Ready pull request that changes only pyathena/util.py: both sqla and spark should be true.
- Check that a pull request changing only
pyathena/filesystem/ still skips them.
Use case
The Test workflow's
changesjob (#837) runs the SQLAlchemy tests and the Spark tests of a ready pull request only when files under their own packages change (.github/workflows/test.yaml:125-127):Both packages depend on modules outside those paths. Their direct imports on master:
pyathena/sqlalchemy/,pyathena/aio/sqlalchemy/):pyathena,pyathena.cursor,pyathena.aio.cursor,pyathena.aio.connection,pyathena.converter,pyathena.error,pyathena.formatter,pyathena.model,pyathena.util.pyathena/spark/,pyathena/aio/spark/):pyathena,pyathena.common,pyathena.error,pyathena.model,pyathena.util,pyathena.aio.util.Through
connect()and the cursor base classes, they also depend onpyathena/connection.pyandpyathena/common.py.So a ready pull request that changes only one of those modules does not run the SQLAlchemy compliance suites,
tests/pyathena/(aio/)sqlalchemy/, or the Spark tests. For example, a change tostrtoboolinpyathena/util.pycan break SQLAlchemy URL option parsing, and a change toretry_api_callcan break Spark session calls, without CI noticing before merge. The weekly schedule and the Release workflow run every suite, so such a regression surfaces only after merge or at release time.This was found by the independent review of the 3.x backport (#893), which copies master's workflow unchanged.
Proposed change
Run the SQLAlchemy and Spark tests when shared core modules change, not only their own packages. Options:
^pyathena/[^/]+\.py$and^pyathena/aio/[^/]+\.py$(top-level modules such asutil.py,common.py,connection.py,cursor.py,formatter.py,converter.py,model.py,error.py,aio/util.py) as shared for both.pyathena/filesystem/,pyathena/pandas/,docs/).Option 1 is the smaller change and keeps the cost savings for pull requests limited to the result-set packages or to other features. Apply the same change to the
3.xbranch afterwards, whose workflow matches master's after #893.Validation plan (if implementing)
changesjob output on a Draft-to-Ready pull request that changes onlypyathena/util.py: bothsqlaandsparkshould betrue.pyathena/filesystem/still skips them.