BREAKING: Reject ineffective Spark arguments and unify environment fallbacks

[laughingman7743]

WHAT

SparkCursor, AsyncSparkCursor, and AioSparkCursor now raise NotSupportedError for any parameters value other than None, including an empty dictionary or list.
Their execute() methods no longer take work_group.
Passing it now raises TypeError through the unknown-keyword check added in #1102, even when the value is None.
Both checks run before any cursor state changes or any calculation request is sent.

Connection work_group now follows the same fallback rule as s3_staging_dir.
Only an omitted argument or None reads the environment variable.
An explicit empty string skips the fallback and resolves to None.
Both breaking changes have PyAthena 4.0 migration notes, and the API docstrings (including connect()) are updated.

WHY

Fixes #1096.
Before this change, Spark calculations ran with parameters and work_group silently ignored, and the two connection settings treated an empty string differently.
Keeping the existing empty-string behavior of s3_staging_dir keeps working configurations for managed query result storage unchanged.

TEST

Tested head: 6e9149616721c2b4e7a6ba356ab2b4ce5cb10108, rebased onto fa8febb3 (includes #1102 and #1103). Python 3.13, run locally.

• just format and just lint: passed (ruff, mypy, CloudFormation validation, license headers).
• uv run --env-file .env pytest --noconftest -p no:rerunfailures -q tests/pyathena/test_connection.py tests/pyathena/aio/test_connection.py tests/pyathena/spark/test_common.py: 298 passed, offline.
• uv run --env-file .env pytest --noconftest -p no:rerunfailures -q tests/pyathena/spark/test_spark_cursor.py tests/pyathena/spark/test_async_cursor.py tests/pyathena/aio/spark/test_cursor.py -k "rejects or accepts_none": 21 passed, offline. With the same test files on the master source, the 12 rejection cases fail and the 9 accepted-None controls pass.
• just docs lint: passed.
• I did not run the Spark AWS integration tests locally; the AWS CI run after Ready covers them.

Each new test fails on the code before the change:

• Spark rejection of {}, [], {"value": 1}, and work_group=None, tested in TestSparkCursor, TestAsyncSparkCursor, and TestAioSparkCursor.
• Fallback cases for work_group="".
• The ProgrammingError raised when both settings are empty strings.

Review: two self-review rounds and an independent static review by Codex are recorded inline on the reviewed head.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (implementation behavior): CLEAN.

Frozen diff: 971dd12..6af17de. Covered all 10 changed files: the shared Spark guard, all three Spark execute paths, Connection initialization, synchronous/async connection regression and signature tests, Spark regression tests, docstrings, and both migration documents. Traced Connection.cursor(), Spark session creation, calculation submission, polling, and AioConnection inheritance.

Checked non-None parameters including empty containers, removed work_group reaching **kwargs, validation before cursor-state mutation or calculation/polling submission, accepted None and positional call compatibility, supported Spark option forwarding and return shapes, missing/empty/nonempty environment values, early ProgrammingError when both settings are disabled, and existing cancellation/interrupt behavior. No actionable defects, repairs, or deferrals in this scope.

Author validation: lint passed, 320 affected offline tests passed, and 18 existing offline interruption/cancellation tests passed. The new regressions failed on the original source. This review is source inspection; AWS integration remains pending.

[laughingman7743]

Self-review round two (claims, compatibility, and operational behavior): CLEAN.

Frozen diff: 971dd12..6af17de. Audited the PR body, commit claim, all changed docstrings, both migration documents, and regression evidence across the full 10-file inventory.

Verified prior silent Spark behavior against the base revision, rejection through **kwargs including work_group=None, the guard's ordering before existing state/reset and submission paths, retained None/positional calls and supported options, and the unchanged successful return/cancellation contracts. Traced connect(), aio_connect(), AioConnection.create()/inheritance, Connection.cursor(), Spark session WorkGroup selection, and SQL request construction. Confirmed that explicit empty connection settings disable environment fallback, both disabled settings raise before session construction, and s3_staging_dir='' still supports managed query result storage.

Checked migration instructions against callers and searched related docs for obsolete claims. Documentation builds passed; migration sections render, and warning messages also appear in the unchanged master build. Checked that the core-file workflow filters select Spark/PyAthena plus synchronous and asynchronous SQLAlchemy suites after Ready. Local validation is explicitly offline and has not been described as AWS proof. No actionable findings or repairs; current-head AWS CI remains the final delivery gate.

[laughingman7743]

Relayed independent static review: CLEAN.

Reviewer: Claude Code, actual canonical model claude-opus-5-5, first-party Max subscription/profile, effort high. Session 02f2b152-845e-4069-9aa7-024001b86003. Frozen diff: 971dd12..6af17de.

The reviewer read the full literal diff and traced Connection fallback and cursor construction, AioConnection/factory forwarding, Spark session WorkGroup selection, the shared argument check and all three execute paths, regression and signature tests, and both migration documents. It found no actionable defects. Its introductory file count says 9; the actual diff contains 10 changed files and its listed coverage explicitly includes all 10.

The reviewer noted three non-actionable items: connect() has a less complete work_group docstring than Connection; other Spark **kwargs remain accepted and ignored as before; and an empty work_group in a SQLAlchemy URL now disables the environment fallback. The first two are pre-existing limits outside this bounded change, and the third is the documented breaking change.

Constraints: exported tracked source at the published head plus the literal diff only; no .env, credentials, .git history, memory, PR discussion, author conclusions, or commit messages. Read/Glob/Grep only, with customizations and MCP disabled and file access confined to the snapshot. No edits, commands, builds, tests, GitHub access, or delegation. Authentication was verified as first-party Max with provider/API overrides removed. The actual result reports no permission denials. Both snapshot and PR worktree remained unchanged. This is static source review, not runtime or AWS validation.

[laughingman7743]

Self-review round one (implementation behavior) — base dafee9a4264ae2bb765cc7c5e1e3ef842bd6cfb8, head 6d39f1489b9f0f2fae568858c8468f508845fc07. Result: FINDINGS, now repaired.

This review covers the branch after it was rebased onto master, which now includes #1102.

Covered: execute() in SparkCursor, AsyncSparkCursor, and AioSparkCursor; Connection.__init__ fallbacks for work_group and s3_staging_dir; callers of both fallbacks (connect(), aio_connect(), AioConnection.create(), and SQLAlchemy create_connect_args); the docs; and the tests.

• Since BREAKING: Reject unknown cursor keyword arguments #1102, the Spark execute() methods reject unknown kwargs through BaseCursor._validate_execute_kwargs. Removing work_group from their signatures is therefore enough to raise TypeError. The rebase drops the PR's separate SparkBaseCursor._validate_execute_arguments helper and keeps only an inline parameters is not None check. Both checks still run before any cursor state is reset or any calculation is started.
• Tests: the Spark cases now cover parameters {}, [], and {"value": 1}, work_group=None, and parameters omitted or None. The class-specific branches and the session_id/return-shape assertions, which this change does not touch, are gone. The connection fallback tests set both environment variables and list explicit expected values instead of normalizing them in the test body. The aio file keeps one test for argument forwarding, since AioConnection.create() runs the same __init__. Each new test fails on the code before the fix.
• Finding (repaired in 6d39f148): the work_group description in the connect() docstring (pyathena/__init__.py:101) did not mention the empty-string behavior.
• No change needed: SQLAlchemy URL parsing drops blank query values (?work_group= becomes {}), so the SQLAlchemy URL path behaves the same before and after the change.

[laughingman7743]

Rebase follow-up, self-review round one — new base fa8febb350f112c1200ba805ef94cb2e60910c87 (#1103 merged), new head 86f4443c6f3f742d250cf4a044a09c0056e05e0c. Result: CLEAN.

I compared git range-diff dafee9a4..6d39f148 fa8febb3..86f4443c after verifying that both old commits still exist. Commits 2 and 3 are identical. Commit 1 differs only in where the docs section sits in docs/usage.md: the conflict with #1103's new "Worker settings in PyAthena 4.0" section was resolved by keeping both sections, with the worker section first. #1103 changes only the pandas and Polars cursors, the benchmarks, and their tests. It does not touch Spark execute() or the Connection fallbacks.

Checks run at the new head: just lint and just docs lint passed, and the same offline pytest command (connection, aio connection, Spark common) gave 319 passed.

[laughingman7743]

Repair follow-up, self-review round one: 86f4443c → 169c8a03 → 6e914961 (base fa8febb350f112c1200ba805ef94cb2e60910c87). Tests only.

The Spark execute() argument tests were in tests/pyathena/spark/test_common.py under a TestSparkExecute class, which matches no class. They now live in TestSparkCursor, TestAsyncSparkCursor, and TestAioSparkCursor, next to the existing offline __new__-based tests in each file. The checks are in each cursor's execute(), not in spark/common.py, so that is where the tests belong. The shared fixture and the sync/aio _execute helper were removed with the old tests.

The independent follow-up found that the AsyncSparkCursor rejection test no longer checked that the previous calculation ID is kept. I verified the finding: AsyncSparkCursor inherits calculation_id from SparkBaseCursor, and the interrupt path sets it. Fixed in 6e914961: every rejection test, for both parameters and work_group, now sets _calculation_id = "previous" and asserts it is unchanged.

Validation:

• just lint passed.
• The moved tests: 21 passed. Copied onto the master source, the 12 rejection cases fail and the 9 accepted-None controls pass.
• The connection, aio connection, and Spark common tests: 298 passed, offline.

[laughingman7743]

Self-review round two (claims and callers) — base dafee9a4264ae2bb765cc7c5e1e3ef842bd6cfb8, head 6d39f1489b9f0f2fae568858c8468f508845fc07. Result: CLEAN.

Checked against the code:

• The fallback table: omitted or None reads the environment variable, "" becomes None, and a nonempty string is used as given.
• The ProgrammingError raised when neither setting resolves.
• The statement that this applies to connect(), aio_connect(), Connection, and AioConnection.
• The statement that s3_staging_dir="" behaves as before.
• The claims in docs/spark.md: NotSupportedError for any non-None parameters, TypeError for work_group even when it is None, and calculations running in the session selected by the cursor or by the session_id override.

Every Spark example in the docs sets work_group on the connection; none passes it to execute(). work_group passed when creating a cursor reaches StartSession through BaseCursor._work_group.

Local evidence at this head:

• just lint and just docs lint passed.
• uv run --env-file .env pytest --noconftest -p no:rerunfailures -q tests/pyathena/test_connection.py tests/pyathena/aio/test_connection.py tests/pyathena/spark/test_common.py: 319 passed, offline.

I did not run Spark AWS integration tests locally; they are left to the AWS CI run after Ready.

[laughingman7743]

Rebase follow-up, self-review round two — head 86f4443c6f3f742d250cf4a044a09c0056e05e0c. Result: CLEAN. The claims are unchanged. Reading docs/usage.md in order, the migration sections for keyword arguments, worker settings, and environment fallbacks each stand on their own, and nothing in #1103 contradicts the fallback or Spark statements.

[laughingman7743]

Repair follow-up, self-review round two: head 6e9149616721c2b4e7a6ba356ab2b4ce5cb10108. Result: CLEAN. The implementation and docs are unchanged. I updated the PR description's TEST section with the new test locations and commands, and its counts match the runs above.

[laughingman7743]

Independent review (relayed)

• Reviewer: Codex CLI 0.161.0, model gpt-6.1-sol, model_reasoning_effort=high, --sandbox read-only. Session 01a123cf-46aa-7f52-bd99-03c20d897f02.
• Scope: base dafee9a4264ae2bb765cc7c5e1e3ef842bd6cfb8, head 6d39f1489b9f0f2fae568858c8468f508845fc07, reviewed in a detached worktree snapshot. The prompt included no PR number, PR description, commit messages, or prior findings.
• After the review, the snapshot and the PR worktree were both unchanged at the reviewed head.
• This is a static review; the reviewer ran no builds or tests.

Reviewer output:

Covered surfaces:

• All three Spark execute() implementations; BaseCursor._validate_execute_kwargs; calculation submission, polling, state handling, and session/workgroup selection.
• Connection fallback resolution and _cursor; connect(), aio_connect(), AioConnection; synchronous and async SQLAlchemy connection argument forwarding.
• Changed documentation and docstrings against current and base source; repository docstring and test conventions.
• All changed tests, plus existing unknown-keyword/state-preservation tests and adjacent Spark failure-path tests.

CLEAN

No actionable regressions or pre-existing defects found in the reviewed paths.

Argument validation consistently precedes state mutation, calculation submission, and executor scheduling. Explicit empty connection settings suppress environment fallback, and the resolved values reach cursors without another fallback lookup. Documentation matches these behaviors.

By static comparison, the Spark rejection tests, empty-workgroup tests, and both-empty-settings test fail on the base behavior. Accepted-None and unchanged fallback cases appropriately remain passing controls. Tests use independently specified expectations and follow repository conventions; testing both wrappers and constructors provides useful forwarding coverage.

Small coverage gaps remain: non-None parameters supplied by keyword, preservation of a populated previous calculation execution, and combined invalid arguments exercising exception precedence. These do not establish an implementation defect.

Read-only, offline source review only. No builds or tests were run.

Author disposition: I accept the result. The coverage gaps it notes are not actionable:

• Passing a non-None parameters by keyword binds the same way as passing it positionally.
• Keeping the previous _calculation_execution follows from validation running before any state change. The generic unknown-keyword test from BREAKING: Reject unknown cursor keyword arguments #1102 also asserts that cursor state stays unchanged.
• The order of exceptions when several arguments are invalid is not documented behavior.

[laughingman7743]

Independent follow-up (relayed): Codex CLI 0.161.0, gpt-6.1-sol, effort high, --sandbox read-only, session 01a123de-afcc-7a11-98db-b94c12e9bbe1. This was a narrow follow-up on the range-diff dafee9a4..6d39f148 → fa8febb3..86f4443c plus the upstream diff between the two bases, reviewed in a detached snapshot that was unchanged afterward. Static review only.

CLEAN — no actionable findings. Code and tests are unchanged; the sole range-diff difference is documentation context around the newly added worker-settings section. Argument rejection still precedes calculation submission and state mutation. Upstream changes do not affect the Connection fallback contracts. The keyword, worker, environment-fallback, and managed-storage guidance remains coherent.

[laughingman7743]

Independent follow-ups (relayed): Codex CLI 0.161.0, gpt-6.1-sol, effort high, --sandbox read-only. Static review in detached snapshots, each unchanged afterward.

• 86f4443c..169c8a03, session 01a123f1-6a0e-7453-8aad-b654ccc754ef, result FINDINGS. "tests/pyathena/spark/test_async_cursor.py:263: the relocated rejection test omits both _calculation_id = \"previous\" and the assertion that cursor.calculation_id remains \"previous\"… if AsyncSparkCursor.execute() clears _calculation_id before rejecting parameters={}, the new test passes." It found no other issues: placement and mocks follow the neighboring tests, and nothing is left over in test_common.py.
• 169c8a03..6e914961, session 01a123f3-2546-7cf3-beff-ffc3f6ec5dcc, result CLEAN. "The previous finding is resolved… The relocated tests retain the removed shared tests' rejection cases, None acceptance variants, and calculation-call arguments across all three Spark cursors."