Correct user guide claims that contradict the implementation - #940
Conversation
Fix the guide findings verified in #927: examples with wrong import modules, missing imports, an abstract converter class, connect() calls without a staging directory, Core select().from_statement(), json.loads() on decoded JSON, and cancellation examples that always wait out the timeout. Describe as_polars() with chunksize, the cache match rule, on_start_query_execution support, aio fetch behavior, Arrow timeout scope, ROW value types, and Athena's JSON results as measured. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| - `AioCursor` and `AioDictCursor` page through `GetQueryResults` as rows are fetched. | ||
| - `AioPandasCursor`, `AioArrowCursor`, and `AioPolarsCursor` download the result file (CSV or | ||
| Parquet) inside `execute()`, wrapped in `asyncio.to_thread()`. With `chunksize` (pandas and |
There was a problem hiding this comment.
Self-review round one (implementation behavior): FINDINGS (1, repaired)
Base 9b74767cf65e0ef513338fb2bb2da678cdfbcd2a, head 8bd91260c83e1b70d0906cc64f67d4b157b0d707 (full diff, 11 guide pages).
Finding: the new aio fetch bullet here said AioPandasCursor/AioPolarsCursor download the result file inside execute() and also read S3 lazily with chunksize.
With chunksize, the Polars result set builds a lazy scan_csv/scan_parquet and pandas builds a TextFileReader, so execute() does not download the whole file.
Repaired in 4d919ee: the download happens in execute() without chunksize, and fetch calls read S3 lazily with it.
Everything else checked against code:
- Timer-based cancellation in
usage.md/sqlalchemy.md:Cursor.cancel()needsquery_id, which is set before polling.- The SQLAlchemy example sets it through the
query_idsetter (result_set.py:966). threadsafety = 2.- A fake-cursor run cancels on timeout and returns at once otherwise.
on_start_query_execution: only the sync and aio cursors call_call_on_start_query_execution; noAsyncCursorvariant does.- Cache match: requires SUCCEEDED and DML, an exact query, schema and catalog (
common.py:1107-1129). With unload, the query is wrapped before the lookup and uses a fresh UUID. - Arrow timeouts reach only the pyarrow filesystem. HeadObject and the manifest GetObject use boto3 (
arrow/result_set.py:289,347). - Pandas unload engine:
autoresolves topyarrow, and any other engine raises. - Polars:
as_polars()always returns a DataFrame, anditer_chunks()yields chunks. - SQLAlchemy: zero values for bucket count and transform arguments are treated as unset (
compiler.py:1129,1431-1441,1521). The aio dialects forward the callback. - ROW/MAP/JSON values were measured on Athena (results in the PR body).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| | binary, varbinary | bytes | | ||
| | array, map, row (struct) | Parsed as Python list/dict using JSON-like parsing | | ||
| | varbinary | bytes | | ||
| | array, map, row (struct) | Parsed into Python list/dict (see {ref}`usage-type-hints` for the types of nested values); values too complex to parse are returned as the original string | |
There was a problem hiding this comment.
Self-review round two (claims, callers, documentation reader): FINDINGS (1, repaired)
Base 9b74767cf65e0ef513338fb2bb2da678cdfbcd2a, head 4d919eec2376f6c7fc57b23bf1576147245d7a61.
Finding: this row said nested scalar values "stay strings unless result_set_type_hints is given".
_to_array tries json.loads first (pyathena/converter.py:151-157), so a JSON-valid native value such as [1, 2, 3] comes back as ints. sqlalchemy.md already documents this for numeric arrays.
The fallback also returns None, not the original string, for text that does not look like the type.
Repaired in 855fd11: the row links to the type-hints section through a new usage-type-hints anchor and says only values too complex to parse stay strings.
Other claims checked:
to_sqlalways writes Parquet throughpyathena/pandas/util.py, which imports pyarrow.- Pandas unload:
autoresolves to pyarrow, and any other engine raises. - No
AsyncCursorvariant calls_call_on_start_query_execution. - Polars
iter_chunks()on the cursor is a generator overPolarsDataFrameIterator. AioS3FileSystem()builds its own client when no connection is given.- Obsolete-prose search across
docs/*.md, README, and root Markdown: no remainingAthenaResultSetObject,convertes,from_statement,InvalidRequestException, connection pooling,PolarsDataFrameIteratorreturn claim, or>= 0option values. The README aio example already usesasync with await aio_connect(...). - Evidence: markdownlint passes (0 errors).
sphinx-buildon 855fd11 gives the same warnings as master, and the new{ref}resolves ins3fs.html. - The Athena measurements in the PR body were made locally against the CI account, not in CI. Examples that need Athena and were not measured were checked against the code only.
- The tested commit in the PR body was updated to 855fd11.
…s guides Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| Results will only be re-used if the query strings match *exactly*, | ||
| and the query was a DML statement (the assumption being that you always want to re-run queries like `CREATE TABLE` and `DROP TABLE`). | ||
| Results will only be re-used from a succeeded DML query (the assumption being that you always want to re-run queries like `CREATE TABLE` and `DROP TABLE`) | ||
| whose query string matches *exactly* after parameters are substituted, and that ran with the same schema and catalog as the cursor. |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, read-only sandbox. This was a static review: no builds, tests, edits, or GitHub access.
Session: 01a0ffef-2c3f-78f3-b403-b02c70dcb515. Base 9b74767cf65e0ef513338fb2bb2da678cdfbcd2a, head 855fd11e23c1593d6d0a4a01a28cd850571d93c9.
Input: a detached snapshot plus the literal diff, without the PR number, description, commit messages, or earlier findings.
Limitation: Codex loaded its own local memory summaries at startup; none of them concern this change.
The snapshot stayed clean (git status empty after the run).
Reviewer output follows verbatim.
Covered all 11 changed guides: aio, arrow, cursor, filesystem, introduction, pandas, polars, s3fs, spark, sqlalchemy, and usage. Traced cursor inheritance, result sets, converters, caching, SQLAlchemy adapters, and filesystem executors. Checked changed fences, tables, and MyST references; no syntax defect found.
The literal diff matches the supplied range; HEAD remains clean. This was static inspection only—no builds, tests, or GitHub access.
FINDINGS
-
docs/usage.md:288 — Cache matching does not always include substituted parameters.
Forqmark, common.py:1189 preserves the SQL text and sends parameters separately. The cache compares only query text, schema, and catalog at common.py:1124. RunningWHERE id = ?first with["1"], then with["2"]and caching enabled, can reuse the first result. The new wording incorrectly implies parameter-sensitive matching. -
docs/aio.md:216 —
chunksizedoes not make pandas UNLOAD results lazy.
The new text says pandas fetches read S3 lazily whenchunksizeis set. However, pandas/result_set.py:784 selects the Parquet path, whose read at line 751 loads the complete result without usingchunksize. A reader choosingAioPandasCursor(unload=True, chunksize=...)for a result larger than memory can exhaust memory duringexecute(). -
docs/aio.md:229 — Omitting
chunksizedoes not guarantee an already-loaded pandas result.
Withauto_optimize_chunksize=True, pandas/result_set.py:522 can select chunked CSV reading while_chunksizeremainsNone. Consequently, as_pandas() at line 823 synchronously reads the next chunk. For a sufficiently large result, the documented convenience call blocks the event loop and returns only one chunk. -
docs/aio.md:225 — The retained fetch example exhausts the pandas iterator before conversion.
Row fetching shares_df_iter, established at pandas/result_set.py:331. Without chunking, as_pandas() at line 824 callsnext()on that iterator. Executing the shown sequence withAioPandasCursortherefore raisesStopIterationafterfetchall(), instead of producingdf. -
docs/spark.md:414 — The example still assumes output methods always return futures.
The added explanation correctly documentsNone, but both following calls dereference.result()unconditionally. spark/async_cursor.py:166 and line 185 returnNonewhen the corresponding location is absent. A calculation without a stderr location makes the example raiseAttributeError. -
docs/usage.md:428 — The rewritten timeout example retains an ambiguous SQL column.
Bothdaily_metrics dmanduser_segments usexposeuser_id, but the outer aggregation usesCOUNT(DISTINCT user_id). formatter.py:458 sends this SQL without qualifying identifiers, and cursor.py:197 surfaces query failure. Even with the required table present, Athena rejects the ambiguous reference. It needs qualification, such asdm.user_id. -
docs/usage.md:289 —
unload=Truedoes not guarantee a cache miss.
formatter.py:175 only wraps text starting withSELECTorWITH; leading comments prevent wrapping. The unchanged SQL remains eligible for cache matching. Repeating/* report */ SELECT ...with caching andunload=Truecan therefore return an earlier result, contrary to “the cache never matches.” -
docs/polars.md:363 — The replacement concatenation example is not equivalent for an empty iterator.
polars/result_set.py:153 explicitly makesas_polars()return an empty DataFrame when no chunks exist. An empty UNLOAD manifest produces that condition through line 648. Applying the documented equivalent instead callspl.concat([])and raisesValueError. -
docs/aio.md:7 — The thread-pool correction remains incomplete.
The introduction still claims concurrency does not rely on thread pools. Boto3 calls useasyncio.to_thread()at aio/util.py:44, as does S3AioExecutor. A reader expecting independence from the event loop’s default executor will instead find Athena operations queued behind other work when that executor is saturated. -
docs/s3fs.md:130 — The edited mapping table still restricts JSON results to dictionaries or lists.
s3fs/converter.py:57 uses the standard mappings, whose JSON converter returnsjson.loads()directly. For example,SELECT CAST(1 AS JSON)yields an integer. Reader code treating every non-null JSON result as a collection will fail on valid scalar JSON.
There was a problem hiding this comment.
Repairs in 1cddff1 (all 10 findings verified against the code):
usage.mdcache rule: now says the query string is compared withpyformatparameters substituted. Verified: withqmark,_prepare_querykeeps the text (common.py:1188-1190) and_find_previous_query_idcompares only text, schema, and catalog. A cached result can therefore come from differentExecutionParameters. That is a code defect and will be filed as a bug instead of being documented.aio.mdfetch bullet: lazy reads are limited tochunksizeon CSV results (pandas and Polars) or on Polars UNLOAD results. Pandas UNLOAD reads the whole Parquet result (pandas/result_set.py:751).aio.mdconversion paragraph: now keyed on whetherexecute()loaded the whole result, and coversauto_optimize_chunksize. The first-chunk-only behavior is the code defect PandasCursor.as_pandas() returns only the first chunk when auto_optimize_chunksize chunks the result #924.aio.mdexample:as_pandas()now runs after a newexecute(), so it no longer depends on the shared iterator (as_pandas() and as_polars() consume the result iterator, so repeated calls and later fetches return nothing #935).spark.md: checks for None before.result().usage.mdSQL:COUNT(DISTINCT dm.user_id).usage.mdunload cache: limited to queries that are wrapped inUNLOAD. A leading comment prevents wrapping (formatter.py:175).polars.md: the equivalence now notes the empty-result difference (pl.concat([])raises).aio.mdintro: concurrency comes from asyncio tasks, and boto3 calls run on the loop's default executor.s3fs.mdjsonrow: dict, list, or scalar.
Also fixed the same sync-wrapper restriction that Codex found in the #939 docstring (aio.md standalone usage).
Validation on 1cddff1: markdownlint has 0 errors, and sphinx-build gives the same warnings as master.
Both self-review perspectives on the repair: the claims are traced to the cited code, and the examples run in order without relying on the defects in #924/#935.
There was a problem hiding this comment.
Independent follow-ups (relayed)
Reviewer: Codex CLI 0.160.0, gpt-6-astra, read-only, static. Each round reviewed only the repair patch against the previous round's findings, and the snapshot stayed clean every time.
855fd11e..1cddff1e(session01a0ffff-2eb0-71d3-aabd-e1944fac7471): 9/10 RESOLVED. The automatic-chunking iteration claim inaio.mdwas not resolved and was repaired in 7f0b100.1cddff1e..7f0b1007(session01a10005-0610-70e3-b5c6-1bbb9ed89b8e): RESOLVED. New finding: pandas UNLOAD withchunksizeis not lazy. Repaired in 56e1368 (CSV only).7f0b1007..56e13686(session01a10008-325b-7390-8f7c-2d7bae94edb9): RESOLVED. New findings:- (a) The bullet missed automatic chunking. Repaired in 2e447f2.
- (b) S3FS loads every row through
GetQueryResultswith managed result storage. Repaired in 89c8bf4, which adds a managed-storage paragraph covering pandas, Arrow, Polars, and S3FS (*/result_set.pyelif STATE_SUCCEEDEDbranches). - (c) Row fetches consume the shared iterator before
as_pandas()/as_polars(). Deferred by decision: this is the code defect as_pandas() and as_polars() consume the result iterator, so repeated calls and later fetches return nothing #935. Check the user guides and docstrings against the implementation #927 asks for defects to be filed rather than documented, so the guide keeps the intended behavior. The reviewer noted the disagreement, and the decision is recorded here.
2e447f23..89c8bf45(sessions01a1000d-30bd-7630-90f5-59658425c761,01a1000f-f8f6-7d23-b36a-f2c4c081e92f): managed-storage finding RESOLVED, CLEAN apart from the deferred item.
Validation on 89c8bf4: markdownlint has 0 errors, and sphinx-build gives the same warnings as master.
The qmark cache-matching defect found in the first round is pending a decision on filing an issue; the guide no longer claims parameter-sensitive matching for qmark.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WHAT
This PR changes the
docs/*.mduser guides only. It corrects claims that do not match the code or Athena's measured behavior.AsyncPandasCursor,AsyncArrowCursor, andAsyncDictCursor.import pyathenaforparamstyle,polars as pl, andtyping.Any.convertoutside the class, so the class could not be instantiated.connect(region_name=...)without a staging directory infilesystem.md.AioS3FileSystem(asynchronous=True).select().from_statement(text(...)).json.loads()on JSON values the converter has already decoded.cancel()on a cursor without a query ID. Both now usethreading.Timer, and the timer is cancelled once the query finishes.as_polars()returns one DataFrame even withchunksize; the chunk examples now useiter_chunks().cache_sizematch rule now includes the SUCCEEDED state, schema and catalog, and the fact that it never matches withunload=True.on_start_query_executionis described as applying only to the synchronous and aio cursors.asyncio.to_thread()forS3AioExecutor.get_std_out()/get_std_error()can returnNone.AsyncSparkCursoris no longer described as anAsyncCursor.usage-type-hintsanchor.INTreplacesINTEGERin DDL.CAST(ROW(...) AS JSON)without field names returns{"":...}, so the example now uses a named ROW type.CAST('[1, 2, 3]' AS JSON)works, so the claim that it raisesInvalidRequestExceptionis removed.convertes→converter,AthenaResultSetObject→AthenaResultSet,expiration_time→cache_expiration_time, and the "As with AsyncXCursor" lines in the AsyncXCursor sections now point to the right cursor.WHY
Closes #927, together with #939 (docstrings). The audit results are in #927 (comment).
Readers copy these examples, so a wrong claim turns into a failing call.
Code defects found by the audit are filed separately (#934-#938). The guides keep describing the intended behavior for them.
For example, the
version_idargument stays infilesystem.mdand is tracked by #936.TEST
Tested commit 89c8bf4:
markdownlint-cli2ondocs/**/*.mdpasses (0 errors).sphinx-build -b html docs <out>gives the same warnings as master 9b74767.threading.Timercancellation pattern cancels on timeout and returns at once when the work finishes first (fake cursor).CAST('{"a": 1}' AS JSON)comes back as a JSON string, andjson_parse(...)as an object.CAST('[1, 2, 3]' AS JSON)works throughsqlalchemy.types.JSON.ROW(...)gives{"0": "John Doe", "1": "30", ...}.CAST(ROW('John', 30) AS JSON)returns{"":"John","":30}.{"name": "John", "age": 30}.CAST(MAP(...) AS JSON)returns{"price": "999", "rating": "4.5"}.🤖 Generated with Claude Code