Skip to content

Correct user guide claims that contradict the implementation - #940

Merged
laughingman7743 merged 9 commits into
masterfrom
docs/927-user-guides
Oct 3, 2026
Merged

laughingman7743 merged 9 commits into
masterfrom
docs/927-user-guides

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

WHAT

This PR changes the docs/*.md user guides only. It corrects claims that do not match the code or Athena's measured behavior.

  • Examples that fail as written:
    • Wrong import modules for AsyncPandasCursor, AsyncArrowCursor, and AsyncDictCursor.
    • Missing imports: import pyathena for paramstyle, polars as pl, and typing.Any.
    • The Arrow custom converter left convert outside the class, so the class could not be instantiated.
    • connect(region_name=...) without a staging directory in filesystem.md.
    • A sync wrapper called on AioS3FileSystem(asynchronous=True).
    • Core select().from_statement(text(...)).
    • json.loads() on JSON values the converter has already decoded.
    • Cancellation examples that always waited out the full sleep. The SQLAlchemy one also called cancel() on a cursor without a query ID. Both now use threading.Timer, and the timer is cancelled once the query finishes.
  • Behavior descriptions:
    • as_polars() returns one DataFrame even with chunksize; the chunk examples now use iter_chunks().
    • The cache_size match rule now includes the SUCCEEDED state, schema and catalog, and the fact that it never matches with unload=True.
    • on_start_query_execution is described as applying only to the synchronous and aio cursors.
    • The aio fetch behavior is described per cursor, with asyncio.to_thread() for S3AioExecutor.
    • The scope of the Arrow timeouts and of UNLOAD is stated.
    • Spark get_std_out() / get_std_error() can return None.
    • AsyncSparkCursor is no longer described as an AsyncCursor.
    • The S3FS type table is corrected, and its complex-type row links to a new usage-type-hints anchor.
    • The SQLAlchemy callback timing is corrected, and the aio dialects are added to the callback list.
    • Table options that must be positive integers now say so.
    • INT replaces INTEGER in DDL.
    • The connection-pooling claim is removed.
  • Athena results, measured on 2026-10-03:
    • Anonymous ROW scalars stay strings.
    • CAST(ROW(...) AS JSON) without field names returns {"":...}, so the example now uses a named ROW type.
    • A top-level CAST('[1, 2, 3]' AS JSON) works, so the claim that it raises InvalidRequestException is removed.
  • Names: 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_id argument stays in filesystem.md and is tracked by #936.

TEST

Tested commit 89c8bf4:

  • markdownlint-cli2 on docs/**/*.md passes (0 errors).
  • sphinx-build -b html docs <out> gives the same warnings as master 9b74767.
  • Offline runs:
    • The corrected Arrow converter instantiates and converts.
    • The S3FS converter example runs.
    • The threading.Timer cancellation pattern cancels on timeout and returns at once when the work finishes first (fake cursor).
  • Athena runs (a few queries in the CI account):
    • CAST('{"a": 1}' AS JSON) comes back as a JSON string, and json_parse(...) as an object.
    • A top-level CAST('[1, 2, 3]' AS JSON) works through sqlalchemy.types.JSON.
    • Anonymous ROW(...) gives {"0": "John Doe", "1": "30", ...}.
    • CAST(ROW('John', 30) AS JSON) returns {"":"John","":30}.
    • A named ROW cast to JSON returns {"name": "John", "age": 30}.
    • CAST(MAP(...) AS JSON) returns {"price": "999", "rating": "4.5"}.
  • Other examples that need Athena were checked against the code, not run. The changes are Markdown only, so no AWS integration tests are needed.

🤖 Generated with Claude Code

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>
Comment thread docs/aio.md Outdated

