Fix/739 cache schema catalog#740
Conversation
| if ( | ||
| execution.query == query | ||
| and execution.database == self._schema_name | ||
| and execution.catalog == self._catalog_name |
There was a problem hiding this comment.
Thanks for the fix — the direction is exactly right. One issue with the catalog comparison, though: Athena normalizes catalog names to lowercase in query execution records. I verified this against live Athena: submitting Catalog: "AwsDataCatalog" to StartQueryExecution comes back as Catalog: "awsdatacatalog" from GetQueryExecution.
Since this line compares the recorded (lowercased) value against the user-supplied connection value, anyone connecting with catalog_name="AwsDataCatalog" (the casing used throughout the AWS docs) would never get a cache hit again — even for queries issued by the same connection. That's a silent caching regression.
A case-insensitive comparison is safe here: catalog names are case-insensitive in Athena. I also verified that creating a data catalog whose name differs only in case from an existing one fails with InvalidRequestException: DataCatalog ... has already been created, so two distinct catalogs can never differ only by case. Something like:
and (execution.catalog or "").lower() == (self._catalog_name or "").lower()This keeps the strict None-vs-value semantics (a recorded execution without a catalog still won't match a connection with one, which is the safe default) while fixing the case mismatch.
The database comparison is fine as is — Athena echoes the submitted Database value verbatim (no normalization), so queries issued through the same connection always match.
| if ( | ||
| execution.query == query | ||
| and execution.database == self._schema_name | ||
| and execution.catalog == self._catalog_name |
There was a problem hiding this comment.
Same catalog case-sensitivity issue as in pyathena/common.py — see the comment there. Athena records catalog names lowercased, so this comparison should be case-insensitive as well.
| "QueryExecutionContext": {"Database": schema}, | ||
| "Status": { | ||
| "State": AthenaQueryExecution.STATE_SUCCEEDED, | ||
| "CompletionDateTime": datetime.now(), |
There was a problem hiding this comment.
Nit: the synchronous counterpart in tests/pyathena/test_cursor.py uses datetime.now(timezone.utc) — please use the timezone-aware form here too for consistency. _find_previous_query_id sorts executions by completion_date_time, so mixing naive and aware datetimes in a future test extension would raise a TypeError.
| ) | ||
|
|
||
| cursor = Cursor.__new__(Cursor) # bypass __init__ to avoid AWS calls | ||
| cursor._catalog_name = None |
There was a problem hiding this comment.
The test approach itself looks good — it follows the same __new__ + patch.object pattern as the existing no-AWS unit tests in this file, and the negative + positive assertion pair guards against _find_previous_query_id's broad except Exception masking a setup mistake. I confirmed both new tests pass locally on this branch.
One coverage gap: with _catalog_name = None and no Catalog in the mocked execution context, the new catalog condition is only ever exercised as None == None, so the catalog half of the fix is untested. Could you extend this test (and the aio counterpart) with a catalog-mismatch case — e.g. an execution recorded with "Catalog": "some_catalog" while the cursor has a different _catalog_name? If the case-insensitive comparison suggested in pyathena/common.py is adopted, a case-difference case ("awsdatacatalog" recorded vs. "AwsDataCatalog" on the cursor, expecting a hit) would pin that behavior down too.
|
A note on the failed CI run: the failure is environmental, not caused by this PR. The test workflow authenticates to AWS via OIDC, and GitHub does not issue OIDC tokens to I've verified this branch locally instead: both new unit tests pass, and |
2 participants
WHAT
Check the database name and the catalog are the same for a request before allowing a query cache hit. See #739.
WHY
When the pyathena cache is checked for hits, queries from the given workgroup are checked for having the same query string, but NOT whether they were queried against the same Catalog or DatabaseName. Thus queries among multiple environments with the same table names and schemas (e.g., dev vs. prod catalogs or databases) will cause spurious cache hits from the other environment, possibly with erroneous data.