Skip to content

Skip the result cache for qmark queries with parameters - #959

Merged
laughingman7743 merged 2 commits into
masterfrom
fix/941-qmark-cache-parameters
Oct 3, 2026
Merged

laughingman7743 merged 2 commits into
masterfrom
fix/941-qmark-cache-parameters

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

WHAT

Client-side result caching (cache_size / cache_expiration_time) no longer searches for a previous execution when a qmark query has parameters; such a query always starts a new execution.
BaseCursor._execute() and AioBaseCursor._execute() skip _find_previous_query_id() when the request carries ExecutionParameters.
Queries without parameters, including pyformat queries, keep the existing cache behavior.

The ExecuteOptions.cache_size and _execute() docstrings and the cache section of docs/usage.md state the new rule.

Release note (behavior change): cache_size / cache_expiration_time have no effect on qmark queries with parameters.
Before this change they could return the result of an execution that ran with different parameter values.
A repeated qmark query with the same parameters, which the cache used to reuse, now starts a new execution as well.

WHY

Closes #941.

With qmark, the query text keeps its ? placeholders and the values go to Athena as ExecutionParameters, so the cache search matched an earlier execution with the same text but different values.

The issue proposed comparing AthenaQueryExecution.execution_parameters with the current parameters.
Athena does not return them: measured on 2026-10-03 (work groups pyathena and primary, engine version 3), a query started with ExecutionParameters=["'1'"] has no ExecutionParameters key in either GetQueryExecution or BatchGetQueryExecution, also when read again minutes later.
The botocore 1.43.102 QueryExecution shape declares the field but documents it: "The list of parameters is not returned in the response."
Every earlier execution therefore looks parameterless, so a comparison could never match a parameterized query; it would only spend ListQueryExecutions / BatchGetQueryExecution calls.
Skipping the search gives the same results without those calls.

TEST

Tested commit: 9126987 (the later 9f3557c changes only one docs/usage.md sentence; just docs lint re-run on it: 0 errors)

  • just lint: passed.
  • just docs lint: 0 errors.
  • Offline (--noconftest with dummy environment variables): the new test_execute_qmark_parameters_skip_cache (sync and aio) and the existing cache-search and legacy-kwargs passthrough tests pass; with the pyathena/ change reverted, both new tests fail.
  • Against Athena: uv run --env-file .env pytest -n 1 tests/pyathena/test_cursor.py tests/pyathena/aio/test_cursor.py -k "cache or qmark or legacy_kwargs": 21 passed, including the new test_cache_size_with_qmark_parameters, which runs the issue's reproduction (different parameters) and checks that the same parameters also start a new execution.
  • Not run locally: the full just test pyathena suite and the SQLAlchemy suites; the change is limited to _execute() and is covered by the AWS CI once Ready.

🤖 Generated with Claude Code

laughingman7743 and others added 2 commits October 3, 2026 15:09
With the qmark paramstyle the query text keeps its placeholders and the
values go to Athena as ExecutionParameters, so the cache search matched
an earlier execution that ran with different values. Athena does not
return the ExecutionParameters of earlier executions, so the cache
cannot compare them; a query with parameters now always starts a new
execution.

Closes #941

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/common.py
query_id = None
# Athena does not return the ExecutionParameters of earlier executions,
# so the cache cannot tell which parameters an execution ran with (#941).
if not request.get("ExecutionParameters"):

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 1 (implementation behavior) — FINDINGS (1, repaired)

Scope: base e0e85da, reviewed head 9126987; all 6 changed files.

Covered:

  • Callers: every cursor (cursor.py, async_cursor.py, pandas/arrow/polars/s3fs sync + async, aio cursor/pandas/arrow/polars/s3fs) reaches the cache only through BaseCursor._execute() / AioBaseCursor._execute(), so the guard here and at pyathena/aio/common.py:92 covers all of them. Spark cursors do not use the cache.
  • Request shape: _build_start_query_execution_request() sets ExecutionParameters only when the list is non-empty, so qmark with None/[] and all pyformat queries keep the existing lookup; the global pyathena.paramstyle = "qmark" takes the same _prepare_query() path.
  • Behavior: return value, cursor state, on_start_query_execution, and interrupt handling are unchanged; a skipped lookup takes the same _start_execution() path as a cache miss. _find_previous_query_id() signatures are unchanged (dbt-athena 1.x does not call them; the legacy _execute() kwargs are untouched and the passthrough tests still assert the lookup call).
  • Tests: test_execute_qmark_parameters_skip_cache (sync/aio) fails with the pyathena/ change reverted; test_cache_size_with_qmark_parameters runs the issue's reproduction against Athena.

Finding: the added docs/usage.md:289 sentence explained why ("because Athena does not return ..."); user docs state behavior only, the rationale stays in the PR. Repaired in 9f3557c.