- `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

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 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() needs query_id, which is set before polling.
    • The SQLAlchemy example sets it through the query_id setter (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; no AsyncCursor variant 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: auto resolves to pyarrow, and any other engine raises.
  • Polars: as_polars() always returns a DataFrame, and iter_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>
Comment thread docs/s3fs.md
| 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 |

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 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_sql always writes Parquet through pyathena/pandas/util.py, which imports pyarrow.
  • Pandas unload: auto resolves to pyarrow, and any other engine raises.
  • No AsyncCursor variant calls _call_on_start_query_execution.
  • Polars iter_chunks() on the cursor is a generator over PolarsDataFrameIterator.
  • AioS3FileSystem() builds its own client when no connection is given.
  • Obsolete-prose search across docs/*.md, README, and root Markdown: no remaining AthenaResultSetObject, convertes, from_statement, InvalidRequestException, connection pooling, PolarsDataFrameIterator return claim, or >= 0 option values. The README aio example already uses async with await aio_connect(...).
  • Evidence: markdownlint passes (0 errors). sphinx-build on 855fd11 gives the same warnings as master, and the new {ref} resolves in s3fs.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>
Comment thread docs/usage.md Outdated
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.

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

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

  1. docs/usage.md:288 — Cache matching does not always include substituted parameters.
    For qmark, common.py:1189 preserves the SQL text and sends parameters separately. The cache compares only query text, schema, and catalog at common.py:1124. Running WHERE id = ? first with ["1"], then with ["2"] and caching enabled, can reuse the first result. The new wording incorrectly implies parameter-sensitive matching.

  2. docs/aio.md:216 — chunksize does not make pandas UNLOAD results lazy.
    The new text says pandas fetches read S3 lazily when chunksize is set. However, pandas/result_set.py:784 selects the Parquet path, whose read at line 751 loads the complete result without using chunksize. A reader choosing AioPandasCursor(unload=True, chunksize=...) for a result larger than memory can exhaust memory during execute().

  3. docs/aio.md:229 — Omitting chunksize does not guarantee an already-loaded pandas result.
    With auto_optimize_chunksize=True, pandas/result_set.py:522 can select chunked CSV reading while _chunksize remains None. 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.

  4. 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 calls next() on that iterator. Executing the shown sequence with AioPandasCursor therefore raises StopIteration after fetchall(), instead of producing df.

  5. docs/spark.md:414 — The example still assumes output methods always return futures.
    The added explanation correctly documents None, but both following calls dereference .result() unconditionally. spark/async_cursor.py:166 and line 185 return None when the corresponding location is absent. A calculation without a stderr location makes the example raise AttributeError.

  6. docs/usage.md:428 — The rewritten timeout example retains an ambiguous SQL column.
    Both daily_metrics dm and user_segments us expose user_id, but the outer aggregation uses COUNT(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 as dm.user_id.

  7. docs/usage.md:289 — unload=True does not guarantee a cache miss.
    formatter.py:175 only wraps text starting with SELECT or WITH; leading comments prevent wrapping. The unchanged SQL remains eligible for cache matching. Repeating /* report */ SELECT ... with caching and unload=True can therefore return an earlier result, contrary to “the cache never matches.”

  8. docs/polars.md:363 — The replacement concatenation example is not equivalent for an empty iterator.
    polars/result_set.py:153 explicitly makes as_polars() return an empty DataFrame when no chunks exist. An empty UNLOAD manifest produces that condition through line 648. Applying the documented equivalent instead calls pl.concat([]) and raises ValueError.

  9. docs/aio.md:7 — The thread-pool correction remains incomplete.
    The introduction still claims concurrency does not rely on thread pools. Boto3 calls use asyncio.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.

  10. 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 returns json.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.

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.

Repairs in 1cddff1 (all 10 findings verified against the code):

  1. usage.md cache rule: now says the query string is compared with pyformat parameters substituted. Verified: with qmark, _prepare_query keeps the text (common.py:1188-1190) and _find_previous_query_id compares only text, schema, and catalog. A cached result can therefore come from different ExecutionParameters. That is a code defect and will be filed as a bug instead of being documented.
  2. aio.md fetch bullet: lazy reads are limited to chunksize on CSV results (pandas and Polars) or on Polars UNLOAD results. Pandas UNLOAD reads the whole Parquet result (pandas/result_set.py:751).
  3. aio.md conversion paragraph: now keyed on whether execute() loaded the whole result, and covers auto_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.
  4. aio.md example: as_pandas() now runs after a new execute(), 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).
  5. spark.md: checks for None before .result().
  6. usage.md SQL: COUNT(DISTINCT dm.user_id).
  7. usage.md unload cache: limited to queries that are wrapped in UNLOAD. A leading comment prevents wrapping (formatter.py:175).
  8. polars.md: the equivalence now notes the empty-result difference (pl.concat([]) raises).
  9. aio.md intro: concurrency comes from asyncio tasks, and boto3 calls run on the loop's default executor.
  10. s3fs.md json row: 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.

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 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.

  1. 855fd11e..1cddff1e (session 01a0ffff-2eb0-71d3-aabd-e1944fac7471): 9/10 RESOLVED. The automatic-chunking iteration claim in aio.md was not resolved and was repaired in 7f0b100.
  2. 1cddff1e..7f0b1007 (session 01a10005-0610-70e3-b5c6-1bbb9ed89b8e): RESOLVED. New finding: pandas UNLOAD with chunksize is not lazy. Repaired in 56e1368 (CSV only).
  3. 7f0b1007..56e13686 (session 01a10008-325b-7390-8f7c-2d7bae94edb9): RESOLVED. New findings:
  4. 2e447f23..89c8bf45 (sessions 01a1000d-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.

laughingman7743 and others added 4 commits October 3, 2026 13:27
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>
@laughingman7743
laughingman7743 merged commit 23ff61f into master Oct 3, 2026
3 checks passed
@laughingman7743
laughingman7743 deleted the docs/927-user-guides branch October 3, 2026 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check the user guides and docstrings against the implementation

1 participant