Read the dialect's own queries through an API cursor and resolve federated absence from information_schema - #798
Conversation
The dialect parses the rows of two queries it issues itself, and both ran through whichever cursor class the connection was configured with. Those cursors disagree on NULL and blank values, so: - get_view_definition() raised TypeError on pandas and polars when a view's DDL contained a blank line, and silently dropped that line on arrow. - the information_schema fallback needed a NaN guard, and wrote a Parquet UNLOAD to S3 for a five-row metadata lookup under unload=true. Both now open a plain Cursor. Reflection no longer inherits a result format chosen for user queries, and the NaN guard goes away with it. Also widen the fallback to a MetadataException that survived unwrapping. A federated catalog reports a missing table in its connector's own words, with no Glue error envelope, so has_table() raised OperationalError instead of returning False and create_all(checkfirst=True) failed against one. Deciding absence from information_schema keeps throttling and connector outages distinct from a genuinely missing table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The compliance suite pinned "an unwrapped MetadataException propagates" as a contract. That contract is what breaks has_table() on a federated catalog, which reports a missing table in its connector's words with no Glue envelope. Replace that case with one that asserts the fallback resolves it, for both a present and an absent table, and keep the recognized Glue codes propagating. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shared dialect now asks for the API cursor class positionally, which the async adapted connection did not accept, and it could not drive a synchronous Cursor anyway. Accept the argument and map it to the async counterpart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review found that an unrecognized MetadataException can also carry a permission failure: _get_error_code only unwraps the AmazonDataCatalog envelope, and information_schema filters by Lake Formation rather than erroring, so answering from it would report a table the caller cannot see as absent -- the same false absence this work exists to prevent. Glue states missing tables and permission failures in an envelope this client recognizes, so an unrecognized one there keeps propagating. Only a catalog that is not awsdatacatalog reaches the fallback, which is where the connector's own wording makes recognition impossible. Also pin the converter on the internal cursor, so a connection-level one chosen for a DataFrame cursor cannot reintroduce NaN comments, and cover a failed StartQueryExecution in get_view_definition: it raises DatabaseError, the parent of OperationalError, and must not be reported as a missing view. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This suite runs against the Glue Data Catalog, where an unrecognized MetadataException now keeps propagating, so the case removed earlier belongs here after all -- with a permission message, which is the reason it must not be re-asked of information_schema. The federated case it was replaced with has no catalog in this suite and is covered by the stub-cursor test instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- get_view_definition() caught result-paging failures as a missing view. A GetQueryResults page that exhausts its retries raises OperationalError from the same block, so an existing view was reported absent. Only the rejected query establishes absence now; the fetch runs outside that handler. - Restore the comment guard in the information_schema fallback. Pinning the converter on the internal cursor is not sufficient: Connection.cursor() applies cursor_kwargs after the explicit arguments, so a converter supplied there still reaches it and can report a missing value as NaN. - The blank-line test could skip on the very defect it guards: losing every blank row still left CREATE VIEW and UNION ALL, so both assertions passed and the skip blamed Athena's formatting. Compare the whole definition against the rows Athena returned through an API cursor instead, and skip only when those rows carry no blank line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| @staticmethod | ||
| def _without_throttling_retries(retry_config: RetryConfig) -> RetryConfig: | ||
| """Copy a policy without the codes that carry throttling. | ||
| def _answerable_from_information_schema(code: str | None, catalog: str | None) -> bool: |
There was a problem hiding this comment.
Self-review round one — implementation behavior (/code-review, high). Base 15325e172714e2e809dd5c597e8babb2c51a6fac (rebased; reviewed originally at d7e1dcd..580cf7a), head 205c9b14d4e152b39c810609455852f22d38512d. Surfaces: _get_columns fallback eligibility, _internal_cursor, _columns_from_information_schema, get_view_definition, the async adapter, the retry-policy derivation, and both test trees.
Result: FINDINGS, repaired.
- High, repaired in
82d5648.MetadataExceptionin the fallback set turned unparsed permission failures into false absence._get_error_codeunwraps only theAmazonDataCatalogenvelope, andinformation_schemafilters by Lake Formation instead of erroring, so it returns 0 rows for a table the caller cannot see. The fallback is now gated on the catalog not beingAwsDataCatalog, in_answerable_from_information_schema(line 388). - Medium, disproven by measurement. "The fallback lowercases, so a mixed-case federated table is missed." The DynamoDB connector reports
PyAthenaFederatedProbeMixedCaseaspyathenafederatedprobemixedcasein bothListTableMetadataandinformation_schema. No change. A case-preserving connector remains unverified (see PR Limits). - Medium, repaired by test. A failed
StartQueryExecutionraisesDatabaseError, the parent ofOperationalError, so it is not caught. That is correct: a request that never ran is not a missing view.test_get_view_definition_propagates_a_failed_requestpins it. - Low, repaired in
82d5648. A connection-levelconverterwould reach the internal cursor, so the default converter is now pinned. The follow-up is under the independent review below. - Low, repaired in
a3fbf4d. The propagation case for an unrecognized Glue error had been removed. It is restored with a permission message. - Low, repaired, then superseded by the independent review. The blank-line assertion depended on Athena's formatter.
Also found myself: the async adapter did not accept a positional cursor class, and could not drive a synchronous Cursor anyway. just test sqla-async failed 3 cases on it. Repaired in c61c361 by mapping Cursor to AioCursor.
There was a problem hiding this comment.
Renamed _answerable_from_information_schema to _is_fallback_error in 8e90677, to match _FALLBACK_ERROR_CODES and _without_fallback_retries. The docstring now also says the catalog, not only the code, decides the result. No behavior change.
| Column reflection and `has_table()` do not retry a table-metadata request that `information_schema` can answer; they read `information_schema.columns` instead, executed without query result reuse, and log a warning. | ||
| That covers a throttled request in any catalog. | ||
| It also covers a `MetadataException` carrying no recognized Glue error envelope, but only outside `AwsDataCatalog`: a federated catalog reports a missing table in its connector's own words, so absence is decided by the query against that catalog rather than by an unrecognized message. | ||
| In `AwsDataCatalog` an unrecognized `MetadataException` still propagates, because Glue does state missing tables and permission failures in a recognized envelope, and `information_schema` filters by Lake Formation instead of failing, so reading it there would report a table the caller cannot see as absent. |
There was a problem hiding this comment.
Self-review round two — claims and operational behavior. Base 15325e172714e2e809dd5c597e8babb2c51a6fac, head 205c9b14d4e152b39c810609455852f22d38512d. Claims audited: this docs paragraph, the docstrings of _internal_cursor and _answerable_from_information_schema, the commit messages, and the PR body.
Result: FINDINGS, corrected.
- The docs still said any
MetadataExceptionwithout an envelope goes to the fallback, after the code had been narrowed to non-Glue catalogs. Corrected in8ecd6bf. The paragraph now states the catalog boundary and the Lake Formation reason for it. - The claim "only outside
AwsDataCatalog" was not yet checked against S3 Tables, which is outsideAwsDataCatalogbut is Glue-federated. Measured live: a missing S3 Tables table returnsEntity Not Found (Service: AmazonDataCatalog; ...; Error Code: EntityNotFoundException; ...), so_lookup_tablemaps it before the fallback. S3 Tables behavior is unchanged. The PR body says so. get_view_definitionagainst a missing view was measured live on rest and pandas:NoSuchTableErroron both. The stub test for that path cites this.
Operator view: the fallback adds one information_schema query per unrecognized federated failure, instead of that failure's retry ladder. Internal queries no longer UNLOAD, which removes an S3 write and adds no query.
Deferred and stated in the PR: extra_info partition marking and case-preserving connectors on federated catalogs remain unverified. The raw pyathena.error.* from get_view_definition is not SQLAlchemy-wrapped. A cursor_kwargs converter still reaches the internal cursor.
| # Only a rejected query says the view is absent. A failure while | ||
| # paging the results is a failed read of a view that does exist. | ||
| raise exc.NoSuchTableError(f"{schema}.{view_name}") from e | ||
| rows = cursor.fetchall() |
There was a problem hiding this comment.
Independent review — relayed result. Reviewer: Codex, a different model from the author, through the repository's Codex rescue agent. Read-only static inspection, with no edits, tests or GitHub access. Reviewed d7e1dcd..5f87ed3; the rebased equivalent of that head is 8ecd6bf. Surfaces the reviewer read: all changed files, plus util.py, connection.py, common.py, cursor.py, converter.py and result_set.py, the async connection/cursor/adapter paths, and the pandas/arrow/polars cursor and result-set paths.
Result: FINDINGS, 3 items, all repaired in 205c9b1.
- P2.
fetchall()sat inside theOperationalError -> NoSuchTableErrorhandler. AGetQueryResultspage that exhausted its retries (result_set.py:370) reported an existing view as absent. This was a regression introduced by this branch: before it, iteration was outside the handler. Now onlyexecute()is inside the handler. - P2.
Connection.cursor()appliescursor_kwargsafter the explicit arguments, so acursor_kwargs["converter"]still overrides the pinned one. Dropping theisinstance(comment, str)guard had therefore reopened the NaN-comment path. The guard is restored at line 482, and the stub test has its NaN row back. - P2. The blank-line test could skip on the defect itself. Losing every blank row left
CREATE VIEWandUNION ALL, so both assertions passed and the skip blamed Athena's formatting. The test now compares the full definition against rows fetched through an API cursor (line 949), and skips only when those rows have no blank line.
A follow-up on 8ecd6bf..205c9b1 has been requested and will be recorded in reply.
There was a problem hiding this comment.
Independent follow-ups on the repairs (same Codex reviewer, read-only, static inspection).
Follow-up 1, 8ecd6bf..205c9b1: item 2 (comment guard) CONFIRMED-RESOLVED. Two new FINDINGS, both repaired in 185f582:
Cursor.execute()prefetches the first result page (result_set.py_pre_fetch), so narrowing the handler toexecute()still caught a failed firstGetQueryResultsas a missing view. Measured live: a missing view is a query Athena runs and fails (View not found or not a valid presto view: ...), raised asOperationalErrorwith__cause__ is None. An API failure is raisedfromitsClientError. Only the cause-less form maps toNoSuchTableErrornow. My earlier missing-view stub had modelled a rejectedStartQueryExecution, which actually raisesDatabaseErrorand never reaches this handler. It is replaced, and a failed-first-page test is added.- The live test's baseline is read through the same API-cursor path as the fix, so a loss on both sides would go unnoticed. The independent
CREATE VIEW/UNION ALLcontent checks are restored alongside the full comparison.
Follow-up 2, 205c9b1..185f582: stubs and live test CONFIRMED-RESOLVED. One remaining FINDING, recorded rather than fixed in bde5075: a CANCELLED query, or one FAILED for a reason other than a missing view, also raises a cause-less OperationalError and is still reported as absence. The reviewer confirmed this mapping predates the branch. Master mapped every OperationalError to NoSuchTableError; this branch only removed API failures from it. Distinguishing further would need a GetQueryExecution call on the failure path. I measured one: a missing view reports ErrorCategory 1 (SYSTEM) and ErrorType 1502, which does not single it out, so keying on it would be a guess. The comment now states the limit instead of implying a precise test, and the PR body lists it. No has-cause-but-failed-query path was found, sync or async.
Live after the repairs: a missing view gives NoSuchTableError on rest and pandas, and test_get_view_definition_across_cursor_types passes 5 of 5.
There was a problem hiding this comment.
Final independent follow-up, 185f582..8e90677 (same Codex reviewer, read-only): CONFIRMED-RESOLVED. There is no behavior change. The only edits are the rename, the docstring, and the new limit comment in get_view_definition(), and the decision logic of _is_fallback_error is unchanged. git grep finds no stale reference to the old name in tracked code, tests or docs. The new comment and docstring match the code: a failed or cancelled query raises a cause-less OperationalError and is read as absence, while API-layer failures keep their cause and propagate. The ErrorCategory/ErrorType measurement was taken as given, not re-derived.
Cursor.execute() prefetches the first result page, so narrowing the handler
to execute() still caught a GetQueryResults failure as a missing view.
Measured live, a missing view is a query Athena runs and fails ("View not
found or not a valid presto view"), which the cursor raises as
OperationalError with no underlying API error; an API failure carries its
ClientError as the cause. Only the former is absence now.
The stub for a missing view modelled a rejected StartQueryExecution, which
never reaches this handler (that raises DatabaseError). Model the failed
query instead, cover the failed first page, and restore the content checks
in the live test, since its baseline goes through the same API cursor path.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A cancelled query, or one that failed for another reason, also raises OperationalError without a cause and is still reported as a missing view. That mapping predates this branch, which only stopped API failures from taking it. The cursor does not keep the failed query's state, and a missing view reports ErrorCategory 1 / ErrorType 1502, which does not single it out, so the comment says so rather than implying a precise test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Match _FALLBACK_ERROR_CODES and _without_fallback_retries, and say in the docstring that the catalog, not only the code, decides the outcome. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WHAT
Two reflection defects with one cause: the SQLAlchemy dialect parses the rows of queries it issues itself, and it ran those queries through whatever cursor class the connection was configured with, or decided absence from an error message it could not read.
get_view_definition()on DataFrame dialects (#785).SHOW CREATE VIEWreturns the definition one row per line. Measured against a view whose formatted DDL contains a blank line:awsathena+restawsathena+pandasTypeError: sequence item 6: expected str instance, float foundawsathena+arrowawsathena+polarsTypeError: sequence item 6: expected str instance, NoneType foundThe dialect's own queries —
SHOW CREATE VIEWand theinformation_schema.columnsfallback — now open a plain API cursor through_internal_cursor(), with the default converter pinned, whatevercursor_classorunloadthe connection carries. The async adapter accepts a cursor class and mapsCursortoAioCursor. Underunload=truethe fallback also stops writing a Parquet UNLOAD to S3 for a five-row metadata lookup.A missing view is a query Athena runs and fails, raised without an underlying API error, and only that maps to
NoSuchTableError. A failedGetQueryResultscall propagates, including the first page thatexecute()prefetches. So does aDatabaseErrorfrom a failedStartQueryExecution.has_table()on Lambda federated catalogs (#781 / #783 item 1). Measured against a liveAthenaDynamoDBConnectorcatalog, a missing table comes back as:There is no
(Service: AmazonDataCatalog; ...; Error Code: EntityNotFoundException; ...)envelope, so since #777has_table()raisedOperationalErrorinstead of returningFalse, andcreate_all(checkfirst=True)failed against such a catalog. The wording is the connector's, so recognizing this string would not generalize.A
MetadataExceptionthat survives unwrapping now goes to the existinginformation_schemafallback, but only outsideAwsDataCatalog. The fallback runs against the same catalog through the same connector: 0 rows means absent, a failing connector fails the query too, so an outage is not reported as absence.Glue is excluded on purpose. It reports missing tables and permission failures in a recognized envelope, so an unrecognized message there has an unknown cause.
information_schemafilters by Lake Formation instead of failing, so answering from it would report a table the caller cannot see as absent — the false absence #777 exists to prevent._is_fallback_error()holds that boundary. An S3 Tables catalog is notAwsDataCatalog, but it was measured to return theAmazonDataCatalog/EntityNotFoundExceptionenvelope, so_lookup_tablemaps it before the fallback is reached and its behavior is unchanged.WHY
#781 asked whether #777's error handling and fallback hold on federated catalogs. Verified live against a temporary DynamoDB connector catalog; results are recorded on #781:
information_schema.columnsin a federated catalog: works. Same type names asGetTableMetadata, lowercase names, and 0 rows rather than an error for a missing table or schema. That last property is what makes the item 1 repair sound.ListTableMetadataExpressionover 128 characters: works. A 140-character name matched with the lowercase regex_lookup_tablesends, the original case, and a prefix regex.get_view_definition()was found while comparing both reflection paths after #784 and shares the root cause of the arrow comment defect repaired in #782. #782 normalized values at one column;SHOW CREATE VIEWloses whole rows on arrow, so no value-level normalization could recover them.Behavior changes
cursor_classset for instrumentation. The dialect's own queries always use the API cursor.Inspector.get_view_definition()can now raise a rawpyathena.error.*instead of a SQLAlchemy-wrappedDBAPIError, because it no longer goes throughConnection.scalars(). Theinformation_schemafallback has behaved this way since Fix metadata reflection throttling and reuse listed metadata #777.MetadataExceptionfromGetTableMetadatais no longer retried.information_schemais asked immediately instead, as for throttling._without_throttling_retriesis renamed_without_fallback_retries; the excluded code set is unchanged.test_metadata_errors_do_not_establish_absence[None]now uses a permission message. It still asserts propagation, which is correct because that suite runs against Glue. The federated case has no catalog in CI and is covered byTestAthenaDialect::test_unrecognized_metadata_error_asks_information_schema.Validation
just format,just lint(ruff, mypy, cfn-lint),just docs lintpass.TestAthenaDialect(no AWS): 11 passed.tests/pyathena/sqlalchemy/+tests/pyathena/aio/sqlalchemy/— 446 passed at the rebased head.just test sqla— 510 passed;just test sqla-async— 509 passed. Both suite runs were before the last review repair and rebase, which CI covers._internal_cursor()to the user's cursor failstest_get_view_definition_across_cursor_typesfor pandas, pandas+unload and arrow. That test compares the full reflected definition with the rows Athena returned through an API cursor, and skips only if Athena formats the view without a blank line.has_table('this_table_does_not_exist')changed fromOperationalErrortoFalse,has_tableon an existing table isTrue, andget_columnson the 140-character name returns the right columns.8e90677passed every job: lint, docs build, offline, and 5 each oftest,test-sqlaandtest-sqla-async./code-review(7 findings), a claims audit, an independent Codex review (3 findings), and two independent follow-ups. Every actionable finding was repaired, including two regressions this branch had introduced: result-paging failures reported as a missing view, and the comment guard dropped although acursor_kwargsconverter can still produce NaN. Records follow inline.Limits
extra_info = 'partition key', because a DynamoDB backend has no partition columns, and case-preserving connectors, because the DynamoDB connector lowercases names in bothListTableMetadataandinformation_schema. Both need a Hive or similar connector.convertergiven incursor_kwargsstill reaches the internal cursor, becauseConnection.cursor()appliescursor_kwargslast. Theisinstance(comment, str)guard keeps that from producing NaN comments.get_view_definition()still reports a CANCELLED query, or one FAILED for a reason other than a missing view, as absence. This predates the branch: master mapped everyOperationalErrortoNoSuchTableError, and this branch removes only API failures from that. The cursor does not keep the failed query's state, and a missing view reportsErrorCategory 1/ErrorType 1502, which does not single it out.get_table_comment()andget_table_options()still have no throttling fallback. Tracked in get_table_comment and get_table_options have no throttling fallback #786.Closes #781, closes #783, closes #785. Refs #777, #782, #784, #786
🤖 Generated with Claude Code