Deferred (pre-existing, not introduced): a qmark query text with ? placeholders executed later without parameters could still match an earlier parameterized execution of the same text and return its result instead of Athena's missing-parameter error. Detecting this would need SQL parsing; such a call is invalid input either way.

Comment thread pyathena/common.py
)
query_id = None
# Athena does not return the ExecutionParameters of earlier executions,
# so the cache cannot tell which parameters an execution ran with (#941).

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 2 (claims, compatibility, operations) — FINDINGS (PR description only, corrected)

Scope: base e0e85da, head 9f3557c; claims in the PR body, commit messages, this comment, the _execute() / ExecuteOptions.cache_size docstrings, docs/usage.md:289, and the issue premise.

Claims checked:

  • Issue premise "AthenaQueryExecution.execution_parameters exposes them" — false in practice. Measured 2026-10-03 in work groups pyathena and primary: neither GetQueryExecution nor BatchGetQueryExecution (the API the cache search uses) returns ExecutionParameters for a query started with them, although botocore 1.43.102's QueryExecution shape declares the field. This comment's claim holds for both measured work groups.
  • Round 1's deferral: measured, SELECT ? AS v with paramstyle="qmark" and no parameters fails with INVALID_PARAMETER_USAGE: Incorrect number of parameters: expected 1 but found 0, so the remaining pre-existing match only affects invalid calls.
  • "Queries without parameters, including pyformat, keep the existing behavior": _build_start_query_execution_request() adds ExecutionParameters only for a non-empty list; pyformat passes None.
  • Docs reader: the parameterized-query section (docs/usage.md:108-143) says nothing about caching; the cache section now states the rule; no other doc mentions qmark with the cache.
  • Existing caller: no signature changes. Repeated qmark queries with identical parameters used to get correct cache hits and now run again (more Athena queries for those callers). This was missing from the release note.
  • AWS operator: the change only removes ListQueryExecutions / BatchGetQueryExecution calls; no retry layer changes.
  • Evidence: the local AWS run (21 passed) and offline revert check belong to 9126987; 9f3557c changes one docs sentence only.

Corrections to the PR description: named the second measured work group, added the identical-parameters behavior change to the release note, and stated which commit the test results belong to.


@pytest.mark.parametrize("cursor", [{"work_group": ENV.work_group}], indirect=["cursor"])
def test_cache_size_with_qmark_parameters(self, cursor):
query = f"SELECT ? AS v -- {datetime.now(UTC)!s}"

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 (2; 1 rejected with evidence, 1 pre-existing and deferred)

Reviewer: Codex CLI 0.160.0, model gpt-6-astra (OpenAI), codex exec --sandbox read-only, session 01a10064-26ca-7751-a71d-935256265a41. Static review only: no builds, tests, edits, or network access. The prompt gave the base/head SHAs and the repository conventions, without the PR number, description, commit messages, or self-review records.
Snapshot: detached worktree at head 9f3557c, base e0e85da; afterwards it was still clean at the same HEAD, and the PR worktree was unchanged.

Covered (reviewer): the full diff; all sync, threaded-async, and aio SQL cursor paths (Dict, pandas, Arrow, Polars, S3FS); UNLOAD; legacy private-method signatures; cache matching; changed docs; relevant tests. The reviewer confirmed that the guard reaches every applicable cursor, private signatures are preserved, both mocked tests fail against the original code, and botocore's service documentation says the parameters are not returned.

  1. Introduced, P2 (this line): "SELECT ? AS v puts a parameter in the SELECT list, which Athena supports only in WHERE clauses, so the first execution fails." — Rejected. This test passed against Athena in the local run (-k "cache or qmark or legacy_kwargs": 21 passed) and returned [('2',)] / [('1',)]; the issue's live reproduction used the same SELECT ? AS v form.
  2. Pre-existing, P2 (pyathena/common.py:1125): the same ? SQL executed with parameters=None/[] and cache_size can reuse an earlier parameterized execution instead of letting Athena reject the missing binding. — Deferred, pre-existing (also present at the base SHA, as noted in round 1). Measured: Athena rejects such a call with INVALID_PARAMETER_USAGE: Incorrect number of parameters: expected 1 but found 0, so this only affects invalid calls. Skipping on unresolved ? markers would need a heuristic over the SQL text, which is outside this fix's scope.

No code change resulted, so no follow-up review is needed. The PR description now also quotes botocore's field documentation.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 06:19
@laughingman7743
laughingman7743 merged commit f3ef301 into master Oct 3, 2026
14 checks passed
@laughingman7743
laughingman7743 deleted the fix/941-qmark-cache-parameters branch October 3, 2026 06:52
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.

qmark queries reuse cached results that ran with different parameters

1 participant