Skip to content

Document every public API in pyathena/ and check docstrings with ruff - #919

Merged
laughingman7743 merged 13 commits into
masterfrom
chore/882-ruff-pydocstyle
Oct 3, 2026
Merged

laughingman7743 merged 13 commits into
masterfrom
chore/882-ruff-pydocstyle

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

WHAT

  • Enable ruff's pydocstyle rules (D) with convention = "google" for pyathena/:
    • ignore-decorators = ["pyathena.util.override"]: methods marked as overrides (Mark every checkable override with @override #918) are exempt.
    • D105 (magic methods) is ignored.
    • "!pyathena/**" = ["D"]: files outside the package (tests/, scripts/, docs/conf.py) are not checked.
    • No per-file exceptions: every module, package, public class, function, method, property getter and __init__ in pyathena/ has a docstring.
  • Add the 508 missing docstrings (D102 367, D100 63, D107 63, D104 15; with the three D101 classes below, 511 definitions gain a docstring), one commit per package: model classes, result sets, S3 filesystem, SQLAlchemy dialects, connections/base cursors/shared modules, and the Arrow/pandas/Polars/S3FS/Spark cursors.
    • Property getters get a one-line "The ...." docstring that names the Athena or S3 response field they read.
    • __init__ docstrings describe every argument, including where *args/**kwargs go.
    • The fsspec overrides in filesystem/s3.py and s3_async.py are documented because they cannot carry @override (fsspec has no type information).
    • Code is unchanged. Stripping docstrings, every changed module's AST equals master's. The one exception is a pass removed from AsyncAdapt_pyathena_connection.rollback, which ruff's PIE790 flags once the method has a docstring.
    • Eleven package __init__.py files were empty and now contain a docstring, so they get the 2026 license header. Their earliest content is from 2026, so this follows docs/contributing.md.
  • Fix the findings that are not missing docstrings:
    • D417: describe *args of connect() and aio_connect(). aio_connect() forwards to AioConnection.create(), which takes keyword arguments only, so the docstring says so.
    • D415: first lines of DBAPITypeObject, BaseCursor.setinputsizes and setoutputsize (which also get Args:).
    • D101: S3File, AthenaDictResultSet, the SQLAlchemy test-suite Requirements.
  • Correct existing descriptions that the new docstrings touch:
    • Cursor.execute documents its seven missing arguments, in the wording the format cursors use. It is marked @override, and ignore-decorators skips D417 too, so ruff did not report them.
    • cache_size in AsyncCursor.execute ("cache size in MB") and AioCursor.execute ("cache size") is the number of recent queries searched for a reusable result.
    • on_start_query_execution was described as running right after StartQueryExecution, or "when the query starts". The cursors call it once _execute() returns a query ID, and with cache_size that can be a reused query that starts nothing. This is fixed in the ten execute() docstrings, ExecuteOptions, Connection, BaseCursor, and the "Query execution callback" section of docs/usage.md.
    • kill_on_interrupt in the new __init__ docstrings follows Re-raise and stop interrupted queries in the SQL cursors, sharing the Spark handling #853, by cursor family. The SQL cursors cancel the query on a KeyboardInterrupt while starting or waiting. The asyncio cursors cancel it when the task is cancelled while starting or waiting. The AsyncCursor variants cover only the start, because they wait on worker threads, which do not receive KeyboardInterrupt.
  • docs/contributing.md gets a "Write docstrings" section with the rules, and AGENTS.md (CLAUDE.md) links to it.

Pre-existing docstring errors found while writing these (wrong examples, attributes that do not exist, and similar) are not fixed here and will follow in a separate PR. Code bugs found on the way were verified and filed as #921, #922, #923, #924, #925 and #926.

WHY

Closes #882. Nothing checked docstrings, so coverage drifted after #601. Filling every gap now and enabling the rules without exceptions means ruff reports any new gap in pyathena/.

TEST

Tested commit: 6be315e (rebased on master 775874c)

  • just format, just lint: passed (ruff with the D rules and no per-file D exceptions, ruff format, mypy, cfn-lint, license headers).
  • just docs lint: 0 errors.
  • No code change: for each of the 83 changed modules under pyathena/, the AST with docstrings and pass statements removed equals master's.
  • sphinx-build -b html docs on master and on this head: no new warnings. The count drops from 151 to 145, because six warnings came from fsspec and SQLAlchemy docstrings that the API reference inherited and that the new docstrings now replace (S3File.__init__, S3FileSystem.cat_file, AioS3FileSystem.__init__, AthenaDDLCompiler.__init__).
  • Rule scope, checked on 06ad690 with temporary edits that were reverted:
    • A new pyathena/zz_tmp.py without docstrings reports D100 and D103.
    • An undocumented class appended to pyathena/util.py reports D101 and D102.
    • The same class in tests/pyathena/util.py reports nothing.
    • Malformed docstrings on private helpers report D415/D417.
  • ruff check pyathena --select D417,D415,D101 without ignore-decorators: no findings, so no marked override has an incomplete Args: section either.
  • Rendered API reference: PandasCursor.get_default_converter has no docstring of its own and shows BaseCursor.get_default_converter's, as the contributing guide states.
  • aio_connect("s3://x/") raises TypeError: AioConnection.create() takes 1 positional argument but 2 were given, as the new *args description states.
  • Not run locally: the AWS integration suites. Only docstrings, documentation and lint configuration change. The suites run in CI when the PR is marked Ready.

🤖 Generated with Claude Code

Comment thread pyproject.toml
[tool.ruff.lint.pydocstyle]
convention = "google"
# Overrides marked with @override inherit the base method's documentation.
ignore-decorators = ["pyathena.util.override"]

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): CLEAN

Base 9ef49d311a7124ef90a58703d02b14e612a17bdc, head 06ad69063963497fd2d6423ad9cfca296b18c067. All 11 changed files covered.

Covered:

  • Rule scope: !pyathena/** disables D outside the package, so tests/, scripts/ and docs/conf.py are not checked. Inside the package, a new file and an undocumented class are reported (mutations reverted). ignore-decorators exempts @override methods. D105 is ignored. The per-file entries match the measured findings (83 files, 508 findings) and do not hide the D101/D415/D417 codes.
  • Docstring facts checked against the code:
    • connect(*args) follows Connection.__init__'s parameter order (s3_staging_dir, region_name, ...) in both overloads and the implementation.
    • aio_connect(*args) raises TypeError because AioConnection.create(cls, **kwargs) takes keyword arguments only.
    • S3FileSystem._open returns S3File.
    • AthenaDictResultSet._get_rows builds rows with dict_type from the column names.
    • DBAPITypeObject.__eq__ uses other in self.
    • cache_size is the number of recent executions searched (common.py _find_previous_query_id), not a size in MB.
    • setinputsizes/setoutputsize are empty in BaseCursor. AsyncAdapt_pyathena_cursor.setinputsizes in the async dialect is a separate adapter method and is unaffected.
  • Runtime: only docstrings and configuration change. No code path changes.
  • Tests: none needed. The guard is just lint, and the rule scope was checked by mutation.

Findings: none.

@laughingman7743
laughingman7743 force-pushed the chore/882-ruff-pydocstyle branch from 06ad690 to 824650f Compare October 2, 2026 00:30
Comment thread pyproject.toml Outdated
"D102",
"D107",
]
# Missing docstrings (#882). Remove an entry once its file is documented.

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, callers, evidence): FINDINGS (repaired)

Base 9ef49d311a7124ef90a58703d02b14e612a17bdc, head 06ad69063963497fd2d6423ad9cfca296b18c067 → 824650f9b0582f93319b8bbbbc535dff0953ca38 (same tree, commit message only).

Finding: the PR body said that any other gap fails just lint, and the commit message said that new code cannot add more gaps. Both overstate the check. These ignores apply per file and per code, so a new undocumented method in a file that lists D102 is not reported. Verified: appending a documented class with an undocumented method to pyathena/model.py (which lists D102) passes ruff. The edit was reverted.
Repair: the commit message and PR body now say that a gap of an unlisted code, or in a file without an entry, fails; a listed code in a listed file is not reported until that entry is removed. The model.py case is added to TEST.

Other claims checked:

  • 83 files and 508 findings by code, recounted from pyproject.toml and ruff output.
  • ruff --select D417,D415,D101 without ignore-decorators passes, so no marked override has an incomplete Args: section. D417 only checks docstrings that have an Args: section, and the wording claims only that.
  • Contributing guide:
    • "the API reference shows the base method's docstring" was checked in the rendered HTML (PandasCursor.get_default_converter). Autodoc inherits docstrings by default (autodoc_inherit_docstrings is not set in docs/conf.py).
    • "mypy reports a missing decorator, except on an unannotated property" matches the minimal reproduction from Mark every checkable override with @override #918: an unannotated method is reported, an unannotated property is not. Untyped fsspec bases are covered by the next bullet.
  • Documentation reader: no other docs state docstring rules. CONTRIBUTING.md links to docs/contributing.md. The AGENTS.md anchor exists in the built HTML and in GitHub's heading slug.
  • Existing callers and AWS operator: not applicable. Only docstrings and lint configuration change.

Comment thread docs/contributing.md Outdated
- New or changed private functions and methods use the same style, although ruff does not check them.

`per-file-ignores` in `pyproject.toml` lists the files that still lack docstrings.
Remove a file's entry when its docstrings are complete.

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.159.3, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a0fa05-568a-7133-9633-2f680781c761. It did not author the change.
Scope: base 9ef49d311a7124ef90a58703d02b14e612a17bdc, head 824650f9b0582f93319b8bbbbc535dff0953ca38. The reviewer read a detached clean checkout and the literal diff. Its prompt left out the PR number, description, commit messages and earlier findings. Constraints: no edits, builds, tests, linters, network, GitHub or memory. This is a static review. The snapshot and PR worktree were unchanged afterward.

Reviewer's result (verbatim; local file links point to the review snapshot):

Surfaces covered:

  • Ruff selection, Google convention, D105, negated glob, combined per-file ignores, dotted decorator matching, and the pinned version.
  • Every changed docstring and its implementation, including connection forwarding, execution options, cache lookup, DB API no-ops, S3 files, dictionary results, and SQLAlchemy requirements.
  • Contributor instructions, override, mypy configuration, AGENTS.md/CLAUDE.md, and Sphinx/napoleon settings.

Verdict: FINDINGS

Introduced issues:

  1. docs/contributing.md:91 — Removing an entire entry can remove required naming exemptions.
    The entries for pyathena/sqlalchemy/compiler.py and pyathena/aio/sqlalchemy/base.py contain N802 and N801, respectively, alongside the new D codes. Following this instruction after completing their docstrings would expose existing names such as visit_TINYINT and AsyncAdapt_pyathena_cursor to lint failures. Instruct contributors to remove resolved D codes while preserving unrelated exemptions.

  2. docs/contributing.md:88 — Ruff still checks existing private docstrings.
    Private functions are exempt from missing-docstring requirements, but their supplied docstrings remain subject to applicable formatting rules. For example, an undecorated private helper under pyathena/ with """Return the result""" can trigger Google-convention D415 for missing terminal punctuation. Replace “ruff does not check them” with the narrower missing-docstring exemption.

Pre-existing issue within a touched docstring:

  • pyathena/cursor.py:117 — The callback also runs on client-side cache hits.
    _execute() can return an existing query ID without calling start_query_execution, but execute() invokes the callback unconditionally. With a matching cached query, monitoring code relying on this description could record a new execution that never occurred. Document invocation after obtaining either a new or cached query ID.

Based on my knowledge of Ruff’s documented behavior, the configuration otherwise matches the intent: the negated pattern excludes D rules outside pyathena/, matching ignores combine, and the qualified decorator name matches the imported override. The newly written API descriptions are consistent with their implementations; I found no introduced napoleon formatting defect or runtime behavior change.

This was static inspection only. Ruff’s exact remaining-findings inventory and rendered documentation were not executed or verified.

Author verification:

  1. Confirmed. compiler.py and aio/sqlalchemy/base.py keep N802/N801 in the same entries. Fixed in 0e3b2d8: the guide and the pyproject.toml comment now say to remove a file's D codes and keep its other codes.
  2. Confirmed. """Return the result""" on a private helper appended to pyathena/util.py reports D415 (reverted). Fixed in 0e3b2d8: ruff does not require private docstrings, but checks the format of those that exist.
  3. Confirmed and pre-existing. BaseCursor._execute returns a cached query ID from _find_previous_query_id without calling start_query_execution, but Cursor.execute calls the callback anyway. The same description is in the execute docstrings of 13 cursor modules, none of whose lines this PR changes. Deferred to the Restore docstring coverage and enforce it with ruff's pydocstyle rules #882 PR that fills those cursor modules, so that all 13 change together.

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.

Repair record

Commits: 0e3b2d861b1a983fb9468dd167cf20ba7830f066 and da04a626360852f13a7b825e13e11eca02e82312, on top of the reviewed head 824650f9. They change only docs/contributing.md and a pyproject.toml comment.

  • Findings 1 and 2: the guide says to remove a file's D codes and keep its other codes, and says ruff does not require private docstrings but checks those that exist. The wording is "checks the docstrings they have", not only their format, because D417 also applies: a private helper whose Args: section misses an argument reports D417 (temporary edit, reverted).
  • Finding 3: deferred as stated above. It is a pre-existing problem in 13 cursor modules.

Self-review of the repair:

  • Behavior: only documentation and a TOML comment change. just lint and just docs lint pass.
  • Claims: the N801/N802 entries were checked in pyproject.toml, and the private D415/D417 behavior on ruff 0.14.14 with temporary edits.

Independent follow-up, relayed: Codex CLI 0.159.3, model gpt-6-astra, effort high, sandbox read-only, session 01a0fa09-4fda-7711-a17b-22a5c5c2eeaf. Range 824650f9b0582f93319b8bbbbc535dff0953ca38..da04a626360852f13a7b825e13e11eca02e82312, static review:

Verdict: CLEAN. Both earlier findings are resolved: Guidance removes only D codes, preserving the shared N801/N802 naming exemptions. Private-function guidance correctly distinguishes docstring presence requirements from checks on existing docstrings, including D415/D417. The wording is consistent with the surrounding section and configuration. No new actionable inaccuracies found.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 2, 2026 00:42
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 02:16
@laughingman7743
laughingman7743 force-pushed the chore/882-ruff-pydocstyle branch from da04a62 to b59c120 Compare October 3, 2026 02:27
@laughingman7743 laughingman7743 changed the title Check docstrings in pyathena/ with ruff's pydocstyle rules Document every public API in pyathena/ and check docstrings with ruff Oct 3, 2026
Comment thread pyathena/filesystem/s3.py
def info(self, path: str, **kwargs) -> S3Object:
"""Return information about an S3 path.

Uses the directory cache first: a cached entry for the path is

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), full pass after the scope change: FINDINGS (repaired)

Base 2fcbcdbe (master after #853), head 358d15542471bdbc3cadb1678ba7d5c04c1397ae. Scope: all 87 changed files, including the 508 new docstrings written by six parallel helper agents (one per package). This is self-review by the authoring model.

Covered: every added docstring, read against the code it describes. I checked the non-trivial claims in the source:

  • model DataError keys and state constants; table-metadata compression order, row_format/file_format, and the case-insensitive checks.
  • result-set _pre_fetch, description, type-hint lookup by index and then by lowercased name, and as_pandas.
  • S3FileSystem.info/touch/rm/cat_file/invalidate_cache, S3File.__init__, S3Object.__init__/to_api_repr, and the AioS3FileSystem forwarding.
  • visit_struct/_enable_hive_column_ddl, visit_getitem_binary slice/step/index, and AthenaDialect.__init__ option precedence.
  • the Converter/RetryConfig jitter, AsyncPandasCursor.arraysize, the Spark default engine configuration, and cancel.
    The no-code-change check (AST without docstrings, against master) still passes for all 83 modules.

Findings, all repaired in 358d155:

  1. S3FileSystem.info said a cache miss goes to HeadObject. A cached listing of the path itself makes it a directory, and a cached parent listing without it raises FileNotFoundError, both without a request.
  2. S3File.__init__ said a large existing object is copied for append. It is copied only once a multipart upload starts.
  3. Seven new kill_on_interrupt descriptions predated Re-raise and stop interrupted queries in the SQL cursors, sharing the Spark handling #853 and said "while polling". They are now aligned by cursor family: SQL cursors cancel on an interrupt while starting or waiting; asyncio cursors cancel on task cancellation while starting or waiting; AsyncCursor variants cover only the start.

Limitations: docstrings that describe AWS API fields (statistics meanings, engine versions) follow the AWS API reference, not code. Pre-existing docstring errors and suspected bugs reported by the helpers are out of scope (see PR body).

Comment thread docs/usage.md
PyAthena provides a callback mechanism that allows you to get immediate access to the query ID
as soon as the `start_query_execution` API call is made, before waiting for query completion.
This is useful for monitoring, logging, or cancelling long-running queries from another thread.
When `cache_size` finds a reusable query, no new query starts, and the callback receives the reused query's ID.

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, callers, evidence), full pass: CLEAN after PR-body updates

Base 2fcbcdbe, head 358d15542471bdbc3cadb1678ba7d5c04c1397ae.

Claims checked:

  • Counts: an AST comparison with master finds 511 definitions that gained a docstring (78 modules/packages, 63 __init__, 279 property getters, 91 classes/functions/methods). That is the 508 D findings plus the three D101 classes. The PR body now states it that way (the helpers' self-reported totals were higher).
  • No code change: the AST without docstrings equals master's for all 83 modules. The only removed statement is the PIE790 pass.
  • Eleven previously empty __init__.py files now carry the 2026 header (checked with git show origin/master:<f> | wc -c). scripts/check_license_headers.py passes.
  • on_start_query_execution: all ten cursors call _call_on_start_query_execution right after _execute(), and _execute() returns a cached ID from _find_previous_query_id without StartQueryExecution. The new wording, and the sentence added to docs/usage.md (anchored here), match that. The same sentence is not added to docs/sqlalchemy.md, because the SQLAlchemy dialects never pass cache_size.
  • kill_on_interrupt: checked against BaseCursor._poll/_start_execution and AioBaseCursor after Re-raise and stop interrupted queries in the SQL cursors, sharing the Spark handling #853, and against docs/usage.md "Query cancellation on interrupt" and docs/aio.md "Task cancellation". The PR body bullet was stale (it named only two docstrings) and is updated.
  • Rendered docs on this head: 145 Sphinx warnings, none new against master's 151. ruff --select D417,D415,D101 without ignore-decorators passes.
  • Commit messages: the claims in Check every docstring..., Say when on_start_query_execution... and Align the new docstrings... match the diff.
  • Existing callers and AWS operator: only docstrings, Markdown and lint configuration change. Runtime behavior is unchanged (AST check).

Deferred, as agreed with the maintainer: pre-existing docstring errors go to a separate PR, and suspected code bugs will be verified and then filed as issues.

laughingman7743 and others added 12 commits October 3, 2026 11:50
Enable ruff's D rules with the Google convention for pyathena/. Methods
decorated with pyathena.util.override are exempt, and magic methods need
no docstring. Files outside the package are not checked.

per-file-ignores lists, per file, the codes that still have findings.
A gap of another code, or in a file without an entry, fails the check;
a new gap of a listed code in a listed file is not reported until that
file's entry is removed.

Fix the findings outside that list: complete Cursor.execute's Args,
describe *args of connect() and aio_connect(), end the first lines of
DBAPITypeObject, setinputsizes and setoutputsize with a period, and add
docstrings to S3File, AthenaDictResultSet and the SQLAlchemy test-suite
Requirements. Correct cache_size in the AsyncCursor and AioCursor
execute docstrings, which described it as a cache size rather than the
number of queries to check.

Document the docstring rules in the contributing guide.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The compiler and asyncio dialect entries also hold naming exemptions,
so completing a file removes its D codes, not the whole entry. ruff also
checks the format of private docstrings that exist, so the guide says it
only does not require them.

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>
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>
All public modules, packages, classes, functions, methods, property
getters and __init__ methods in pyathena/ now have docstrings, so the
per-file ignores for the D rules are no longer needed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cursors call on_start_query_execution once _execute() returns a query
ID, which is either a new query from StartQueryExecution or a reusable
query found through cache_size. The docstrings and the usage guide said
it runs right after StartQueryExecution or "when the query starts", which
does not hold for a cache hit.

Since #853, kill_on_interrupt also cancels a query whose start is
interrupted. AsyncCursor waits on worker threads, which do not receive
KeyboardInterrupt, so it only covers the start there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The new kill_on_interrupt descriptions predate #853: the SQL cursors
also cancel a query whose start is interrupted, the asyncio cursors
react to task cancellation while starting or waiting, and the
AsyncCursor variants only cover the start. S3FileSystem.info() also
treats a cached listing of the path as a directory and a cached parent
listing without it as missing, and S3File copies a large existing
object for append only once a multipart upload starts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 force-pushed the chore/882-ruff-pydocstyle branch from 358d155 to 35fad82 Compare October 3, 2026 02:51
Pandas chunksize only applies to CSV results (UNLOAD reads the whole
Parquet output), Polars reads lazily only from result files in S3, and
description is None only for INSERT, UPDATE, DELETE and MERGE, not for
every DML statement type.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/result_set.py
) -> list[tuple[str, str, None, None, int, int, str]] | None:
"""The DB API 2.0 column descriptions.

None without result metadata, or for ``INSERT``, ``UPDATE``, ``DELETE``, and ``MERGE``.

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), full scope: FINDINGS (repaired)

Reviewer: Codex CLI 0.159.3, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a0ffa8-c667-7720-8af3-70a955377565. It did not author the change. Scope: base 2fcbcdbe7ae01b31a4a3bb613e6bccdfcd382014, head 358d15542471bdbc3cadb1678ba7d5c04c1397ae, full diff (about 5200 lines). The reviewer read a detached clean checkout with the dependency sources in .venv available. Its prompt left out the PR number, description, commit messages and earlier findings. Constraints: no edits, builds, tests, network, GitHub or memory. This is a static review. The snapshot was unchanged afterward.

Since the review, the branch was rebased onto master 775874c (#920, docs only). git range-diff shows all 12 patches unchanged, and #920's cancellation wording does not conflict with the new docstrings.

Reviewer's result (verbatim; file links point to the review snapshot):

Surfaces covered: Read the full diff. Closely traced cursor constructors, callback/cache/interrupt behavior, result-set loading, S3 filesystem operations, SQLAlchemy adapters/types/compiler methods, and retry helpers. Sampled model and S3 response properties. Inspected Ruff configuration and Napoleon conventions statically; no checks were run.

Verdict: FINDINGS

  1. [P2] pyathena/aio/polars/cursor.py:81 — Chunking is not always lazy.
    The new description promises lazy reads whenever chunksize is set. Without output_location, AthenaPolarsResultSet instead calls _as_polars_from_api() during construction (pyathena/polars/result_set.py:262), which fetches every row and builds one DataFrame (:527). A user selecting chunksize=1000 for a large managed-results query would still load the entire result during execute(). Qualify the promise as applying to S3-backed results.

  2. [P2] pyathena/aio/pandas/cursor.py:82 — Pandas chunk-size documentation omits the UNLOAD limitation.
    With unload=True, _as_pandas() calls _read_parquet() (pyathena/pandas/result_set.py:785), which calls pd.read_parquet() without applying _chunksize (:751). The iterator then yields that entire DataFrame once (:86). Thus unload=True, chunksize=1000 does not produce 1,000-row chunks and can require memory for the full result. Limit this description—and the newly added equivalent at pyathena/pandas/async_cursor.py:102—to CSV reading.

  3. [P3] pyathena/result_set.py:374 — “None … for DML” is too broad.
    The implementation suppresses descriptions only for INSERT, UPDATE, DELETE, and MERGE substatement types (:60, :375). Athena also classifies SELECT as DML, as the existing SELECT test explicitly asserts (tests/pyathena/test_cursor.py:121). A reader could incorrectly conclude that SELECT results lack column descriptions. Name the four modifying statement types instead.

Author verification, all confirmed and fixed in 6be315e:

  1. AthenaPolarsResultSet calls _as_polars_from_api() when there is no output_location (polars/result_set.py:260-263). The chunksize description now says "result files in S3 are read lazily".
  2. _read_parquet() calls pd.read_parquet() without chunksize. The AioPandasCursor and AsyncPandasCursor chunksize descriptions now say "when reading CSV results".
  3. _DML_SUBSTATEMENT_TYPES is {INSERT, UPDATE, DELETE, MERGE}. The description docstring now names those four.
    After the repair: just lint passes, and the AST check without docstrings still equals master for all 83 modules.

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.

Repair record

Repair commit: 6be315ec4c87758f993778b0b2167696e848390a. It changes only the four docstrings named above.

Self-review of the repair:

  • Behavior: docstrings only. just lint passes, and the AST check still matches master.
  • Claims: checked against polars/result_set.py:260-263 (API fallback), pandas/result_set.py _read_parquet() (no chunksize), and _DML_SUBSTATEMENT_TYPES.

Independent follow-up, relayed: Codex CLI 0.159.3, model gpt-6-astra, effort high, sandbox read-only, session 01a0ffae-9fe5-7283-bae5-76c90fdc2fb4. Range 35fad823de412d8d5bb5d06bb067cf2535bd18e5..6be315ec4c87758f993778b0b2167696e848390a, static review:

CLEAN — all three earlier findings are resolved. No new inaccuracy or formatting problem found. Static inspection only; no execution-based validation.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 02:56
@laughingman7743
laughingman7743 merged commit 9b74767 into master Oct 3, 2026
15 checks passed
@laughingman7743
laughingman7743 deleted the chore/882-ruff-pydocstyle branch October 3, 2026 03:07
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.

Restore docstring coverage and enforce it with ruff's pydocstyle rules

1 participant