Repository navigation
BREAKING: Reject unknown cursor keyword arguments - #1102
Conversation
| """ | ||
| for name in kwargs: | ||
| if name not in allowed: | ||
| raise TypeError(f"{method}() got an unexpected keyword argument '{name}'") |
There was a problem hiding this comment.
Self-review round one — CLEAN (implementation and regression coverage).
Covered the complete 28-file change inventory: terminal constructor binding and forwarding through each cursor MRO; all sync, Future, and native asyncio execute entry points; validation before state reset, query/calculation start, and executor submission; Arrow/S3FS result-set overrides; pandas CSV/Parquet and Polars eager/scan reader names; SQLAlchemy API-cursor defaults and async adaptation; documentation and regression tests.
Verified the existing connection-default precedence and custom constructor extension coverage. The regression tests exercise all 21 cursor classes, assert unchanged state and no AWS requests on rejection, cover valid reader names and invalid reader modes, and keep unknown reflection defaults visible. No actionable implementation findings remain.
Validation: just format; just lint; 355 offline tests; Markdown lint; multi-version and working-tree Sphinx builds passed. This is a static self-review with the stated offline evidence; AWS validation is pending and is not inferred from CLEAN.
There was a problem hiding this comment.
Self-review round one repair follow-up — CLEAN.
Base: 23a54e1
Head: 0241fc2
Previous reviewed head: 09f7fb3
Checked both old objects and the bounded patch-series comparison, then traced all 11 repaired files into their callers and tests. The initial CLEAN record missed the independently identified reader-selection and test-factory regressions.
The six pandas/Polars execute methods now resolve the statement locally and validate its effective reader before resetting state, executing a query, or submitting work. Non-SELECT statements under unload=True retain CSV options. Polars accepts both possible CSV readers before result metadata is available and checks the actual reader at read time; renamed reader arguments remain accepted. Existing SQLAlchemy test doubles now implement the new private contract without weakening reflection, missing-table, cache, or error assertions. The live view-definition baseline uses a separate API connection. Spark tests seed actual calculation state, and internal-cursor factory failures now enter guarded cleanup.
Validation: just format, just lint, 428 offline tests, Markdown lint, and working-tree Sphinx build passed. No actionable repair findings remain. AWS-suite validation and independent follow-up remain pending; this CLEAN record does not claim either.
There was a problem hiding this comment.
Self-review round one second repair follow-up — CLEAN.
Base: 23a54e1
Head: 7496dcd
Previous reviewed head: 0241fc2
Verified the old commit objects and range-diff correspondence: the two prior entries are unchanged, followed by a runtime repair and a documentation clarification. Traced all 10 affected files and callers.
CSV dtypes replaces inferred schema_overrides without suppressing Polars deprecation handling; eager and scan readers preserve leading-zero strings, renamed columns, and timestamp fallback. Scan-specific option validation now runs before generator creation, so result-set creation fails deterministically rather than consuming a one-shot error. Preparation failures reset sync/native-async cursor state as before, while keyword rejection preserves it; Future-based cursors retain their no-reset contract. The fallback classifier is guarded for non-string SQL, and validation takes precedence over local preparation errors. Test mocks now represent an open prior result, and internal API-cursor tests configure a real backend converter to expose converter-pinning regressions.
Evidence: just format and just lint pass; 481 offline tests pass, including 48 preparation-error/name-priority combinations and actual dtype/renaming/timestamp reads. Markdown lint and working-tree Sphinx builds pass. The final commit after the offline run changes only documentation wording. No actionable implementation findings remain; independent follow-up and final AWS CI are pending.
There was a problem hiding this comment.
Self-review round one dtype repair follow-up — CLEAN.
Base: 23a54e1
Head: 6d6fe2f
Previous reviewed head: 7496dcd
Verified both prior objects and the patch-series comparison; all four preceding entries are unchanged. Reviewed the two changed files and traced dtype aliases through default merging, column renaming, timestamp fallback, eager/scan readers, and result-set construction.
The timestamp fallback now excludes explicit dtypes exactly as it excludes schema_overrides, and no longer removes the user’s alias. Tests use milliseconds rather than inferred microseconds, non-numeric data requested as Int64, and renamed timestamp columns; each fails identically under old/new names instead of silently returning inferred types. Existing successful default truncation and valid alias/column-renaming cases still pass. The new iterator test label now names the actual checked method.
just format and just lint pass. The full focused offline selection passes: 486 passed, with three expected library deprecation warnings for dtypes. No actionable implementation findings remain. Independent follow-up and current-head AWS CI are pending.
There was a problem hiding this comment.
Self-review round one wording follow-up — CLEAN.
Base: 23a54e1
Head: f2460c5
Previous head: 6d6fe2f
The range-diff preserves every preceding entry and appends only two test-docstring lines. No runtime code, assertions, parameter coverage, or public interfaces changed. The wording now names schema_overrides/dtypes and describes the guard directly. just format and just lint pass; the 486 offline passes on the preceding runtime head remain applicable. Independent wording follow-up and current-head AWS CI are pending.
| The validation happens before starting a query or a Spark calculation. | ||
| For native asyncio cursors, `execute()` validates arguments when awaited. | ||
| Pandas and Polars cursors continue to accept their reader options, with names checked against the installed library and the selected CSV, UNLOAD, and chunking mode. | ||
| Invalid values and combinations remain subject to the reader's validation. |
There was a problem hiding this comment.
Self-review round two — CLEAN (caller compatibility, operational effects, and claims).
Audited the full changed-file inventory and the PR/documentation claims against actual callers and the installed reader APIs. AST signature comparison confirms that existing execute and public connection/cursor signatures retain their positional order; only BaseCursor.init loses the ignored **kwargs. Async validation runs on await, while Future-based execution rejects unknown names before starting work. Shared constructor callbacks retain their existing support and invocation behavior.
Traced reader-name validation into CSV/UNLOAD and eager/scan paths, including pandas-to-PyArrow Parquet forwarding. Values and combinations remain reader-validated. Traced SQLAlchemy synchronous and async reflection, converter pinning, common default precedence, and preservation of unknown defaults. Core usage and backend module imports still work with optional reader imports blocked; constructing a default pandas converter still requires pandas, as before.
No retry, quota, or AWS performance claim is made. Local evidence is lint, 355 offline tests, and successful documentation builds. AWS suites and an independent static review remain pending; neither is claimed complete. No actionable compatibility or factual-claim findings remain.
There was a problem hiding this comment.
Self-review round two repair follow-up — CLEAN.
Base: 23a54e1
Head: 0241fc2
Previous reviewed head: 09f7fb3
Audited the updated PR description, docs/cursor.md, validator docstrings, deprecated-reader comments, test assertions, and the bounded patch-series comparison. The initial round-two record overstated selected-reader coverage; the repaired claims explicitly distinguish unknown-name rejection before AWS execution from CSV-mode-specific rejection after metadata determines managed-result fallback.
Checked unload=True DESCRIBE behavior against Formatter.wrap_unload and AthenaResultSet.is_unload, and checked eager/scan options plus renamed aliases against installed Polars reader sources. Rechecked sync, Future, and native asyncio call timing, state preservation, reader value-validation boundaries, unchanged public signatures, and shared callback behavior. The separate view baseline preserves an independent runtime comparison; real-connection tests still check converter pinning and default isolation. No retry, quota, or performance claims are made.
Evidence: 428 offline tests pass, including the old failing dialect unit cases and actual CSV reading with managed/eager options and deprecated row-index names. Lint and documentation checks pass. The interrupted AWS run is reported as interrupted with failures, not success. Required live suites and independent follow-up remain pending. No actionable compatibility or factual-claim findings remain.
There was a problem hiding this comment.
Self-review round two second repair follow-up — CLEAN.
Base: 23a54e1
Head: 7496dcd
Previous reviewed head: 0241fc2
Audited the revised documentation, reader-helper comments and docstrings, test expectations, and PR claims separately from implementation review. The docs now distinguish unknown-name rejection before AWS execution from mode-specific CSV checks during result-set creation, including Future.result() timing. No claim of successful AWS execution is made for either interrupted local run.
Checked alias behavior against installed Polars wrapper source and actual eager/scan output; ambiguous user-provided old/new dtype pairs remain reader-validated combinations. Checked restoration of original preparation-failure state behavior and error visibility, retained defaults/public signatures, independent SQLAlchemy view-baseline construction, configured-converter isolation, shared callbacks, and optional import boundaries. No retries, quota improvements, or performance changes are claimed.
Evidence: 481 offline cases passed on fbb2832; the only subsequent change clarifies one documentation sentence, with final lint/docs checks passing. The full AWS coverage will be obtained from current-head CI, whose tox configuration executes just test pyathena, just test sqla, and just test sqla-async. CI path filters select all three suites plus the PyAthena Spark and SQLAlchemy directories for this patch. No actionable compatibility or factual-claim findings remain. Independent follow-up and AWS CI remain pending.
There was a problem hiding this comment.
Self-review round two dtype repair follow-up — CLEAN.
Base: 23a54e1
Head: 6d6fe2f
Previous reviewed head: 7496dcd
Audited the changed helper/test docstrings and the reader-value guarantee against installed Polars wrapper source, the explicit-schema fallback guard, and real CSV parsing. The new name and its alias now have the same error visibility and requested-type behavior. Library validation of invalid values/combinations remains visible as OperationalError through the existing wrapper. The no-override timestamp precision fallback remains unchanged.
Checked the full two-file repair and necessary callers; constructor/execute signatures, state ordering, reader selection, managed CSV fallback, optional imports, and AWS request/retry behavior are unchanged by this repair. The record states the actual 486 offline passes and expected deprecation warnings and does not infer AWS success. No actionable compatibility or factual-claim findings remain. Independent follow-up and AWS CI remain pending.
There was a problem hiding this comment.
Self-review round two wording follow-up — CLEAN.
Base: 23a54e1
Head: f2460c5
Previous head: 6d6fe2f
Checked the changed claim against _get_timestamp_dtypes and its caller: both named options return an empty fallback mapping, so ComputeError is re-raised and wrapped as OperationalError. The text makes no claim about Polars inferring types or AWS/runtime validation. Existing default timestamp fallback is unchanged. No actionable factual-claim findings remain. Independent wording follow-up and AWS CI are pending.
There was a problem hiding this comment.
Final validation completed on reviewed head f2460c5. Ready-triggered AWS CI run https://github.com/pyathena-dev/PyAthena/actions/runs/37352006458 succeeded on Python 3.14.7: just test pyathena (3,744 passed, 12 skipped), just test sqla (589 passed, 759 skipped), and just test sqla-async (589 passed, 759 skipped). Current-head lint, documentation, license checks, and offline CI also passed. The PR is Ready, mergeable, and CLEAN; the reviewed head matches the CI head and the worktree is clean. These completed runs replace the earlier pending AWS evidence, not the independently recorded static review scope.
| return raw_connection.driver_connection.cursor( # type: ignore[union-attr] | ||
| Cursor, converter=Cursor.get_default_converter() | ||
| ) | ||
| return raw_connection.driver_connection._internal_cursor(Cursor) # type: ignore[union-attr] |
There was a problem hiding this comment.
Relayed independent static review — Claude Code, claude-opus-5-5, Max profile, effort high, first-party authentication (not Enterprise). Session: 42543ff2-b082-4c60-b3f9-176757cbc8fc.
FINDINGS — Medium: Existing dialect test doubles still provide only cursor(), so 13 information_schema and view-definition unit cases raise AttributeError after this call changes to _internal_cursor(). Affected factories are in tests/pyathena/sqlalchemy/test_base.py:185,353,390,443,483,576,607. Their old converter-call assertions also need updating. Verified by the local AWS-suite run. The production Connection and async adapter implement this method. A further integration baseline at test_base.py:1417 passes backend unload defaults to public Cursor and must instead create an isolated baseline connection.
There was a problem hiding this comment.
Author repair record.
Current reviewed head: f2460c5
Updated every affected dialect test factory to implement _internal_cursor(), preserving the existing reflection/error/cache assertions. The view-definition runtime baseline now uses a separate API Connection. Repaired in 0241fc2; all affected unit cases pass.
Both self-review perspectives and the applicable independent follow-ups are complete. The last code follow-up found no regressions; the final wording follow-up is CLEAN. Local focused offline tests: 486 passed on 6d6fe2f (only the two-line docstring clarification follows). Current-head lint/docs/offline CI has passed. AWS CI is still pending and will run after Ready.
| unload: Whether the cursor reads UNLOAD Parquet results. | ||
|
|
||
| Raises: | ||
| TypeError: If an argument is not supported by the selected reader. |
There was a problem hiding this comment.
Relayed independent static review — Claude Code, claude-opus-5-5, Max profile, effort high, first-party authentication (not Enterprise). Session: 42543ff2-b082-4c60-b3f9-176757cbc8fc.
FINDINGS — Low: Reader selection based only on the configured unload flag rejects valid CSV options for non-SELECT/WITH statements. For example, PandasCursor(unload=True).execute("DESCRIBE t", nrows=10) reads CSV but this validator selects Parquet. The corresponding Polars validator has the same mismatch. Also, chunked Polars queries without an output location use eager read_csv for managed results, so eager-only options are incorrectly rejected before execution. Reader choice must follow the prepared statement and allow the managed-result fallback until result metadata is known. The reviewer also identified an unverified deprecated-Polars-name compatibility risk; installed reader decorators confirm that aliases exist.
There was a problem hiding this comment.
Author repair record.
Current reviewed head: f2460c5
Reader validation now follows the prepared statement, so unload=True non-SELECT statements retain CSV options. Chunked Polars CSV allows managed eager options before metadata is known and validates the selected reader when constructing the result set. The constructor-stage check prevents one-shot generator failure. Repaired across 0241fc2 and fbb2832; regression cases pass.
Both self-review perspectives and the applicable independent follow-ups are complete. The last code follow-up found no regressions; the final wording follow-up is CLEAN. Local focused offline tests: 486 passed on 6d6fe2f (only the two-line docstring clarification follows). Current-head lint/docs/offline CI has passed. AWS CI is still pending and will run after Ready.
| async def test_unknown_execute_keyword_preserves_state(offline_connection, cursor_class): | ||
| cursor = cursor_class(**_constructor_kwargs(offline_connection, cursor_class)) | ||
| cursor._query_id = "previous" | ||
| cursor._result_set = MagicMock() |
There was a problem hiding this comment.
Relayed independent static review — Claude Code, claude-opus-5-5, Max profile, effort high, first-party authentication (not Enterprise). Session: 42543ff2-b082-4c60-b3f9-176757cbc8fc.
FINDINGS — Low: Spark preservation tests need a prior _calculation_id and _calculation_execution; their real fields currently stay unset and would not expose a misplaced reset. The reviewer also flagged internal-cursor test cleanup at lines 272-284. The specific claim that factory failure triggers UnboundLocalError is rejected: the factory call is outside try. Its actual failure path skips connection cleanup, and will be repaired with guarded cleanup. Tests/builds/AWS were not executed by the independent reviewer; the author verified these findings separately.
There was a problem hiding this comment.
Author repair record.
Current reviewed head: f2460c5
Spark preservation tests now seed the real calculation ID/execution fields. Internal-cursor creation runs inside guarded cleanup, and the prior result mock explicitly models is_closed=False. The original UnboundLocalError claim was rejected with source evidence; skipped connection cleanup was repaired.
Both self-review perspectives and the applicable independent follow-ups are complete. The last code follow-up found no regressions; the final wording follow-up is CLEAN. Local focused offline tests: 486 passed on 6d6fe2f (only the two-line docstring clarification follows). Current-head lint/docs/offline CI has passed. AWS CI is still pending and will run after Ready.
| allowed |= keyword_parameters(polars.read_csv) - {"source"} | ||
| # Polars still accepts these renamed parameters through its reader decorators. | ||
| for old, new in { | ||
| "dtypes": "schema_overrides", |
There was a problem hiding this comment.
Relayed independent repair follow-up — Claude Code, claude-opus-5-5, Max profile, effort high, first-party authentication (not Enterprise). Session: d2f8dd1e-ff00-4e4d-8d1f-cff51dab65ab.
Base: 23a54e1
Head: 0241fc2
Previous reviewed head: 09f7fb3
FINDINGS — Medium: The newly accepted dtypes alias collides with automatically supplied schema_overrides in both CSV readers. Polars rejects receiving both names; the result set wraps the failure as OperationalError after AWS execution. Verified against the installed reader decorators and default-merging code. This reproduces a base behavior but is a regression from the previous strict validation head. Preserve the alias by letting it replace inferred schema defaults, including renamed columns and timestamp fallback.
There was a problem hiding this comment.
Author repair record.
Current reviewed head: f2460c5
The dtypes alias replaces the automatically supplied schema_overrides default and follows the same column-renaming and timestamp-error rules as the new name. Valid eager/scan aliases preserve leading-zero strings and renamed columns; paired invalid-type cases preserve OperationalError. Repaired in fbb2832 and 6d6fe2f.
Both self-review perspectives and the applicable independent follow-ups are complete. The last code follow-up found no regressions; the final wording follow-up is CLEAN. Local focused offline tests: 486 passed on 6d6fe2f (only the two-line docstring clarification follows). Current-head lint/docs/offline CI has passed. AWS CI is still pending and will run after Ready.
| on_start_query_execution=on_start_query_execution, | ||
| result_set_type_hints=result_set_type_hints, | ||
| ) | ||
| operation, unload_location = self._prepare_unload(operation, options.s3_staging_dir) |
There was a problem hiding this comment.
Relayed independent repair follow-up — Claude Code, claude-opus-5-5, Max profile, effort high, first-party authentication (not Enterprise). Session: d2f8dd1e-ff00-4e4d-8d1f-cff51dab65ab.
Base: 23a54e1
Head: 0241fc2
Previous reviewed head: 09f7fb3
FINDINGS — Low: Moving reset below local option resolution and UNLOAD preparation keeps an old result after valid-keyword calls fail with an empty query or missing staging directory. Equivalent sync/native-async Polars and pandas methods are affected. Restore the previous reset on preparation failure while keeping keyword failures state-preserving. Future-based cursors retain their original no-reset behavior.
There was a problem hiding this comment.
Author repair record.
Current reviewed head: f2460c5
Preparation failures now restore the original reset/close behavior for sync and native-async cursors; keyword errors keep prior state and take precedence over local preparation errors. Future-based cursors retain their original no-reset behavior. All 48 combinations of cursor style, failure cause, and unknown-name presence pass.
Both self-review perspectives and the applicable independent follow-ups are complete. The last code follow-up found no regressions; the final wording follow-up is CLEAN. Local focused offline tests: 486 passed on 6d6fe2f (only the two-line docstring clarification follows). Current-head lint/docs/offline CI has passed. AWS CI is still pending and will run after Ready.
| """ | ||
| import polars as pl | ||
|
|
||
| validate_execute_kwargs( |
There was a problem hiding this comment.
Relayed independent repair follow-up — Claude Code, claude-opus-5-5, Max profile, effort high, first-party authentication (not Enterprise). Session: d2f8dd1e-ff00-4e4d-8d1f-cff51dab65ab.
Base: 23a54e1
Head: 0241fc2
Previous reviewed head: 09f7fb3
FINDINGS — Low: Scan-specific CSV validation is inside a one-shot generator. With a chunked S3 result and eager-only columns=, the first read raises TypeError, then subsequent reads silently return empty results. Move actual scan validation to result-set/iterator construction before creating the generator. This should fail execute(), await execute(), or future.result() deterministically. The initial repair also receives two minor nits: make the TypeError test match the offending name and fix its docstring line wrap; these will be addressed.
The reviewer covered the bounded repair and relevant source/tests/dependency contracts, ran no tests/code/AWS, and found no other actionable issues. Snapshot hashes and worktree status confirm no reviewer edits.
There was a problem hiding this comment.
Author repair record.
Current reviewed head: f2460c5
Actual scan-reader validation now runs in _create_dataframe_iterator before the generator exists. Invalid eager-only options fail result-set creation; repeated checks keep raising the same TypeError rather than exhausting a generator and returning empty rows. Repaired in fbb2832; repeated-error tests pass.
Both self-review perspectives and the applicable independent follow-ups are complete. The last code follow-up found no regressions; the final wording follow-up is CLEAN. Local focused offline tests: 486 passed on 6d6fe2f (only the two-line docstring clarification follows). Current-head lint/docs/offline CI has passed. AWS CI is still pending and will run after Ready.
| # Athena writes up to 12 fractional digits, which Polars does not parse | ||
| # into a Datetime whose time unit holds fewer, so the data is read again | ||
| # with the timestamp columns as text. | ||
| kwargs.pop("dtypes", None) |
There was a problem hiding this comment.
Relayed independent static repair review — Claude Code, claude-opus-5-5, Max profile, effort high, firstParty (not Enterprise). Session: 31c82e42-b828-45b6-ba27-9360c35a324a.
Base: 23a54e1
Head: 7496dcd
Previous head: 0241fc2
FINDINGS — Medium: Timestamp rereading still treats dtypes differently from schema_overrides. _get_timestamp_dtypes skips fallback for the new name but not the alias; the new pop then discards the user’s requested types. A timestamp result with dtypes={"v": pl.Int64} and non-numeric v silently returns strings instead of raising. Datetime("ms") is likewise replaced with inferred microseconds; renamed timestamp columns can be left as raw strings. Verified against the guard at pyathena/polars/result_set.py:495 and the new pop at line 661. Add dtypes to the guard, remove the pop, and assert the same reader failure under both names.
No other actionable defects were found in the bounded update. Covered alias/default merging, early iterator validation and repeated errors, all six preparation-failure paths and error priority, converter pinning, test assertions, and timing claims. Static review only: no tests, Polars, or AWS were executed. Snapshot hashes are unchanged. Two non-blocking observations are recorded: the new iterator test label is stale and will be corrected; preparation exceptions remain as TypeError traceback context, while the correct primary exception and state behavior are preserved.
There was a problem hiding this comment.
Author repair record.
Current reviewed head: f2460c5
Added dtypes to the explicit-override fallback exclusion and removed the destructive pop. Paired old/new-name cases cover milliseconds instead of inferred microseconds, invalid Int64 text, and renamed timestamp columns. Both names preserve reader failures. Repaired in 6d6fe2f.
Both self-review perspectives and the applicable independent follow-ups are complete. The last code follow-up found no regressions; the final wording follow-up is CLEAN. Local focused offline tests: 486 passed on 6d6fe2f (only the two-line docstring clarification follows). Current-head lint/docs/offline CI has passed. AWS CI is still pending and will run after Ready.
| GetQueryResults rows are mocked. | ||
| ``with_column_names`` is a scan-only option and is rejected by the eager reader. | ||
| Explicit schema overrides, including the dtypes alias, retain reader errors | ||
| instead of retrying with inferred types. |
There was a problem hiding this comment.
Relayed independent static dtype follow-up — Claude Code, claude-opus-5-5, Max profile, effort high, firstParty (not Enterprise). Session: 0118e756-fe6c-404b-a37c-b35e4f8abc74.
Base: 23a54e1
Head: 6d6fe2f
Previous reviewed head: 7496dcd
FINDINGS — Low wording inaccuracy only: tests/pyathena/polars/test_result_set.py:366-367 calls the old fallback types inferred. The fallback uses Athena metadata types, so the wording should say metadata types. No regressions were found in changed code.
Covered explicit dtype/alias exclusion, safety of removing the pop, preservation of caller types and errors, renamed columns, unchanged default fallback, real old/new-name test pairs, iterator test label, and dependency versions. Review was static: no code, tests, Polars or AWS were executed; parser error expectations were inferred from primary Python source and existing tests. Snapshot hashes and clean worktree status confirm no reviewer edits.
A possible full-schema parameter interaction was identified as pre-existing and unverified without the Rust parser; it is outside this keyword-validation repair and is not reported as a new defect.
There was a problem hiding this comment.
Author repair record.
Current reviewed head: f2460c5
Replaced the ambiguous type-source description with the directly checked contract: schema_overrides/dtypes disable timestamp retries and preserve reader errors. Only two docstring lines change in f2460c5. The final independent wording follow-up is CLEAN.
Both self-review perspectives and the applicable independent follow-ups are complete. The last code follow-up found no regressions; the final wording follow-up is CLEAN. Local focused offline tests: 486 passed on 6d6fe2f (only the two-line docstring clarification follows). Current-head lint/docs/offline CI has passed. AWS CI is still pending and will run after Ready.
| reads them, so they are not, and the read fails as before. No AWS calls; the | ||
| GetQueryResults rows are mocked. | ||
| ``with_column_names`` is a scan-only option and is rejected by the eager reader. | ||
| The ``schema_overrides`` option and its ``dtypes`` alias disable timestamp |
There was a problem hiding this comment.
Relayed independent review chain completed — CLEAN for the final wording repair.
Reviewer: Claude Code, claude-opus-5-5, Max profile, effort high; verified claude.ai/firstParty Max authentication (not Enterprise). Final session: d0876b58-8de1-4fd7-87e5-599db1db68f5.
Base: 23a54e1
Head: f2460c5
Previous head: 6d6fe2f
The initial full review and bounded repair reviews are recorded in their finding threads. Every verified finding has been repaired; the previous dtype follow-up found no code regression, and this final follow-up confirms the corrected option-name, retry-disable, and error-preservation claims. All earlier patch-series entries are unchanged. Both self-review perspectives also completed each repair.
The final reviewer was limited to Read/Grep/Glob and this clean snapshot, without edits, execution, tests, GitHub, PR context, commit messages, memory, or credentials. A Glob attempt against the parent temporary directory was denied; the review completed using permitted snapshot files. Snapshot hashes are unchanged, and the PR worktree remains clean at the reviewed head. This is static review, not AWS or runtime validation. The author has 486 offline passes; current-head AWS CI remains to run after Ready.
Actual narrow result:
CLEAN. I found nothing wrong with the two changed docstring lines (tests/pyathena/polars/test_result_set.py:366-367).
What I checked
- "disable timestamp retries":
_get_timestamp_dtypes(pyathena/polars/result_set.py:494-499) returns{}ifschema_overridesordtypesis a key inself._kwargs. Then_read_csv(pyathena/polars/result_set.py:657-660) re-raises instead of reading the file again with the timestamp columns as text (:661-668). So either option on its own turns the retry off. It's a presence check, which fits calling them "options". - "
dtypesalias": the alias mapping is atpyathena/polars/result_set.py:76-83. In_read_kwargs(:377-378), passing onlydtypesdrops the defaultschema_overrides, sodtypesis what reaches Polars. Both names are spelled exactly as in the code. - "preserve reader errors": the original
ComputeErroris re-raised and caught at:672-674, then raised again asOperationalError(*e.args) from e. The message and cause are kept, wrapped in the DB-API error type. That's what the tests check: all sixdtypes/schema_overridescases (:345-356) expectOperationalErrormatching"could not parse", and the plain{}case (:344) shows the retry working when neither option is set. - Coverage: the claim holds whether the bad column is the timestamp
t, the non-timestampv(Int64 on"x"), or a renamed column withnew_columns. The retry is disabled in each case, no matter which column caused the error.
Not reviewed, as you asked: the unchanged docstring lines (including the with_column_names sentence), runtime behavior, and entries 1–5 of the series.
Limitation: this was a static review of the snapshot only. I didn't run anything or check which exception types and messages the installed Polars actually raises, so "could not parse" and the ComputeError path were taken from the source and test assertions, not seen at runtime.
| if not getattr(self, "_unload", False) or not isinstance(operation, str): | ||
| return False | ||
| prefixes = ("UNLOAD",) if prepared else ("SELECT", "WITH", "UNLOAD") | ||
| return operation.strip().upper().startswith(prefixes) |
There was a problem hiding this comment.
Self-review round one, bounded DRY follow-up: CLEAN. Merge-base 23a54e1; published head 5b48a62; previous reviewed head f2460c5. Both old objects exist and the earlier patch entries are unchanged. Covered the shared predicate, all six pandas/Polars sync/Future/aio delegates, preparation success/failure, supported and unknown reader names, exceptions, previous-result cleanup, and the added public execute-path tests. SQL normalization and the distinct prepared/unprepared prefixes remain consistent with the previous paths; validation/reset/AWS ordering is unchanged for the supported contracts. No new imports or optional dependency requirements. just format and just lint passed; focused offline coverage passed 510 tests with three expected dtypes deprecation warnings. Full updated-head AWS validation remains pending.
| options = {"use_threads": False} if unload_reader else {"sep": "|"} | ||
| try: | ||
| with pytest.raises(ProgrammingError, match="s3_staging_dir is required"): | ||
| await _execute(cursor, operation=operation, **options) |
There was a problem hiding this comment.
Self-review round two, bounded DRY follow-up: CLEAN. Merge-base 23a54e1; published head 5b48a62; previous reviewed head f2460c5. Audited claims separately: the helper handles raw SELECT/WITH/UNLOAD only before preparation and prepared UNLOAD afterward, requires the cursor's unload setting, and tolerates invalid input during preparation-error validation. Existing supported reader options, return shapes, callback handling, error precedence, and no-AWS-on-unknown-name guarantees retain their call ordering. The 24 added cases check accepted library names on staging failure across all six execution variants; existing tests cover wrong-mode names, invalid operation/options, state preservation, and successful preparation. Current origin/master changes on these affected paths only make existing public parameters keyword-only and do not alter the new helper contract. Static compatibility review and 510 offline passes are distinct from AWS evidence; previous CI belongs to f2460c5 and is not claimed for this new head.
There was a problem hiding this comment.
Final validation for the DRY refactor completed on reviewed head 5b48a62. AWS CI https://github.com/pyathena-dev/PyAthena/actions/runs/37551498860 succeeded on Python 3.14.7: just test pyathena (3,891 passed, 12 skipped), just test sqla (589 passed, 759 skipped), and just test sqla-async (589 passed, 759 skipped). Current-head lint, documentation lint/build, license checks, and offline CI also passed. The PR remains Ready, mergeable, and has mergeStateStatus=CLEAN; the published head matches the reviewed/CI head and the worktree is clean. This completes the current-head AWS evidence previously recorded as pending; the independent review remains a separate static CLEAN verdict within its bounded scope.
| if not getattr(self, "_unload", False) or not isinstance(operation, str): | ||
| return False | ||
| prefixes = ("UNLOAD",) if prepared else ("SELECT", "WITH", "UNLOAD") | ||
| return operation.strip().upper().startswith(prefixes) |
There was a problem hiding this comment.
Relayed independent review: CLEAN within the bounded appended DRY update. Reviewer: Claude Code, actual canonical model claude-opus-5-5, firstParty provider, Max profile (verified claude.ai subscriptionType=max; not Enterprise), effort high. Session: 25c5728f-9356-4a84-957a-5e9c5bda0d8b. Old/new merge-base: 23a54e1; old head: f2460c5; new published head: 5b48a62. Range-diff confirms the previous six patch entries are unchanged. Covered the new predicate and twelve delegates across all six pandas/Polars execution variants, the added staging-failure tests, and necessary formatter/result-set/validator/MRO/state-management context. No actionable introduced defects found. The non-string return edge case requires a formatter violating its declared return contract; the pre-preparation assumption about custom formatter prefixes predates this update. The added tests specifically exercise preparation failure; existing tests also cover successful preparation and DESCRIBE. Static review only with Read/Grep/Glob: no commands, tests, builds, GitHub, memory, credentials, or subagents. Installed reader signatures were not separately inspected by the reviewer; author's 510 offline passes provide distinct runtime evidence. The exported snapshot excludes .env/.git, hash verification passed, and the PR worktree remained clean.
| from typing import Any | ||
|
|
||
|
|
||
| @lru_cache(maxsize=64) |
There was a problem hiding this comment.
Self-review round one — performance follow-up: CLEAN.
Review merge-base: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f; published head: 5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee.
Previous reviewed head: 5b48a622667322689dee3fc973a6d1c26e3e8353 at the same merge-base. Both old objects exist; git range-diff confirms all seven previous commits unchanged and one appended performance commit.
Covered all nine changed files and traced the leaf signature cache into pandas/Polars validators and synchronous/async SQLAlchemy internal cursor creation. Checked immutable cached names, fresh returned sets, bounded retention, unhashable-callable fallback, constructor replacement, all six reader cursor variants, preparation failures, validation-before-state/AWS ordering, effective chunk settings, and Unicode/whitespace prefix semantics. No actionable regressions found; there are no public signature or reader-option changes in this follow-up.
Validation: just format, just lint (including mypy, license headers, and CloudFormation validation), and the focused offline suite passed: 538 tests, three existing Polars dtypes deprecation warnings. Tests cover inspection reuse without mutable-set contamination, unhashable readers, constructor replacement, explicit Unicode classification cases, and skipping classification with empty keywords in both preparation-success and failure paths across all six variants. Prior reader-option and state-preservation coverage still passes.
Limits: author self-review, not independent review. Local tests block AWS requests. Cold cache misses still inspect signatures; in-place signature mutations require the documented private cache clear. Current-head AWS CI remains pending.
There was a problem hiding this comment.
Design consultation: Claude Fable 5.1 (claude-fable-5-1), effort high, verified first-party max profile (not Enterprise). Session: e756833b-6cc3-40cb-94f1-c69ad71443d0; completed successfully with the requested actual model/provider.
Read-only consultation on the tracked-source snapshot at merge-base 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f, previous head 5b48a622667322689dee3fc973a6d1c26e3e8353, plus the separate native keyword-only patch and local benchmark artifacts. Read/Grep/Glob only; no edits, commands, tests, GitHub access, credentials, memory, or subagents.
The reviewer suggested bounded signature-name caching, an empty-keyword fast path, and inspecting the SQL prefix. The authored follow-up implements these with immutable cache entries and fresh sets, retains the constructor MRO walk, and preserves existing Unicode uppercase semantics. The separate keyword-only API change does not remove the previous repeated introspection if reverted, so it remains outside this repair. Measurements and limits are recorded in the second self-review and PR description.
This was a design consultation before implementation, not the independent review of the resulting patch. The separate final Opus 5.5 Max static review is in progress.
| *reader_args: Additional reader settings needed by the validator. | ||
| prepared: Whether ``_prepare_unload()`` completed. | ||
| """ | ||
| if kwargs: |
There was a problem hiding this comment.
Self-review round two — performance follow-up: CLEAN.
Review merge-base: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f; published head: 5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee. Verified the frozen snapshot, published-head equality, both previous objects, and the seven unchanged commits plus one appended repair against previous head 5b48a622667322689dee3fc973a6d1c26e3e8353.
Claim inventory: installed-reader option compatibility; validation before state mutation/AWS; immutable bounded caching and replacement behavior; empty-keyword fast paths; prefix classification; measured performance; the separation from native keyword-only API binding; and actual review/CI status. Audited all nine repair files, their constructor/reader/reflection callers, unchanged cursor documentation, the PR description, commit, benchmark scripts, and raw measurements.
The cache stores signature-derived names, not query results, AWS metadata, or user values. Fresh sets isolate per-mode removals and Polars aliases; retaining the MRO walk observes replaced constructors. Optional dependency imports remain lazy. A miss still performs introspection, unhashable callables are uncached, and same-object signature mutation requires the documented explicit clear. All six variants retain preparation-error precedence, supported reader options, and their existing sync/async/Future state behavior. Prefix checks preserve existing whitespace/case/Unicode semantics; normal formatter processing remains unchanged.
Measured warm CPU costs on the same Python/library versions, 2,000 calls per repeat, seven repeats, median: pandas CSV validation 39.874 -> 1.107 microseconds; constructor-name discovery 153.367 -> 4.623; 100 KiB SQL predicate 32.556 -> 0.319. Actual synchronous execute paths with AWS and result readers replaced by fakes: pandas CSV/dtype 54.815 -> 11.921, pandas UNLOAD/use_threads 61.697 -> 25.889, chunked Polars CSV 23.480 -> 12.648. These establish local CPU improvements, not AWS end-to-end latency or first-import performance. Short-string predicate overhead is not claimed to disappear; empty execute keywords bypass it entirely.
The separate merged keyword-only change uses native Python binding and contains no runtime signature inspection. Its isolated binding experiment measures roughly 0.01–0.07 microseconds of additional cost for the tested signatures, not zero cost and not complete API latency. Reverting it would leave this PR's former repeated introspection intact; no revert is included.
Validation: 538 focused offline tests and full local lint passed. Current Draft lint/license/offline checks passed; documentation build and independent review are pending at recording time. Prior-head AWS results are explicitly not presented as validation of this head. All three current-head AWS suites, including SQLAlchemy and Spark selection from the core path filters, must succeed after Ready. No AWS retry or request-count reduction is introduced. Limits: author claim/operational review, not independent review or live AWS evidence.
There was a problem hiding this comment.
Final performance-follow-up validation: all applicable checks passed on published/reviewed head 5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee, merge-base 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f.
Ready-triggered Test run 37877232431, Python 3.14.8: just test pyathena 3,919 passed / 12 skipped; just test sqla 589 passed / 759 skipped; just test sqla-async 589 passed / 759 skipped. All three AWS jobs completed successfully, including the PyAthena suite's selected SQLAlchemy and Spark coverage. Draft-run skipped AWS jobs are expected from workflow conditions; they are superseded as validation by this complete Ready-triggered run.
Local format/full lint and 538 focused offline tests passed; current-head documentation lint/build, license, and offline CI passed. Both author review perspectives and the separate Opus 5.5 Max independent static review are CLEAN for this follow-up. The frozen source snapshot and PR worktree remained unchanged.
Final GitHub state: Ready (isDraft=false), MERGEABLE, merge state CLEAN, exact reviewed head. Source worktrees are clean. Performance claims remain limited to the explicitly scoped local benchmarks; no AWS end-to-end performance improvement is inferred from CI duration. PR description updated with the final validation.
| start = _OPERATION_START.search(operation) | ||
| if start is None: | ||
| return False | ||
| return operation[start.start() : start.start() + 6].upper().startswith(prefixes) |
There was a problem hiding this comment.
Relayed independent static review — performance follow-up: CLEAN.
Reviewer: Claude Opus 5.5 (claude-opus-5-5), effort high, verified first-party max profile (not Enterprise). Session: 530606ba-826d-4f2e-a6ca-432ca0e81e9d. The CLI completed successfully; actual model and provider metadata match the request.
Review merge-base: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f; published head: 5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee. Previous reviewed head: 5b48a622667322689dee3fc973a6d1c26e3e8353, same merge-base; both old objects verified and all seven prior commits unchanged in the range-diff. The clean tracked-source snapshot, literal repair/full-series diffs, and source/test contracts were supplied without PR description/discussion, commit messages, author conclusions, credentials, or memory. Read/Grep/Glob only; no edits, execution, tests, GitHub access, or subagents. Snapshot hashes and the clean PR worktree were verified unchanged afterward.
Covered all nine appended-update files, caching/constructor MRO callers, pandas/Polars reader-name mutations, sync/async SQLAlchemy internal cursors and reflection, Unicode/whitespace classification, prepared/unprepared reader routing, effective chunk settings, empty-keyword/preparation-failure paths, state/AWS ordering, and regression assertions. No actionable defects found.
The reviewer noted non-blocking cache-clear wording, the fixed six-character prefix width, the lack of an inspection-count assertion in the unhashable-callable test, and bounded retention of constructor functions/classes. These are retained in this patch: current prefixes are all at most six characters and explicitly tested; the fallback regression verifies unhashable callables remain accepted; retention is capped at 64 callables. The private cache-clear instruction is intentional for same-object signature mutation. None changes the CLEAN verdict.
Limits: static source review, not runtime or AWS proof. The reviewer did not execute against installed reader signatures or free-threaded Python; local installed-library coverage and CPU measurements are separate author validation. Current-head Ready-triggered AWS CI remains to be collected.
Actual reviewer result follows:
Result: CLEAN
I found no actionable defects in the appended update (patch 8, 5b48a622… → 5c96c925…). The range-diff shows patches 1–7 are unchanged, and the snapshot files match repair.patch and review.patch.
What I covered
Signature caching (pyathena/_kwargs.py:16-46)
- Cache keys and replacement: functions and the
object.__init__slot wrapper are cached by identity. The cache holds a strong reference to each cached callable, so its id can't be reused while the entry exists. Assigning a new__init__creates a new key, as the docstring says. - Mutation isolation: every call returns
set(frozenset), a fresh set. So the in-place edits in the validators (allowed -= …,|=and.add(old)atpandas/result_set.py:56-67,polars/result_set.py:71-84) andnames.update(...)at_kwargs.py:61never touch the cache. - Unhashable callables: if
hash()raisesTypeError, the code calls the undecorated function directly, so nothing is cached and the error isn't hidden. - Exceptions:
lru_cachedoes not cache them, sosignature()errors reach the caller as before. - Concurrency:
lru_cacheis thread-safe. At worst two threads compute the same value twice. - Optional dependencies: the module still uses only the standard library.
import_module("pandas")/("polars")still only runs when keywords are present. Python ≥3.11 (pyproject.toml:17) supportsfrozenset[str]and__wrapped__. - Callers:
Connection._internal_cursor(connection.py:645-652), the async adapter (aio/sqlalchemy/base.py:279-282) and SQLAlchemy reflection (sqlalchemy/base.py:539, 566, 674) only subtract or read fresh sets, so their behavior is unchanged.
UNLOAD classification (common.py:1198-1215)
- Whitespace: Python's regex
\sandstr.strip()use the same whitespace definition (str.isspace()), so the first non-whitespace position is identical. That holds for U+2003 and U+001C, and U+FEFF is excluded in both. - Unicode case:
str.upper()maps each character independently (onlylower/casefoldlook at context) and never shortens text. So uppercasing the first 6 characters gives a prefix of the full uppercased string with length ≥ 6. With the longest prefix being 6 characters (SELECT/UNLOAD), the result is exactly the same as the oldstrip().upper().startswith(...), includingſ→S,ı→Iandİunchanged. Trailing whitespace stripping can't matter because no prefix contains whitespace. - Prepared vs. unprepared: results agree with
Formatter.wrap_unload(formatter.py:175-179) and with its"\nUNLOAD ("output. - Performance: cost is now proportional to leading whitespace plus 6 characters, instead of the whole string.
Skipping validation when no keywords are passed (common.py:1217-1240, and the six execute methods: pandas/cursor.py:228-242, pandas/async_cursor.py:257-269, aio/pandas/cursor.py:197-211, polars/cursor.py:219-235, polars/async_cursor.py:260-274, aio/polars/cursor.py:177-193)
- Both validators already returned immediately for empty
kwargs(pandas/result_set.py:51,polars/result_set.py:64), and_is_unload_querycan't raise. So skipping only removes the SQL scan; it doesn't change any outcome. - Polars still passes the effective chunk size,
kwargs.get("chunksize", self._chunksize), on both paths. - The order of validation,
_reset_state()and_execute(the AWS call) is unchanged. Synchronous and asyncio cursors still reset state on a preparation failure; Future-based ones don't. - When an unknown keyword hits the failure path, the
TypeErroris still raised with the original error attached as its context, and state isn't reset. This behavior comes from patch 7, not this update.
Tests (tests/pyathena/test_kwargs.py:68-134, 188-213)
- The cache test patches the module-level
signature, which the cached helper looks up at call time, and uses a fresh local function, so the "called once" assertion is valid. - The classification table's expected values match the reasoning above.
- The skip test would fail on the old code, which called
_is_unload_queryunconditionally.patch.objecton the asyncio_executeuses anAsyncMock.
Non-blocking notes (not defects)
_kwargs.py:37: "require clearing…cache_clear()" should read "require calling". The docstring also exposes a private helper.common.py:1215: the hard-coded6relies on the longest prefix being 6 characters. Adding a longer prefix later would silently produce wrong results.test_kwargs.py:81-90: the unhashable test doesn't check that the cache was bypassed (e.g. thatsignatureran twice).- The cache keeps up to 64
__init__functions alive. Ones that usesuper()also keep their class alive. This is bounded and harmless.
Limitation
This is a static review only. I used Read, Grep and Glob within this snapshot, and ran no code or tests. The Unicode, re/str.isspace() and lru_cache conclusions come from my knowledge of CPython, not from running anything. I didn't check behavior against the installed pandas, Polars or pyarrow signatures, or under free-threaded Python.
| *reader_args, | ||
| ) | ||
|
|
||
| def _prepare_reader_query( |
There was a problem hiding this comment.
Reader-preparation follow-up — self-review round one: CLEAN.
Literal current review range: f3532e9fb57fd769441e533f0b8375fd854ece69..3daaedf06100384cde8eea65d438069539c6ecc7.
Previous completed review range: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f..5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee.
Both old objects were verified. The bounded patch-series comparison shows all eight previously reviewed commits unchanged and one added reader-preparation commit (769b115c3d9b00bdd9764f8d8746fcde54458711). Current master was merged without conflicts; affected upstream contracts were inspected rather than treating the previous CI as current evidence.
Coverage: all 12 follow-up files (BaseCursor helper, six pandas/Polars sync/Future/aio execute methods, both reader validators, kwargs and both result-set regression files), necessary unchanged formatting/execution/result-set callers, and upstream keyword-only signatures and ExecuteOptions resolution. The earlier full-series coverage remains bounded by its recorded scopes; this is a repair and integration follow-up, not a claim of executing every backend locally.
The shared helper resolves the same fields, prepares once, and validates once on each normal/preparation-failure path. Successful preparation selects the actual returned operation's reader; failed preparation selects the original operation's reader. Unknown names still take precedence over preparation errors and preserve prior state. Valid preparation failures reset synchronous/aio state; Future variants retain their existing state. KeyboardInterrupt/SystemExit bypass Exception handling and preserve state. Callback and type-hint options, None/false/zero behavior, and public execute signatures are preserved.
Immutable backend cursor-setting sets allow validators to return before optional module lookup/signature inspection only when every supplied name is already a PyAthena-owned setting. Mixed reader options or unknown names still take the installed-reader validation path. No module or accepted-user-value cache was introduced. Replaced readers remain detectable by their callable identity; in-place signature mutation retains the existing explicit cache-clear requirement.
Validation on this published tree: just format, just lint, just docs lint, and just docs build passed. Focused offline pytest: 682 passed, three existing dtypes deprecation warnings. New regressions cover custom prepared-mode changes, interrupts, import omission, and reader replacement; existing six-variant failure/reset, supported-option, callback, and constructor cases remain covered. No actionable findings or repairs from this round. CPU measurements, independent static review, and current-head AWS CI remain separate pending evidence.
There was a problem hiding this comment.
Round-one repair follow-up: CLEAN at bb993fa8c3985bee117006f07c6912517d9bdea6, merge-base f3532e9fb57fd769441e533f0b8375fd854ece69.
Inspected git diff 3daaedf06100384cde8eea65d438069539c6ecc7..bb993fa8c3985bee117006f07c6912517d9bdea6 plus its six execute callers and regression tests. The single changed file folds the unreleased _validate_reader_kwargs helper into shared preparation, removing a nested call without changing the two distinct reader-mode decisions, error precedence, kwargs forwarding, empty-keyword guard, or reset/interrupt behavior. The previous full follow-up coverage and inspected upstream contracts remain applicable.
just format, just lint, and the same focused offline suite passed on this final source: 682 passed, three existing dtypes warnings. Local docs lint/build passed on the integrated predecessor; documentation sources are unchanged by this one-file repair, and final-head docs CI is collected separately. No actionable findings. The 103-case final measurements keep the same upstream on both sides and explicitly report remaining default/reader-option cost, as well as cursor-setting-only improvements. Independent review and final-head AWS CI are still pending.
There was a problem hiding this comment.
Round-one native-import follow-up: CLEAN.
Current merge-base/head: f3532e9fb57fd769441e533f0b8375fd854ece69..c42d65d04a54d3fe1b5cb90122635b495a64d09d.
Inspected git diff bb993fa8c3985bee117006f07c6912517d9bdea6..c42d65d04a54d3fe1b5cb90122635b495a64d09d: both reader validators and both result-set test files, plus the six shared-preparation callers and unchanged optional-reader/option/formatter contracts. The earlier complete follow-up and upstream coverage is retained; the three new preparation/import commits are separately auditable and all eight original-series commits remain unchanged.
The validators use native optional imports instead of import_module. Imports remain inside the dependency-requiring path after the empty/cursor-setting-only guard; no eager module import or separate module-pointer cache was added. Each validation still reads the live callable attribute and selects the installed reader signature. Polars' explicitly annotated Callable variable allows the CSV/Parquet callable union to be inspected without changing runtime behavior. The dynamic pandas CSV-engine name import elsewhere is unchanged because its name is selected at runtime.
The import-omission tests now observe builtins.import and reader signature inspection, so they cover the native-import path rather than mocking a removed import_module alias. CSV reader replacement tests for pandas and Polars remain, and added PyArrow read_table replacement coverage accepts the new name and rejects the removed name. Existing failure precedence, state/reset/interrupt cases, supported options and callbacks remain covered.
Final-source just format and just lint passed. The same focused offline suite plus the new Parquet replacement case passed: 683 passed, three existing Polars dtypes deprecation warnings. No actionable findings or additional repairs. Independent review is not counted as completed: the previous Max attempt hit its weekly limit; the final snapshot is prepared for a retry after the reported reset. Final-head AWS validation is still pending.
| return False | ||
| return operation[start.start() : start.start() + 6].upper().startswith(prefixes) | ||
|
|
||
| def _prepare_reader_query( |
There was a problem hiding this comment.
Reader-preparation follow-up — self-review round two: CLEAN.
Literal base/head: f3532e9fb57fd769441e533f0b8375fd854ece69..bb993fa8c3985bee117006f07c6912517d9bdea6.
Previous reviewed base/head: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f..5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee. Patch-series comparison preserves all eight previously reviewed commits and adds the two bounded reader-preparation changes; current-master integration was checked for affected contracts.
Claim inventory and falsification checks: early rejection versus cursor state/AWS start, supported pandas/Polars reader names and modes, PyAthena-owned names, imported-reader replacement and lazy optional dependencies, callback/hint/value preservation, sync/Future/aio error ordering and interrupts, keyword-only public signatures after upstream integration, and benchmark attribution/performance limits. Traced all 12 repair files and their upstream/unchanged option, formatting, execution, result-reader, and constructor/reflection callers. Checked the current PR's complete changed-file inventory for expanded contracts; the unchanged initial series remains covered by its prior records rather than relabeled as a new full implementation pass.
The synthesized comparison tree merges the previous published validation head with the exact same current master used by the candidate, including #1110. It is identified as a tree, not presented as a published revision. Checked the 103-case harness, nine fresh-process samples per cohort, alternating order, actual import identities, retained BaseCursor._execute and formatting, fake provider/reader/scheduling boundaries, 2000 versus 100 iteration counts, warm-up/GC behavior, and all median/min/max calculations. No AWS performance extrapolation or statistical significance claim is made. Additional shared-preparation cost on default/reader-option paths is disclosed; measurements support cursor-setting-only savings, not a general speedup.
The new immutable cursor-name sets bypass module/signature discovery only for names already handled by PyAthena. Reader replacement still selects a new callable-signature cache entry; no separate module cache or cached mutable options were added. Optional-dependency availability and reader values remain lazy library concerns, not a promised preflight. Ordinary import_module calls use Python's existing module cache and initialization synchronization.
Supported kwargs preserve the result-reader routing, including managed chunked Polars eager fallback and deprecated dtype aliases. Invalid names still fail before AWS/state changes; valid preparation failures keep the existing sync/aio reset versus Future preservation distinction. KeyboardInterrupt/SystemExit remain outside Exception handling. Upstream ExecuteOptions direct construction and keyword-only bindings remain intact. No requests, retry settings, polling or cancellation policy changed.
Final-source format/lint and 682 offline tests passed (three existing dtypes warnings). Docs sources were unchanged by the final one-file repair; local docs checks belong to the integrated predecessor and current-head docs CI is recorded separately. Earlier-head AWS CI is explicitly historical; final-head AWS coverage is not yet complete. No actionable findings, no additional repairs, and no claimed end-to-end runtime proof from this static/CPU audit.
There was a problem hiding this comment.
Independent review status: INCOMPLETE at head bb993fa8c3985bee117006f07c6912517d9bdea6, merge-base f3532e9fb57fd769441e533f0b8375fd854ece69.
The first-party Max account/profile was verified (loggedIn=true, authMethod=claude.ai, apiProvider=firstParty, subscriptionType=max). Requested model/effort: claude-opus-5-5, high. The bounded tracked-source snapshot excluded .git, .env, credentials, memory, PR discussion and prior review conclusions; Read/Grep/Glob only, safe/restricted modes, no code execution or writes.
CLI session 32048682-a26a-448f-b8b6-d2982de6789d exited 1 with is_error=true, empty modelUsage, and the actual result: "You've hit your weekly limit · resets 12am (Asia/Tokyo)". This attempt did not produce a review verdict and is not counted as CLEAN. The frozen snapshot and worktree were verified unchanged. The PR stays Draft while waiting for the requested Max review to become available; no replacement model/account was used.
Both self-review rounds, the final 682-test offline run, and the recorded 103-case CPU measurements remain completed. Current-head Docs lint/build, Test lint, license headers and benchmark tooling offline passed; Draft AWS skips are intentional. Current-head AWS validation follows the completed independent review and Ready transition.
|
|
||
| Raises: | ||
| TypeError: If an argument is not supported by the selected reader. | ||
| """ |
There was a problem hiding this comment.
Reader-preparation and optional-module follow-up measurements.
Published head: bb993fa8c3985bee117006f07c6912517d9bdea6.
Review merge-base/current master: f3532e9fb57fd769441e533f0b8375fd854ece69.
Comparison source: tree b1d87c391dce1484633d1872a900c9cc6a900bec, produced by a conflict-free git merge-tree --write-tree 5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee f3532e9fb57fd769441e533f0b8375fd854ece69. This is a synthesized pre-follow-up tree, not a separately published commit. Both cohorts contain the same current master, including merged #1110, so its options optimization is not counted as this follow-up's improvement.
CPython 3.13.1, arm64; pandas 3.0.6, Polars 1.44.2, PyArrow 25.0.1, SQLAlchemy 2.0.46. 103 cases, 9 fresh-subprocess repeats, alternating cohort order, 2,000 iterations per main case. The three default_interrupt_mode cases use 100 iterations. Every sample and its case inventory were checked; the reported ranges are observed minima/maxima, not confidence intervals. timeit disables GC during timing.
Retained: actual connection/cursor/execute bodies, BaseCursor._execute, option resolution, UNLOAD formatting, request preparation, and reader keyword validation. Replaced: SDK session/client construction, AWS submission/polling, result readers, Future worker scheduling, and asyncio reader thread offloads. Future measurements collect the fake executor's immediately produced result and do not measure real executor latency. asyncio batches retain event-loop run_until_complete overhead amortized across the batch. Main cases use kill_on_interrupt=False. Three separately named default_interrupt_mode cases retain start-request thread/task scheduling with True. Setup, imports, and cold signature-cache misses are excluded by preparation/warm-up.
import_module itself is called on every ordinary reader-validation invocation. CPython's _find_and_load checks sys.modules and waits for imports still initializing; the already initialized module is returned without rerunning its code. Module identity was checked against sys.modules for each cached-import case. Warm cached calls measured approximately 0.29–0.30 microseconds. No module cache was added by this patch.
Cursor-setting-only validation now returns before module lookup and signature inspection: pandas CSV median 1.020 -> 0.145 microseconds; Polars CSV 2.368 -> 0.153. Actual CSV execute with cursor settings: pandas 9.271 -> 8.323; Polars 10.406 -> 8.047. These are local CPU-path results.
Shared preparation adds work on other paths: PandasCursor defaults 7.278 -> 7.577; PolarsCursor defaults 7.219 -> 7.258; Pandas CSV reader option 8.814 -> 9.085; Polars CSV reader option 9.168 -> 9.311. Their observed ranges overlap; this is not a claim of equal cost or universal improvement. Default interrupt-mode pandas medians 51.280 -> 52.463, ranges 49.206–59.380 / 47.802–56.425. No speedup is demonstrated for that mode, and no AWS end-to-end latency claim is made.
Only cursor-setting names bypass reader discovery. Mixed reader arguments or typos still inspect the selected installed reader's cached signature. Dependency availability and reader values/combinations remain lazy library concerns; this is keyword-name validation, not an optional-dependency preflight.
All case results (microseconds/call, median and observed range):
| Case | Before | This head |
|---|---|---|
| connect_local_session | 16.072 [15.759–16.311] | 16.121 [15.687–17.053] |
| Connection_local_session | 15.701 [15.471–16.383] | 15.861 [15.321–16.351] |
| AioConnection_local_session | 15.995 [15.638–16.649] | 16.087 [15.603–17.065] |
| Cursor.construct_close | 12.865 [12.602–13.056] | 12.792 [12.679–13.281] |
| Cursor.execute_defaults | 6.533 [6.467–6.965] | 6.564 [6.470–6.847] |
| Cursor.execute_keywords | 6.758 [6.640–7.135] | 6.724 [6.582–7.117] |
| Cursor.execute_prebuilt | 8.242 [8.087–8.595] | 8.144 [7.978–8.552] |
| AsyncCursor.construct_close | 13.231 [13.016–13.582] | 13.361 [13.159–14.026] |
| AsyncCursor.execute_defaults | 8.291 [8.004–8.699] | 8.145 [8.074–8.633] |
| AsyncCursor.execute_keywords | 8.319 [8.241–8.840] | 8.357 [8.235–8.596] |
| AsyncCursor.execute_prebuilt | 9.778 [9.625–10.460] | 9.862 [9.674–10.406] |
| AioCursor.construct_close | 12.820 [12.614–13.355] | 12.930 [12.577–13.255] |
| AioCursor.execute_defaults | 7.109 [6.939–7.285] | 7.138 [6.844–7.342] |
| AioCursor.execute_keywords | 7.236 [7.017–7.486] | 7.313 [7.015–7.612] |
| AioCursor.execute_prebuilt | 8.765 [8.384–9.183] | 8.836 [8.443–8.992] |
| PandasCursor.construct_close | 7.893 [7.812–8.129] | 7.915 [7.798–8.328] |
| PandasCursor.execute_defaults | 7.278 [7.190–7.591] | 7.577 [7.340–7.912] |
| PandasCursor.execute_keywords | 7.470 [7.350–7.668] | 7.766 [7.559–7.993] |
| PandasCursor.execute_prebuilt | 8.953 [8.782–9.386] | 9.250 [8.989–9.537] |
| AsyncPandasCursor.construct_close | 9.449 [9.341–9.742] | 9.661 [9.449–10.000] |
| AsyncPandasCursor.execute_defaults | 8.891 [8.649–9.343] | 9.199 [8.946–9.526] |
| AsyncPandasCursor.execute_keywords | 9.055 [8.883–9.319] | 9.194 [8.914–10.007] |
| AsyncPandasCursor.execute_prebuilt | 10.443 [10.250–10.903] | 10.789 [10.422–11.051] |
| AioPandasCursor.construct_close | 7.832 [7.714–8.141] | 7.999 [7.776–8.248] |
| AioPandasCursor.execute_defaults | 8.069 [7.936–8.382] | 8.388 [8.105–8.703] |
| AioPandasCursor.execute_keywords | 8.270 [8.127–9.525] | 8.561 [8.294–9.120] |
| AioPandasCursor.execute_prebuilt | 9.696 [9.544–11.029] | 10.013 [9.731–10.514] |
| ArrowCursor.construct_close | 9.014 [8.817–9.473] | 9.154 [9.002–9.728] |
| ArrowCursor.execute_defaults | 6.946 [6.741–7.169] | 6.982 [6.773–7.479] |
| ArrowCursor.execute_keywords | 7.138 [6.954–7.357] | 7.149 [7.072–7.541] |
| ArrowCursor.execute_prebuilt | 8.598 [8.426–8.763] | 8.607 [8.413–9.055] |
| AsyncArrowCursor.construct_close | 10.829 [10.662–11.086] | 10.903 [10.717–11.460] |
| AsyncArrowCursor.execute_defaults | 8.490 [8.281–8.703] | 8.457 [8.309–9.095] |
| AsyncArrowCursor.execute_keywords | 8.616 [8.407–8.858] | 8.658 [8.503–8.885] |
| AsyncArrowCursor.execute_prebuilt | 10.088 [9.968–10.506] | 10.145 [9.829–10.665] |
| AioArrowCursor.construct_close | 9.069 [8.966–9.516] | 9.099 [9.028–9.468] |
| AioArrowCursor.execute_defaults | 7.535 [7.374–7.952] | 7.578 [7.345–8.074] |
| AioArrowCursor.execute_keywords | 7.799 [7.555–7.934] | 7.761 [7.595–8.131] |
| AioArrowCursor.execute_prebuilt | 9.252 [9.029–9.590] | 9.248 [8.932–9.681] |
| PolarsCursor.construct_close | 8.327 [8.184–8.719] | 8.254 [8.177–8.386] |
| PolarsCursor.execute_defaults | 7.219 [6.946–7.511] | 7.258 [7.163–7.484] |
| PolarsCursor.execute_keywords | 7.462 [7.255–7.744] | 7.477 [7.230–7.765] |
| PolarsCursor.execute_prebuilt | 8.942 [8.698–9.180] | 8.939 [8.672–9.225] |
| AsyncPolarsCursor.construct_close | 9.919 [9.800–10.146] | 10.131 [9.773–10.574] |
| AsyncPolarsCursor.execute_defaults | 8.687 [8.454–9.048] | 8.884 [8.577–9.185] |
| AsyncPolarsCursor.execute_keywords | 8.734 [8.561–9.579] | 8.944 [8.815–9.508] |
| AsyncPolarsCursor.execute_prebuilt | 10.250 [10.051–10.823] | 10.447 [10.176–10.814] |
| AioPolarsCursor.construct_close | 8.291 [8.105–8.784] | 8.302 [8.119–8.761] |
| AioPolarsCursor.execute_defaults | 7.790 [7.570–8.330] | 7.982 [7.840–8.212] |
| AioPolarsCursor.execute_keywords | 7.878 [7.848–8.204] | 8.153 [8.038–8.527] |
| AioPolarsCursor.execute_prebuilt | 9.359 [9.201–10.067] | 9.543 [9.395–10.256] |
| S3FSCursor.construct_close | 12.516 [12.379–13.205] | 12.689 [12.350–12.823] |
| S3FSCursor.execute_defaults | 6.702 [6.530–7.369] | 6.675 [6.553–6.941] |
| S3FSCursor.execute_keywords | 6.872 [6.707–7.278] | 6.893 [6.783–7.196] |
| S3FSCursor.execute_prebuilt | 8.334 [8.235–9.182] | 8.392 [8.181–8.624] |
| AsyncS3FSCursor.construct_close | 14.413 [14.095–15.789] | 14.247 [14.100–14.721] |
| AsyncS3FSCursor.execute_defaults | 8.231 [8.082–8.638] | 8.272 [8.116–8.636] |
| AsyncS3FSCursor.execute_keywords | 8.479 [8.154–8.965] | 8.398 [8.217–8.703] |
| AsyncS3FSCursor.execute_prebuilt | 9.863 [9.613–10.343] | 9.868 [9.668–10.088] |
| AioS3FSCursor.construct_close | 12.629 [12.474–12.818] | 12.752 [12.393–12.920] |
| AioS3FSCursor.execute_defaults | 7.370 [7.222–7.570] | 7.481 [7.181–7.636] |
| AioS3FSCursor.execute_keywords | 7.537 [7.356–8.034] | 7.638 [7.363–7.870] |
| AioS3FSCursor.execute_prebuilt | 9.000 [8.824–9.323] | 9.155 [8.796–9.499] |
| pandas.validate_csv_cursor_settings | 1.020 [1.001–1.052] | 0.145 [0.141–0.158] |
| pandas.validate_csv_reader_setting | 0.948 [0.932–0.990] | 0.970 [0.934–1.052] |
| PandasCursor.execute_csv_cursor_settings | 9.271 [9.088–9.702] | 8.323 [8.185–8.598] |
| PandasCursor.execute_csv_reader_setting | 8.814 [8.692–9.165] | 9.085 [8.878–9.442] |
| AsyncPandasCursor.execute_csv_cursor_settings | 10.816 [10.721–11.264] | 9.835 [9.656–10.035] |
| AsyncPandasCursor.execute_csv_reader_setting | 10.366 [10.298–10.804] | 10.675 [10.464–10.971] |
| AioPandasCursor.execute_csv_cursor_settings | 10.049 [9.887–10.463] | 9.134 [9.023–9.480] |
| AioPandasCursor.execute_csv_reader_setting | 9.677 [9.443–10.199] | 9.894 [9.698–10.056] |
| pandas.validate_parquet_cursor_settings | 1.752 [1.679–1.785] | 0.147 [0.141–0.156] |
| pandas.validate_parquet_reader_setting | 1.694 [1.642–1.739] | 1.713 [1.650–1.829] |
| PandasCursor.execute_parquet_cursor_settings | 23.136 [22.832–23.969] | 21.229 [20.942–22.480] |
| PandasCursor.execute_parquet_reader_setting | 22.589 [22.368–24.055] | 23.024 [22.687–25.365] |
| AsyncPandasCursor.execute_parquet_cursor_settings | 24.880 [24.584–26.554] | 23.124 [22.610–24.905] |
| AsyncPandasCursor.execute_parquet_reader_setting | 24.597 [24.161–26.236] | 24.752 [24.318–26.152] |
| AioPandasCursor.execute_parquet_cursor_settings | 24.066 [23.699–25.253] | 22.283 [21.764–23.194] |
| AioPandasCursor.execute_parquet_reader_setting | 23.642 [23.309–25.887] | 23.804 [23.598–25.354] |
| polars.validate_csv_cursor_settings | 2.368 [2.326–2.420] | 0.153 [0.149–0.157] |
| polars.validate_csv_reader_setting | 2.303 [2.271–2.336] | 2.348 [2.321–2.427] |
| PolarsCursor.execute_csv_cursor_settings | 10.406 [10.215–10.677] | 8.047 [7.887–8.452] |
| PolarsCursor.execute_csv_reader_setting | 9.168 [9.033–9.424] | 9.311 [9.119–9.703] |
| AsyncPolarsCursor.execute_csv_cursor_settings | 12.160 [11.833–12.427] | 9.502 [9.408–10.157] |
| AsyncPolarsCursor.execute_csv_reader_setting | 10.917 [10.657–11.233] | 11.068 [10.726–11.352] |
| AioPolarsCursor.execute_csv_cursor_settings | 11.161 [10.878–11.405] | 8.712 [8.660–8.989] |
| AioPolarsCursor.execute_csv_reader_setting | 9.970 [9.679–10.375] | 10.128 [9.773–10.387] |
| polars.validate_parquet_cursor_settings | 1.427 [1.377–1.454] | 0.154 [0.151–0.161] |
| polars.validate_parquet_reader_setting | 1.356 [1.308–1.396] | 1.400 [1.351–1.462] |
| PolarsCursor.execute_parquet_cursor_settings | 22.465 [22.025–23.664] | 20.888 [20.739–21.851] |
| PolarsCursor.execute_parquet_reader_setting | 22.057 [21.596–22.806] | 22.336 [22.176–23.065] |
| AsyncPolarsCursor.execute_parquet_cursor_settings | 24.179 [23.922–25.676] | 22.671 [22.419–23.385] |
| AsyncPolarsCursor.execute_parquet_reader_setting | 23.845 [23.639–25.134] | 24.425 [23.887–25.485] |
| AioPolarsCursor.execute_parquet_cursor_settings | 23.428 [22.633–24.625] | 22.040 [21.437–22.524] |
| AioPolarsCursor.execute_parquet_reader_setting | 22.762 [22.458–23.672] | 23.304 [22.871–24.456] |
| cached_import.pandas | 0.291 [0.290–0.304] | 0.296 [0.291–0.318] |
| cached_import.polars | 0.294 [0.291–0.303] | 0.294 [0.290–0.303] |
| cached_import.pyarrow.parquet | 0.296 [0.293–0.305] | 0.301 [0.293–0.307] |
| ExecuteOptions.resolve_none_all_none | 1.319 [1.307–1.353] | 1.323 [1.310–1.405] |
| ExecuteOptions.resolve_prebuilt_all_none | 2.915 [2.852–3.021] | 2.936 [2.877–3.055] |
| Cursor.execute_default_interrupt_mode | 53.435 [50.015–63.977] | 51.501 [45.212–59.015] |
| PandasCursor.execute_default_interrupt_mode | 51.280 [49.206–59.380] | 52.463 [47.802–56.425] |
| AioCursor.execute_default_interrupt_mode | 51.689 [50.157–71.610] | 51.459 [49.755–76.194] |
Reproduction and complete measurement harness
Use the same interpreter/dependencies for both source trees. Save the child and driver below as measure-api.py and run-comparison.py under /private/tmp/pyathena-1093-reader-preparation. Export the synthesized baseline, and use a clean checkout of the published head at the driver's worktree path. Write the provided provenance JSON as benchmark-source.json under that temporary directory.
git merge-tree --write-tree 5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee f3532e9fb57fd769441e533f0b8375fd854ece69
mkdir -p /private/tmp/pyathena-1093-reader-preparation/before-on-master
git archive b1d87c391dce1484633d1872a900c9cc6a900bec | tar -xf - -C /private/tmp/pyathena-1093-reader-preparation/before-on-master
uv run python /private/tmp/pyathena-1093-reader-preparation/run-comparison.py > results.jsonProvenance JSON:
{
"before_tree": "b1d87c391dce1484633d1872a900c9cc6a900bec",
"before_inputs": [
"5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee",
"f3532e9fb57fd769441e533f0b8375fd854ece69"
],
"after_head": "bb993fa8c3985bee117006f07c6912517d9bdea6",
"upstream": "f3532e9fb57fd769441e533f0b8375fd854ece69",
"scope": "Both sources include the same current master, including the merged options optimization. Before is a conflict-free merge-tree of the previous published validation head and that master, exported without a git worktree; after is the current PR head."
}Child harness:
import asyncio
from concurrent.futures import Future
from importlib import import_module
from inspect import iscoroutinefunction
import json
from pathlib import Path
import platform
import sys
import timeit
from types import SimpleNamespace
source = Path(sys.argv[1]).resolve()
sys.path.insert(0, str(source))
import pyathena
from pyathena.aio.connection import AioConnection
from pyathena.connection import Connection
from pyathena.model import AthenaQueryExecution
from pyathena.options import ExecuteOptions
from pyathena.spark.common import SparkBaseCursor
assert Path(pyathena.__file__).resolve().parent.parent == source
iterations = int(sys.argv[2])
class NoAWSClient:
def close(self):
pass
def __getattr__(self, name):
raise AssertionError('Unexpected AWS client access: ' + name)
class LocalSession:
def client(self, *args, **kwargs):
return NoAWSClient()
class LocalResult:
def __init__(self, *args, **kwargs):
self.is_closed = False
def close(self):
self.is_closed = True
@classmethod
async def create(cls, *args, **kwargs):
return cls(*args, **kwargs)
class ImmediateExecutor:
def submit(self, fn, *args, **kwargs):
result = Future()
result.set_result(fn(*args, **kwargs))
return result
def shutdown(self, *args, **kwargs):
pass
async def immediate_thread(fn, *args, **kwargs):
return fn(*args, **kwargs)
asyncio.to_thread = immediate_thread
SparkBaseCursor._start_session = lambda *args, **kwargs: 'session'
query = SimpleNamespace(state=AthenaQueryExecution.STATE_SUCCEEDED)
async def start_async(*args, **kwargs):
return 'query'
async def poll_async(*args, **kwargs):
return query
session = LocalSession()
connection_args = dict(region_name='us-east-1', s3_staging_dir='s3://bucket/path/', work_group='wg', session=session, kill_on_interrupt=False)
connection = Connection(**connection_args)
loop = asyncio.new_event_loop()
results = {}
def measure(name, call, count=None):
count = iterations if count is None else count
for _ in range(30):
call()
results[name] = timeit.timeit(call, number=count) / count * 1e6
def create_connection(factory):
conn = factory(**connection_args)
conn.close()
measure('connect_local_session', lambda: create_connection(pyathena.connect))
measure('Connection_local_session', lambda: create_connection(Connection))
measure('AioConnection_local_session', lambda: create_connection(AioConnection))
cursor_definitions = [
('pyathena.cursor', 'Cursor'),
('pyathena.async_cursor', 'AsyncCursor'),
('pyathena.aio.cursor', 'AioCursor'),
]
for backend in ('pandas', 'arrow', 'polars', 's3fs'):
name = {'pandas': 'Pandas', 'arrow': 'Arrow', 'polars': 'Polars', 's3fs': 'S3FS'}[backend]
cursor_definitions.extend([
('pyathena.' + backend + '.cursor', name + 'Cursor'),
('pyathena.' + backend + '.async_cursor', 'Async' + name + 'Cursor'),
('pyathena.aio.' + backend + '.cursor', 'Aio' + name + 'Cursor'),
])
for module_name, class_name in cursor_definitions:
module = import_module(module_name)
cursor_class = getattr(module, class_name)
for name, value in vars(module).copy().items():
if name.startswith('Athena') and name.endswith('ResultSet'):
setattr(module, name, LocalResult)
if 'Pandas' in class_name:
import_module('pandas')
if 'Polars' in class_name:
import_module('polars')
if 'Arrow' in class_name:
import_module('pyarrow')
def close(cursor):
if iscoroutinefunction(cursor.close):
loop.run_until_complete(cursor.close())
else:
cursor.close()
def create_cursor():
cursor = connection.cursor(cursor_class)
close(cursor)
measure(class_name + '.construct_close', create_cursor)
cursor = connection.cursor(cursor_class)
if hasattr(cursor, '_result_set_class'):
cursor._result_set_class = LocalResult
if class_name.startswith('Async'):
cursor._executor.shutdown(wait=True)
cursor._executor = ImmediateExecutor()
if iscoroutinefunction(cursor.execute):
cursor._start_query_execution = start_async
cursor._poll = poll_async
async def execute_batch(settings):
for _ in range(iterations):
await cursor.execute('SELECT 1', **settings)
for label, settings in [('defaults', {}), ('keywords', {'work_group': 'wg', 'cache_size': 0, 'paramstyle': 'pyformat'}), ('prebuilt', {'options': ExecuteOptions(work_group='wg')})]:
loop.run_until_complete(execute_batch(settings))
duration = timeit.timeit(lambda: loop.run_until_complete(execute_batch(settings)), number=1)
results[class_name + '.execute_' + label] = duration / iterations * 1e6
else:
cursor._start_query_execution = lambda *args, **kwargs: 'query'
cursor._poll = lambda *args, **kwargs: query
for label, settings in [('defaults', {}), ('keywords', {'work_group': 'wg', 'cache_size': 0, 'paramstyle': 'pyformat'}), ('prebuilt', {'options': ExecuteOptions(work_group='wg')})]:
def execute():
value = cursor.execute('SELECT 1', **settings)
if isinstance(value, tuple):
value[1].result()
measure(class_name + '.execute_' + label, execute)
close(cursor)
# Focused reader preparation paths with real option resolution and request formatting.
for backend in ('pandas', 'polars'):
name = 'Pandas' if backend == 'pandas' else 'Polars'
reader_options = {'sep': '|'} if backend == 'pandas' else {'separator': '|'}
parquet_options = {'use_threads': False} if backend == 'pandas' else {'parallel': 'none'}
cursor_options = {'chunksize': 10, 'block_size': 64, 'cache_type': 'bytes', 'max_workers': 2}
validator = import_module('pyathena.' + backend + '.result_set').validate_execute_kwargs
for mode in (False, True):
mode_label = 'parquet' if mode else 'csv'
for label, settings in [('cursor_settings', cursor_options), ('reader_setting', parquet_options if mode else reader_options)]:
if backend == 'pandas':
call = lambda: validator('Cursor.execute', settings, mode)
else:
call = lambda: validator('Cursor.execute', settings, mode, 10)
measure(backend + '.validate_' + mode_label + '_' + label, call)
for module_name, class_name in [
('pyathena.' + backend + '.cursor', name + 'Cursor'),
('pyathena.' + backend + '.async_cursor', 'Async' + name + 'Cursor'),
('pyathena.aio.' + backend + '.cursor', 'Aio' + name + 'Cursor'),
]:
cursor = connection.cursor(getattr(import_module(module_name), class_name), unload=mode)
if hasattr(cursor, '_result_set_class'):
cursor._result_set_class = LocalResult
if class_name.startswith('Async'):
cursor._executor.shutdown(wait=True)
cursor._executor = ImmediateExecutor()
if iscoroutinefunction(cursor.execute):
cursor._start_query_execution = start_async
cursor._poll = poll_async
else:
cursor._start_query_execution = lambda *args, **kwargs: 'query'
cursor._poll = lambda *args, **kwargs: query
for label, settings in [('cursor_settings', cursor_options), ('reader_setting', parquet_options if mode else reader_options)]:
if iscoroutinefunction(cursor.execute):
async def reader_batch():
for _ in range(iterations):
await cursor.execute('SELECT 1', **settings)
loop.run_until_complete(reader_batch())
duration = timeit.timeit(lambda: loop.run_until_complete(reader_batch()), number=1)
results[class_name + '.execute_' + mode_label + '_' + label] = duration / iterations * 1e6
else:
def reader_execute():
value = cursor.execute('SELECT 1', **settings)
if isinstance(value, tuple):
value[1].result()
measure(class_name + '.execute_' + mode_label + '_' + label, reader_execute)
close(cursor)
for module_name in ('pandas', 'polars', 'pyarrow.parquet'):
loaded = import_module(module_name)
assert import_module(module_name) is loaded and sys.modules[module_name] is loaded
measure('cached_import.' + module_name, lambda: import_module(module_name))
overrides = dict(work_group=None, s3_staging_dir=None, cache_size=None, cache_expiration_time=None, result_reuse_enable=None, result_reuse_minutes=None, paramstyle=None, on_start_query_execution=None, result_set_type_hints=None)
measure('ExecuteOptions.resolve_none_all_none', lambda: ExecuteOptions.resolve(None, **overrides))
base = ExecuteOptions(work_group='wg')
measure('ExecuteOptions.resolve_prebuilt_all_none', lambda: ExecuteOptions.resolve(base, **overrides))
default_connection = Connection(**dict(connection_args, kill_on_interrupt=True))
for module_name, class_name in [('pyathena.cursor','Cursor'),('pyathena.pandas.cursor','PandasCursor'),('pyathena.aio.cursor','AioCursor')]:
cursor = default_connection.cursor(getattr(import_module(module_name),class_name))
if hasattr(cursor, '_result_set_class'):
cursor._result_set_class = LocalResult
if iscoroutinefunction(cursor.execute):
cursor._start_query_execution = start_async
cursor._poll = poll_async
async def default_batch():
for _ in range(100):
await cursor.execute('SELECT 1')
loop.run_until_complete(default_batch())
results[class_name+'.execute_default_interrupt_mode'] = timeit.timeit(lambda: loop.run_until_complete(default_batch()),number=1)/100*1e6
else:
cursor._start_query_execution = lambda *args, **kwargs: 'query'
cursor._poll = lambda *args, **kwargs: query
measure(class_name+'.execute_default_interrupt_mode', lambda: cursor.execute('SELECT 1'),count=100)
close(cursor)
default_connection.close()
connection.close()
loop.close()
print(json.dumps({'python': platform.python_version(), 'machine': platform.machine(), 'source': str(source), 'iterations': iterations, 'libraries': {name: import_module(name).__version__ for name in ('pandas', 'polars', 'pyarrow', 'sqlalchemy')}, 'results_us': results}))Driver:
import json
from pathlib import Path
from statistics import median
import subprocess
import sys
root = Path('/private/tmp/pyathena-1093-reader-preparation')
worktree = Path('/private/tmp/worktrees/PyAthena/fix-1093-reject-unknown-cursor-keywords')
cohorts = [('before', root / 'before-on-master'), ('after', worktree)]
samples = {name: [] for name, _ in cohorts}
for repeat in range(9):
order = cohorts[repeat % len(cohorts):] + cohorts[:repeat % len(cohorts)]
for name, source in order:
command = [sys.executable, str(root / 'measure-api.py'), str(source), '2000']
result = json.loads(subprocess.check_output(command, cwd=worktree, text=True))
samples[name].append(result)
summary = {'source_provenance': json.loads((root / 'benchmark-source.json').read_text()), 'repeats': 9, 'iterations_per_repeat': 2000, 'default_interrupt_mode_iterations': 100, 'order': 'Rotate source cohort order each repeat; sequential fresh subprocesses with identical interpreter and dependencies.', 'scope': 'Actual connection/cursor/execute bodies, retaining BaseCursor._execute and request formatting. Main cases use kill_on_interrupt=False to isolate CPU work. Three separately named default_interrupt_mode cases use True and retain start-request thread/task scheduling. AWS session/client construction, submission/polling, result readers, Future scheduling, and asyncio reader thread offloads use local fakes. Not AWS end-to-end latency or full asynchronous scheduling performance.', 'samples': samples, 'medians_us': {}}
for name, values in samples.items():
summary['medians_us'][name] = {case: {'median': median([v['results_us'][case] for v in values]), 'min': min(v['results_us'][case] for v in values), 'max': max(v['results_us'][case] for v in values)} for case in values[0]['results_us']}
print(json.dumps(summary, indent=2))
| """ | ||
| if not kwargs or kwargs.keys() <= _CURSOR_KWARGS: | ||
| return | ||
| import pandas |
There was a problem hiding this comment.
Round-two native-import follow-up: CLEAN.
Literal current base/head: f3532e9fb57fd769441e533f0b8375fd854ece69..c42d65d04a54d3fe1b5cb90122635b495a64d09d.
Audited the final four-file import/test change against the previously covered 12-file reader-preparation scope, affected current-master contracts, and the final description/measurement claims. Checked import laziness, optional dependency/reader-value boundaries, live reader replacement, signature-cache ownership, unchanged public keyword-only bindings, options/callback/state/failure behavior, and attribution of the two source cohorts.
Both native import and import_module use Python's existing module cache/initialization synchronization; neither statement guarantees zero lookup cost or independent dependency preflight. Native imports stay inside the optional dependency path. Cursor-setting-only names need no reader discovery, while reader names and unknown names still use live installed callables. No user values or separate module objects are cached by this patch. The None handling documented by ExecuteOptions still applies to all forwarded fields, including the Future helper's unused None callback override.
Audited the final 106-case child harness and driver: same pinned upstream on both sides, baseline merge-tree identified as a synthesized tree rather than a published commit, nine fresh-process samples per cohort in alternating order, actual _execute/option/request formatting retained, SDK/provider/result/Future/reader-offload boundaries faked, 2000 main versus 100 interrupt-mode iterations, warm-up/import exclusion, and timeit's GC behavior. Recomputed every one of the 212 median/min/max aggregates from the stored samples. Three added native-import probes check module identity against the import_module result; they measure warm module lookups, not initialization.
Warm reader-validation medians improved: pandas CSV 0.951 -> 0.712 microseconds, pandas Parquet 1.696 -> 1.290, chunked Polars CSV 2.296 -> 2.098. Native pandas lookup measured 0.070 versus import_module 0.296 microseconds in the candidate environment. Cursor-setting-only actual CSV execute medians were pandas 9.146 -> 8.319 and Polars 10.522 -> 8.013. Default execute medians still rose slightly (pandas 7.339 -> 7.434; Polars 7.212 -> 7.286) with overlapping observed ranges. No equal-cost, universal speedup, statistical-significance or AWS latency claim is made; default-interrupt-mode results remain separately reported without a speedup claim.
Final-source format/lint and 683 offline tests passed, with three existing dtypes warnings. Earlier docs checks and AWS evidence are not relabeled as current-head evidence; final-head CI is collected separately. No actionable findings, no further repairs. Static review and local fake-provider timings do not establish real AWS end-to-end performance. The required independent Max review remains incomplete until an actual successful verdict is collected.
There was a problem hiding this comment.
Final-source validation complete. PR #1102 is Ready, MERGEABLE, merge state CLEAN; it remains open and has not been merged by this work.
Exact published/reviewed head: c42d65d04a54d3fe1b5cb90122635b495a64d09d; review merge-base: f3532e9fb57fd769441e533f0b8375fd854ece69.
Both self-review perspectives and the completed Opus 5.5 first-party Max/high independent review are CLEAN. Its non-blocking observations, author verification and static limitations are preserved in the inline record. The initial weekly-limit response was not counted; actual successful retry session 1d2c8d53-2c96-4708-8011-07fe5f424a83 supplied the verdict.
Ready-triggered AWS Test run 37950215264 completed SUCCESS on Python 3.14.8 for this head:
- PyAthena: 3971 passed, 12 skipped, 16 warnings, 554.55 seconds. Its skip-spark=false and skip-sqla=false inputs were confirmed in the logs.
- SQLAlchemy synchronous: 589 passed, 759 skipped, 16 warnings, 511.93 seconds.
- SQLAlchemy asynchronous: 589 passed, 759 skipped, 16 warnings, 480.45 seconds.
- The run's lint and changes-selection jobs also passed. All three AWS suites ran; the listed skipped counts are pytest output, separate from the historical Draft-only skipped jobs in the check rollup.
Current-head Docs lint/build, license checks and benchmark tooling offline checks also passed. Final-source local just format, just lint, and the focused 683-test offline suite passed. No validation failure or additional source repair occurred after independent review. Snapshot hash manifest, clean candidate worktree and published-head equality were rechecked after CI.
Final 106-case measurements and reproduction remain local elapsed timings with fake provider/scheduling boundaries, nine fresh-process repeats and the same merged #1110 on both sides. Native import and import_module both reuse initialized modules; native imports reduce warm lookup cost, and cursor-setting-only validation bypasses lookup/inspection. Small default-path increases, sampling/provenance limitations and unexplained portions of Parquet execute differences are disclosed. These timings and CI pass results do not establish an AWS end-to-end performance improvement.
| """ | ||
| if not kwargs or kwargs.keys() <= _CURSOR_KWARGS: | ||
| return | ||
| import pandas |
There was a problem hiding this comment.
Final native-import and shared-preparation measurements.
Head: c42d65d04a54d3fe1b5cb90122635b495a64d09d; review merge-base/current master: f3532e9fb57fd769441e533f0b8375fd854ece69.
Baseline tree: b1d87c391dce1484633d1872a900c9cc6a900bec, constructed by conflict-free merge-tree from old validation head 5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee and that same master. This is a synthesized comparison tree, not a published baseline commit. Both cohorts contain merged #1110.
CPython 3.13.1 arm64; pandas 3.0.6, Polars 1.44.2, PyArrow 25.0.1, SQLAlchemy 2.0.46. 106 cases, 9 fresh-process samples per cohort, alternating order, 2000 main iterations; 3 separately named default_interrupt_mode cases use 100. All 212 median/min/max aggregates were recomputed from raw samples. Ranges are observed minima/maxima, not confidence intervals.
Actual connection/cursor/execute bodies, BaseCursor._execute, option resolution and request/UNLOAD formatting are retained. SDK session/client construction, AWS submission/polling, result readers, Future worker scheduling and asyncio reader thread offloads are local fakes. Future timing collects an immediately completed fake result and does not measure real executor latency. asyncio loop entry overhead is amortized across the batch. Main cases use kill_on_interrupt=False. Default_interrupt_mode probes retain start-request thread/task scheduling with True. Setup, imports and cold signature misses are excluded by warm-up; timeit disables GC during timing.
Reader validators now use ordinary optional import statements. Both import paths return the initialized cached module without rerunning module code; imported-module identities were checked. Native pandas/Polars lookup measured about 0.07 microseconds versus import_module about 0.30; native PyArrow Parquet about 0.16 versus import_module about 0.30. There is no added module-pointer cache, and live reader replacement remains visible. The dynamic engine-name import in pandas result construction is unchanged.
Warm reader validation improved (pandas CSV 0.951 -> 0.712; pandas Parquet 1.696 -> 1.290; chunked Polars CSV 2.296 -> 2.098 microseconds). Cursor-setting-only validation returns before import/inspection; actual CSV execute medians with those settings improved (pandas 9.146 -> 8.319, Polars 10.522 -> 8.013). Default execute medians rose slightly (pandas 7.339 -> 7.434, Polars 7.212 -> 7.286); their ranges overlap, which does not prove equal cost. Reader-option execute ranges also overlap. No general speedup, statistical-significance or AWS end-to-end latency claim is made. Default interrupt mode is reported separately without a speedup claim.
| Case, microseconds/call | Before median [range] | This head median [range] |
|---|---|---|
| connect_local_session | 15.988 [15.497–16.665] | 15.970 [15.498–16.445] |
| Connection_local_session | 15.733 [15.213–16.373] | 15.634 [15.255–16.117] |
| AioConnection_local_session | 15.865 [15.451–16.304] | 16.004 [15.396–16.644] |
| Cursor.construct_close | 12.828 [12.643–13.222] | 12.752 [12.600–13.199] |
| Cursor.execute_defaults | 6.498 [6.383–6.969] | 6.477 [6.401–6.740] |
| Cursor.execute_keywords | 6.683 [6.558–6.911] | 6.634 [6.575–7.063] |
| Cursor.execute_prebuilt | 8.146 [7.983–9.185] | 8.196 [8.004–8.544] |
| AsyncCursor.construct_close | 13.209 [13.052–13.923] | 13.277 [13.136–14.118] |
| AsyncCursor.execute_defaults | 8.129 [7.891–8.433] | 8.175 [7.970–8.425] |
| AsyncCursor.execute_keywords | 8.273 [8.096–8.505] | 8.304 [8.169–8.485] |
| AsyncCursor.execute_prebuilt | 9.708 [9.513–10.070] | 9.883 [9.494–10.148] |
| AioCursor.construct_close | 12.738 [12.605–13.274] | 12.861 [12.643–13.535] |
| AioCursor.execute_defaults | 6.980 [6.744–7.197] | 7.199 [6.792–7.244] |
| AioCursor.execute_keywords | 7.181 [6.984–7.548] | 7.217 [7.085–7.457] |
| AioCursor.execute_prebuilt | 8.596 [8.392–9.002] | 8.659 [8.465–9.046] |
| PandasCursor.construct_close | 8.024 [7.865–8.238] | 7.961 [7.772–8.416] |
| PandasCursor.execute_defaults | 7.339 [7.246–7.570] | 7.434 [7.318–7.684] |
| PandasCursor.execute_keywords | 7.574 [7.374–7.616] | 7.661 [7.529–7.831] |
| PandasCursor.execute_prebuilt | 8.945 [8.817–9.228] | 9.160 [8.971–9.406] |
| AsyncPandasCursor.construct_close | 9.653 [9.386–9.785] | 9.566 [9.405–9.836] |
| AsyncPandasCursor.execute_defaults | 8.837 [8.642–9.215] | 9.109 [8.805–9.639] |
| AsyncPandasCursor.execute_keywords | 8.960 [8.756–9.361] | 9.175 [8.891–9.568] |
| AsyncPandasCursor.execute_prebuilt | 10.343 [10.207–10.727] | 10.612 [10.322–10.869] |
| AioPandasCursor.construct_close | 7.868 [7.774–8.159] | 7.910 [7.784–8.355] |
| AioPandasCursor.execute_defaults | 8.142 [7.953–8.415] | 8.383 [7.983–8.696] |
| AioPandasCursor.execute_keywords | 8.321 [8.141–9.047] | 8.499 [8.160–8.804] |
| AioPandasCursor.execute_prebuilt | 9.632 [9.590–10.452] | 9.970 [9.789–10.179] |
| ArrowCursor.construct_close | 9.116 [8.986–9.500] | 9.103 [8.913–9.314] |
| ArrowCursor.execute_defaults | 6.929 [6.786–7.137] | 6.951 [6.752–7.309] |
| ArrowCursor.execute_keywords | 7.075 [6.976–7.180] | 7.160 [6.913–7.304] |
| ArrowCursor.execute_prebuilt | 8.520 [8.407–8.821] | 8.576 [8.414–8.703] |
| AsyncArrowCursor.construct_close | 10.887 [10.657–11.484] | 10.904 [10.671–11.065] |
| AsyncArrowCursor.execute_defaults | 8.415 [8.265–8.802] | 8.491 [8.195–8.636] |
| AsyncArrowCursor.execute_keywords | 8.595 [8.434–8.741] | 8.631 [8.346–8.777] |
| AsyncArrowCursor.execute_prebuilt | 9.993 [9.838–10.586] | 10.049 [9.739–10.403] |
| AioArrowCursor.construct_close | 9.094 [8.925–9.570] | 9.026 [8.933–9.245] |
| AioArrowCursor.execute_defaults | 7.460 [7.408–7.850] | 7.479 [7.356–7.797] |
| AioArrowCursor.execute_keywords | 7.683 [7.577–8.216] | 7.765 [7.535–7.924] |
| AioArrowCursor.execute_prebuilt | 9.203 [9.015–9.600] | 9.190 [8.943–9.857] |
| PolarsCursor.construct_close | 8.335 [8.276–8.568] | 8.330 [8.151–8.507] |
| PolarsCursor.execute_defaults | 7.212 [6.987–7.332] | 7.286 [7.148–7.466] |
| PolarsCursor.execute_keywords | 7.297 [7.175–7.681] | 7.442 [7.234–7.619] |
| PolarsCursor.execute_prebuilt | 8.822 [8.593–8.950] | 8.982 [8.848–9.277] |
| AsyncPolarsCursor.construct_close | 9.970 [9.858–10.324] | 10.010 [9.871–11.062] |
| AsyncPolarsCursor.execute_defaults | 8.589 [8.467–8.977] | 8.820 [8.571–11.064] |
| AsyncPolarsCursor.execute_keywords | 8.689 [8.591–8.865] | 8.896 [8.651–10.017] |
| AsyncPolarsCursor.execute_prebuilt | 10.289 [10.017–10.715] | 10.365 [10.196–10.708] |
| AioPolarsCursor.construct_close | 8.282 [8.203–8.587] | 8.231 [8.107–8.466] |
| AioPolarsCursor.execute_defaults | 7.865 [7.623–8.106] | 7.868 [7.704–8.337] |
| AioPolarsCursor.execute_keywords | 8.017 [7.794–8.224] | 8.114 [7.882–8.432] |
| AioPolarsCursor.execute_prebuilt | 9.358 [9.193–9.705] | 9.645 [9.339–10.083] |
| S3FSCursor.construct_close | 12.536 [12.377–12.811] | 12.713 [12.504–13.017] |
| S3FSCursor.execute_defaults | 6.634 [6.525–6.854] | 6.722 [6.562–7.106] |
| S3FSCursor.execute_keywords | 6.876 [6.668–7.040] | 6.853 [6.759–7.281] |
| S3FSCursor.execute_prebuilt | 8.215 [8.127–8.447] | 8.390 [8.160–8.802] |
| AsyncS3FSCursor.construct_close | 14.339 [13.910–15.454] | 14.189 [14.130–14.758] |
| AsyncS3FSCursor.execute_defaults | 8.118 [8.015–8.789] | 8.237 [8.035–8.579] |
| AsyncS3FSCursor.execute_keywords | 8.246 [8.193–8.517] | 8.300 [8.151–8.704] |
| AsyncS3FSCursor.execute_prebuilt | 9.681 [9.549–10.004] | 9.814 [9.632–10.148] |
| AioS3FSCursor.construct_close | 12.446 [12.249–12.834] | 12.515 [12.414–12.899] |
| AioS3FSCursor.execute_defaults | 7.268 [7.144–7.370] | 7.241 [7.184–7.693] |
| AioS3FSCursor.execute_keywords | 7.449 [7.364–7.822] | 7.548 [7.372–7.946] |
| AioS3FSCursor.execute_prebuilt | 8.830 [8.736–9.151] | 8.944 [8.746–9.243] |
| pandas.validate_csv_cursor_settings | 1.018 [0.999–1.069] | 0.142 [0.140–0.147] |
| pandas.validate_csv_reader_setting | 0.951 [0.941–0.972] | 0.712 [0.694–0.766] |
| PandasCursor.execute_csv_cursor_settings | 9.146 [9.069–9.417] | 8.319 [8.223–8.510] |
| PandasCursor.execute_csv_reader_setting | 8.884 [8.637–9.078] | 8.696 [8.555–8.953] |
| AsyncPandasCursor.execute_csv_cursor_settings | 10.825 [10.675–11.277] | 9.808 [9.740–10.275] |
| AsyncPandasCursor.execute_csv_reader_setting | 10.342 [10.252–10.912] | 10.220 [10.027–10.820] |
| AioPandasCursor.execute_csv_cursor_settings | 10.062 [9.955–10.330] | 9.127 [8.865–9.475] |
| AioPandasCursor.execute_csv_reader_setting | 9.667 [9.427–10.563] | 9.545 [9.324–10.028] |
| pandas.validate_parquet_cursor_settings | 1.789 [1.712–1.874] | 0.144 [0.140–0.151] |
| pandas.validate_parquet_reader_setting | 1.696 [1.654–1.787] | 1.290 [1.260–1.381] |
| PandasCursor.execute_parquet_cursor_settings | 23.432 [22.873–24.205] | 21.011 [20.800–22.733] |
| PandasCursor.execute_parquet_reader_setting | 22.930 [22.469–23.704] | 22.477 [22.298–24.173] |
| AsyncPandasCursor.execute_parquet_cursor_settings | 25.302 [24.611–26.051] | 22.899 [22.704–24.360] |
| AsyncPandasCursor.execute_parquet_reader_setting | 24.618 [24.163–25.341] | 24.326 [23.804–25.979] |
| AioPandasCursor.execute_parquet_cursor_settings | 24.383 [23.727–25.377] | 22.106 [21.807–23.468] |
| AioPandasCursor.execute_parquet_reader_setting | 24.002 [23.125–26.249] | 23.232 [23.000–24.797] |
| polars.validate_csv_cursor_settings | 2.382 [2.334–2.544] | 0.152 [0.149–0.163] |
| polars.validate_csv_reader_setting | 2.296 [2.260–2.398] | 2.098 [2.075–2.241] |
| PolarsCursor.execute_csv_cursor_settings | 10.522 [10.214–10.888] | 8.013 [7.906–8.348] |
| PolarsCursor.execute_csv_reader_setting | 9.104 [8.955–9.442] | 8.921 [8.835–9.355] |
| AsyncPolarsCursor.execute_csv_cursor_settings | 12.074 [11.840–12.659] | 9.591 [9.405–9.907] |
| AsyncPolarsCursor.execute_csv_reader_setting | 10.869 [10.482–11.185] | 10.624 [10.503–11.153] |
| AioPolarsCursor.execute_csv_cursor_settings | 11.159 [10.956–12.222] | 8.645 [8.584–9.111] |
| AioPolarsCursor.execute_csv_reader_setting | 9.861 [9.739–10.414] | 9.681 [9.523–9.948] |
| polars.validate_parquet_cursor_settings | 1.415 [1.396–1.505] | 0.152 [0.150–0.163] |
| polars.validate_parquet_reader_setting | 1.339 [1.329–1.430] | 1.124 [1.086–1.153] |
| PolarsCursor.execute_parquet_cursor_settings | 23.095 [22.262–23.951] | 20.818 [20.296–21.687] |
| PolarsCursor.execute_parquet_reader_setting | 22.695 [21.757–23.069] | 21.896 [21.615–23.190] |
| AsyncPolarsCursor.execute_parquet_cursor_settings | 24.996 [24.018–25.836] | 22.741 [22.447–24.248] |
| AsyncPolarsCursor.execute_parquet_reader_setting | 24.153 [23.491–25.629] | 23.803 [23.494–26.614] |
| AioPolarsCursor.execute_parquet_cursor_settings | 24.229 [23.073–24.678] | 21.690 [21.316–23.273] |
| AioPolarsCursor.execute_parquet_reader_setting | 23.674 [22.718–24.592] | 22.741 [22.401–23.699] |
| native_import.pandas | 0.071 [0.070–0.075] | 0.070 [0.069–0.080] |
| native_import.polars | 0.070 [0.069–0.076] | 0.070 [0.070–0.076] |
| native_import.pyarrow.parquet | 0.161 [0.157–0.167] | 0.155 [0.150–0.161] |
| cached_import.pandas | 0.297 [0.290–0.305] | 0.296 [0.290–0.315] |
| cached_import.polars | 0.301 [0.290–0.321] | 0.296 [0.291–0.316] |
| cached_import.pyarrow.parquet | 0.306 [0.293–0.313] | 0.298 [0.295–0.306] |
| ExecuteOptions.resolve_none_all_none | 1.335 [1.305–1.403] | 1.323 [1.297–1.400] |
| ExecuteOptions.resolve_prebuilt_all_none | 2.899 [2.866–3.059] | 2.901 [2.812–2.968] |
| Cursor.execute_default_interrupt_mode | 54.468 [44.608–63.671] | 50.946 [39.936–56.303] |
| PandasCursor.execute_default_interrupt_mode | 53.078 [50.963–68.188] | 53.907 [45.965–59.078] |
| AioCursor.execute_default_interrupt_mode | 54.060 [49.994–63.443] | 50.403 [49.179–61.449] |
Reproduction and full child/driver
Save the child and driver below under /private/tmp/pyathena-1093-reader-preparation, together with the provenance JSON. Use a clean checkout of the candidate head at the driver's worktree path and identical dependencies/interpreter for both cohorts. Export the baseline using the exact input commits:
git merge-tree --write-tree 5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee f3532e9fb57fd769441e533f0b8375fd854ece69
mkdir -p /private/tmp/pyathena-1093-reader-preparation/before-on-master
git archive b1d87c391dce1484633d1872a900c9cc6a900bec | tar -xf - -C /private/tmp/pyathena-1093-reader-preparation/before-on-master
uv run python /private/tmp/pyathena-1093-reader-preparation/run-comparison.py > results.jsonProvenance (benchmark-source.json):
{
"before_tree": "b1d87c391dce1484633d1872a900c9cc6a900bec",
"before_inputs": [
"5c96c9257f64f6fa5c7fa1bc1cee47d8f496ebee",
"f3532e9fb57fd769441e533f0b8375fd854ece69"
],
"after_head": "c42d65d04a54d3fe1b5cb90122635b495a64d09d",
"upstream": "f3532e9fb57fd769441e533f0b8375fd854ece69",
"scope": "Both sources include the same current master, including the merged options optimization. Before is a conflict-free merge-tree of the previous published validation head and that master, exported without a git worktree; after is the current PR head."
}Child (measure-api.py):
import asyncio
from concurrent.futures import Future
from importlib import import_module
from inspect import iscoroutinefunction
import json
from pathlib import Path
import platform
import sys
import timeit
from types import SimpleNamespace
source = Path(sys.argv[1]).resolve()
sys.path.insert(0, str(source))
import pyathena
from pyathena.aio.connection import AioConnection
from pyathena.connection import Connection
from pyathena.model import AthenaQueryExecution
from pyathena.options import ExecuteOptions
from pyathena.spark.common import SparkBaseCursor
assert Path(pyathena.__file__).resolve().parent.parent == source
iterations = int(sys.argv[2])
class NoAWSClient:
def close(self):
pass
def __getattr__(self, name):
raise AssertionError('Unexpected AWS client access: ' + name)
class LocalSession:
def client(self, *args, **kwargs):
return NoAWSClient()
class LocalResult:
def __init__(self, *args, **kwargs):
self.is_closed = False
def close(self):
self.is_closed = True
@classmethod
async def create(cls, *args, **kwargs):
return cls(*args, **kwargs)
class ImmediateExecutor:
def submit(self, fn, *args, **kwargs):
result = Future()
result.set_result(fn(*args, **kwargs))
return result
def shutdown(self, *args, **kwargs):
pass
async def immediate_thread(fn, *args, **kwargs):
return fn(*args, **kwargs)
asyncio.to_thread = immediate_thread
SparkBaseCursor._start_session = lambda *args, **kwargs: 'session'
query = SimpleNamespace(state=AthenaQueryExecution.STATE_SUCCEEDED)
async def start_async(*args, **kwargs):
return 'query'
async def poll_async(*args, **kwargs):
return query
session = LocalSession()
connection_args = dict(region_name='us-east-1', s3_staging_dir='s3://bucket/path/', work_group='wg', session=session, kill_on_interrupt=False)
connection = Connection(**connection_args)
loop = asyncio.new_event_loop()
results = {}
def measure(name, call, count=None):
count = iterations if count is None else count
for _ in range(30):
call()
results[name] = timeit.timeit(call, number=count) / count * 1e6
def create_connection(factory):
conn = factory(**connection_args)
conn.close()
measure('connect_local_session', lambda: create_connection(pyathena.connect))
measure('Connection_local_session', lambda: create_connection(Connection))
measure('AioConnection_local_session', lambda: create_connection(AioConnection))
cursor_definitions = [
('pyathena.cursor', 'Cursor'),
('pyathena.async_cursor', 'AsyncCursor'),
('pyathena.aio.cursor', 'AioCursor'),
]
for backend in ('pandas', 'arrow', 'polars', 's3fs'):
name = {'pandas': 'Pandas', 'arrow': 'Arrow', 'polars': 'Polars', 's3fs': 'S3FS'}[backend]
cursor_definitions.extend([
('pyathena.' + backend + '.cursor', name + 'Cursor'),
('pyathena.' + backend + '.async_cursor', 'Async' + name + 'Cursor'),
('pyathena.aio.' + backend + '.cursor', 'Aio' + name + 'Cursor'),
])
for module_name, class_name in cursor_definitions:
module = import_module(module_name)
cursor_class = getattr(module, class_name)
for name, value in vars(module).copy().items():
if name.startswith('Athena') and name.endswith('ResultSet'):
setattr(module, name, LocalResult)
if 'Pandas' in class_name:
import_module('pandas')
if 'Polars' in class_name:
import_module('polars')
if 'Arrow' in class_name:
import_module('pyarrow')
def close(cursor):
if iscoroutinefunction(cursor.close):
loop.run_until_complete(cursor.close())
else:
cursor.close()
def create_cursor():
cursor = connection.cursor(cursor_class)
close(cursor)
measure(class_name + '.construct_close', create_cursor)
cursor = connection.cursor(cursor_class)
if hasattr(cursor, '_result_set_class'):
cursor._result_set_class = LocalResult
if class_name.startswith('Async'):
cursor._executor.shutdown(wait=True)
cursor._executor = ImmediateExecutor()
if iscoroutinefunction(cursor.execute):
cursor._start_query_execution = start_async
cursor._poll = poll_async
async def execute_batch(settings):
for _ in range(iterations):
await cursor.execute('SELECT 1', **settings)
for label, settings in [('defaults', {}), ('keywords', {'work_group': 'wg', 'cache_size': 0, 'paramstyle': 'pyformat'}), ('prebuilt', {'options': ExecuteOptions(work_group='wg')})]:
loop.run_until_complete(execute_batch(settings))
duration = timeit.timeit(lambda: loop.run_until_complete(execute_batch(settings)), number=1)
results[class_name + '.execute_' + label] = duration / iterations * 1e6
else:
cursor._start_query_execution = lambda *args, **kwargs: 'query'
cursor._poll = lambda *args, **kwargs: query
for label, settings in [('defaults', {}), ('keywords', {'work_group': 'wg', 'cache_size': 0, 'paramstyle': 'pyformat'}), ('prebuilt', {'options': ExecuteOptions(work_group='wg')})]:
def execute():
value = cursor.execute('SELECT 1', **settings)
if isinstance(value, tuple):
value[1].result()
measure(class_name + '.execute_' + label, execute)
close(cursor)
# Focused reader preparation paths with real option resolution and request formatting.
for backend in ('pandas', 'polars'):
name = 'Pandas' if backend == 'pandas' else 'Polars'
reader_options = {'sep': '|'} if backend == 'pandas' else {'separator': '|'}
parquet_options = {'use_threads': False} if backend == 'pandas' else {'parallel': 'none'}
cursor_options = {'chunksize': 10, 'block_size': 64, 'cache_type': 'bytes', 'max_workers': 2}
validator = import_module('pyathena.' + backend + '.result_set').validate_execute_kwargs
for mode in (False, True):
mode_label = 'parquet' if mode else 'csv'
for label, settings in [('cursor_settings', cursor_options), ('reader_setting', parquet_options if mode else reader_options)]:
if backend == 'pandas':
call = lambda: validator('Cursor.execute', settings, mode)
else:
call = lambda: validator('Cursor.execute', settings, mode, 10)
measure(backend + '.validate_' + mode_label + '_' + label, call)
for module_name, class_name in [
('pyathena.' + backend + '.cursor', name + 'Cursor'),
('pyathena.' + backend + '.async_cursor', 'Async' + name + 'Cursor'),
('pyathena.aio.' + backend + '.cursor', 'Aio' + name + 'Cursor'),
]:
cursor = connection.cursor(getattr(import_module(module_name), class_name), unload=mode)
if hasattr(cursor, '_result_set_class'):
cursor._result_set_class = LocalResult
if class_name.startswith('Async'):
cursor._executor.shutdown(wait=True)
cursor._executor = ImmediateExecutor()
if iscoroutinefunction(cursor.execute):
cursor._start_query_execution = start_async
cursor._poll = poll_async
else:
cursor._start_query_execution = lambda *args, **kwargs: 'query'
cursor._poll = lambda *args, **kwargs: query
for label, settings in [('cursor_settings', cursor_options), ('reader_setting', parquet_options if mode else reader_options)]:
if iscoroutinefunction(cursor.execute):
async def reader_batch():
for _ in range(iterations):
await cursor.execute('SELECT 1', **settings)
loop.run_until_complete(reader_batch())
duration = timeit.timeit(lambda: loop.run_until_complete(reader_batch()), number=1)
results[class_name + '.execute_' + mode_label + '_' + label] = duration / iterations * 1e6
else:
def reader_execute():
value = cursor.execute('SELECT 1', **settings)
if isinstance(value, tuple):
value[1].result()
measure(class_name + '.execute_' + mode_label + '_' + label, reader_execute)
close(cursor)
def native_pandas_import():
import pandas
return pandas
def native_polars_import():
import polars
return polars
def native_parquet_import():
import pyarrow.parquet as parquet
return parquet
for name, call in [('pandas', native_pandas_import), ('polars', native_polars_import), ('pyarrow.parquet', native_parquet_import)]:
assert call() is import_module(name)
measure('native_import.' + name, call)
for module_name in ('pandas', 'polars', 'pyarrow.parquet'):
loaded = import_module(module_name)
assert import_module(module_name) is loaded and sys.modules[module_name] is loaded
measure('cached_import.' + module_name, lambda: import_module(module_name))
overrides = dict(work_group=None, s3_staging_dir=None, cache_size=None, cache_expiration_time=None, result_reuse_enable=None, result_reuse_minutes=None, paramstyle=None, on_start_query_execution=None, result_set_type_hints=None)
measure('ExecuteOptions.resolve_none_all_none', lambda: ExecuteOptions.resolve(None, **overrides))
base = ExecuteOptions(work_group='wg')
measure('ExecuteOptions.resolve_prebuilt_all_none', lambda: ExecuteOptions.resolve(base, **overrides))
default_connection = Connection(**dict(connection_args, kill_on_interrupt=True))
for module_name, class_name in [('pyathena.cursor','Cursor'),('pyathena.pandas.cursor','PandasCursor'),('pyathena.aio.cursor','AioCursor')]:
cursor = default_connection.cursor(getattr(import_module(module_name),class_name))
if hasattr(cursor, '_result_set_class'):
cursor._result_set_class = LocalResult
if iscoroutinefunction(cursor.execute):
cursor._start_query_execution = start_async
cursor._poll = poll_async
async def default_batch():
for _ in range(100):
await cursor.execute('SELECT 1')
loop.run_until_complete(default_batch())
results[class_name+'.execute_default_interrupt_mode'] = timeit.timeit(lambda: loop.run_until_complete(default_batch()),number=1)/100*1e6
else:
cursor._start_query_execution = lambda *args, **kwargs: 'query'
cursor._poll = lambda *args, **kwargs: query
measure(class_name+'.execute_default_interrupt_mode', lambda: cursor.execute('SELECT 1'),count=100)
close(cursor)
default_connection.close()
connection.close()
loop.close()
print(json.dumps({'python': platform.python_version(), 'machine': platform.machine(), 'source': str(source), 'iterations': iterations, 'libraries': {name: import_module(name).__version__ for name in ('pandas', 'polars', 'pyarrow', 'sqlalchemy')}, 'results_us': results}))Driver (run-comparison.py):
import json
from pathlib import Path
from statistics import median
import subprocess
import sys
root = Path('/private/tmp/pyathena-1093-reader-preparation')
worktree = Path('/private/tmp/worktrees/PyAthena/fix-1093-reject-unknown-cursor-keywords')
cohorts = [('before', root / 'before-on-master'), ('after', worktree)]
samples = {name: [] for name, _ in cohorts}
for repeat in range(9):
order = cohorts[repeat % len(cohorts):] + cohorts[:repeat % len(cohorts)]
for name, source in order:
command = [sys.executable, str(root / 'measure-api.py'), str(source), '2000']
result = json.loads(subprocess.check_output(command, cwd=worktree, text=True))
samples[name].append(result)
summary = {'source_provenance': json.loads((root / 'benchmark-source.json').read_text()), 'repeats': 9, 'iterations_per_repeat': 2000, 'default_interrupt_mode_iterations': 100, 'order': 'Rotate source cohort order each repeat; sequential fresh subprocesses with identical interpreter and dependencies.', 'scope': 'Actual connection/cursor/execute bodies, retaining BaseCursor._execute and request formatting. Main cases use kill_on_interrupt=False to isolate CPU work. Three separately named default_interrupt_mode cases use True and retain start-request thread/task scheduling. AWS session/client construction, submission/polling, result readers, Future scheduling, and asyncio reader thread offloads use local fakes. Not AWS end-to-end latency or full asynchronous scheduling performance.', 'samples': samples, 'medians_us': {}}
for name, values in samples.items():
summary['medians_us'][name] = {case: {'median': median([v['results_us'][case] for v in values]), 'min': min(v['results_us'][case] for v in values), 'max': max(v['results_us'][case] for v in values)} for case in values[0]['results_us']}
print(json.dumps(summary, indent=2))
| return False | ||
| return operation[start.start() : start.start() + 6].upper().startswith(prefixes) | ||
|
|
||
| def _prepare_reader_query( |
There was a problem hiding this comment.
Relayed independent review: CLEAN, with the non-blocking notes and limitations preserved below.
Reviewer: Claude Code, actual canonical model claude-opus-5-5, first-party max profile, effort high; no Enterprise account or API/provider override. Authentication was verified immediately before launch. Session: 1d2c8d53-2c96-4708-8011-07fe5f424a83. The retry completed successfully after the reported 00:00 Asia/Tokyo reset; the earlier weekly-limit response was not counted as a review.
Frozen current merge-base/head: f3532e9fb57fd769441e533f0b8375fd854ece69..c42d65d04a54d3fe1b5cb90122635b495a64d09d.
Bounded follow-up: the 12-file diff from comparison tree b1d87c391dce1484633d1872a900c9cc6a900bec to this head, with affected current-master and unchanged contracts. All eight original-series commits remain unchanged. The tracked snapshot excluded credentials, .env, memory and unrelated artifacts. Author conclusions, commit messages and PR discussion were omitted. Read/Grep/Glob only, plan/read-only mode, no builds/tests/writes/GitHub/MCP/subagents. Both self-review rounds preceded this review.
The actual successful result was read in full. Post-review verification confirmed the package hash manifest and clean candidate worktree were unchanged, with exact base/head equality.
Verdict: CLEAN
I found no defects introduced by the three follow-up commits. There are three non-blocking notes (one behavioral, two on test strength), and the benchmark supports some conclusions but not others. This was a read-only, static review: I only used Read, Grep and Glob inside the snapshot. Nothing was executed. Plan mode asked for a plan file, but you ruled out edits and the Write and ExitPlanMode tools aren't available here, so the review is below.
Coverage
- Shared helper:
pyathena/common.py:1218-1291, compared with the baseline_validate_reader_kwargs(baseline-source/pyathena/common.py:1218-1241). Also traced_is_unload_query(common.py:1199-1216),_prepare_unload(:1293-1317),_execute(:1362+) andFormatter.wrap_unload(formatter.py:127+). - All six callers:
pandas/cursor.py:215-231pandas/async_cursor.py:245-263aio/pandas/cursor.py:184-201polars/cursor.py:206-222polars/async_cursor.py:248-264aio/polars/cursor.py:164-186
- Both validators:
pandas/result_set.py:38-73andpolars/result_set.py:42-95. Also the Polars result-set uses atpolars/result_set.py:563-569and:640. - Unchanged contracts traced:
options.py:84-120(upstreamresolvefast path,upstream.patch:668-679)_kwargs.py:16-78(signature cache)result_set.py:935-940(_reset_state)- keyword-only
executeand constructors (upstream.patch:230-269, 458-497) docs/cursor.md:14-22pyproject.toml:58-66(extras)
- Tests:
test_kwargs.py:569, :615, plus the existing tests at:200-510pandas/test_result_set.py:723, :736, :751polars/test_result_set.py:438, :451
Behavior audit (no defects)
- Equivalence with the baseline: the helper reproduces the old inline code exactly:
- Options are resolved, then UNLOAD is prepared.
- On an
Exception, a non-empty kwargs dict is validated against the unprepared operation (prepared=False, so SELECT/WITH/UNLOAD select Parquet). Thenon_prepare_errorruns, then the original error is re-raised. - On success, validation uses the prepared operation (
prepareddefaults to True,common.py:1199). - In the failure path
operationis still the original. A failed tuple assignment never rebinds it, and a failure inresolvehappens before the assignment.
- Rejection order and prior state:
- An unknown keyword raises
TypeErrorbeforeon_prepare_error, so earlier state is kept. This matches the baseline andtest_kwargs.py:282-334. - Sync and aio cursors pass
self._reset_state, which is synchronous (result_set.py:935), so there is no un-awaited coroutine. - Future-based cursors pass no callback and keep their earlier no-reset behavior.
- Successful paths still reset after the helper returns.
- An unknown keyword raises
- Exception vs. interrupts:
except ExceptionleavesKeyboardInterruptandSystemExitunvalidated and unreset. The new test at:615locks this in. - Options, callbacks and type hints:
- Calls go through
ExecuteOptions.resolveby class literal, so a subclass instance passed asoptionsstill dispatches to its ownmerge(). Noneoverrides are ignored, so Future-based cursors omittingon_start_query_executionkeep any value fromoptions(unchanged).- Callback and hint references pass through by identity (upstream
test_options.py). - Every helper option is keyword-only after
*reader_args, and all callers pass them by name, so the upstream*changes are compatible.
- Calls go through
- Effective chunksize: Polars callers still pass
kwargs.get("chunksize", self._chunksize). The result-set checks (allow_eager_fallback=False) are unchanged. - Fast path:
kwargs.keys() <= _CURSOR_KWARGSreturns early only when every key is a cursor setting. The full path always added_CURSOR_KWARGSto the allowed names, so keyword results are identical. Mixed kwargs still take the full path, as does the Polars alias loop and the managedread_csvfallback. - Native imports:
- In-function
import pandas/import polarsmatches the module's existing idiom (e.g.pandas/result_set.py:109,polars/result_set.py:141). - The reader is still looked up as
pandas.read_csvat call time, so replacing it is honored. Thelru_cacheis keyed by function identity, so a replacement gets a new cache entry (_kwargs.py:16-46). - One edge-case difference:
import pyarrow.parquet as parquetresolves through thepyarrow.parquetattribute before falling back tosys.modules. Sopatch.dict(sys.modules, {"pyarrow.parquet": fake})after pyarrow is already imported is no longer seen by the validator;import_modulewould have seen it. This is informational only and not a supported contract. - Native imports go through
builtins.__import__;import_moduledid not. That only matters for import hooks, and the new tests rely on it.
- In-function
Non-blocking notes
- Low — earlier failure removed when an optional dependency is missing (
pandas/result_set.py:61-66).- Scenario: only
pyathena[pandas]is installed (no pyarrow,pyproject.toml:58-60) and you callPandasCursor(unload=True).execute("SELECT ...", chunksize=50_000). - Before:
ModuleNotFoundErrorwas raised beforeStartQueryExecution. - Now: the UNLOAD runs on Athena (billed, writes Parquet to S3), and the failure only happens when results are read.
- This is not a regression of any contract: the baseline already ran the query for the same call with no kwargs, or with
chunksizeset on the constructor. The early failure was incidental. It is worth knowing if anyone relied on it.
- Scenario: only
- Test strength:
test_kwargs.py:569distinguishes "validate the original operation withprepared=False" from correct behavior. It cannot catch "validate the original withprepared=True" (SELECT 1andDESCRIBE tboth pick CSV). The existing tests at:394and:418do catch that, so overall coverage is adequate.- The polars import test's
"pyarrow.parquet"check (polars/test_result_set.py:441-444) is vacuous, because that validator never imports pyarrow. - If the code went back to
import_module, the__import__spy would pass silently. Thekeyword_parameters.assert_not_called()check still catches it.
- Characterization tests: all of the new tests would also pass on the baseline. They protect the refactor rather than show a fixed failure, which is appropriate for a refactor with a fast path.
Benchmark evidence
Attribution: the 106 cases add up correctly: 3 connection, 60 cursor (15 classes × 4), 32 reader-preparation, 6 import, 2 resolve, 3 interrupt-mode.
-
Control cases: the import and
resolvecases run the same code in both cohorts.resolvecame out at 1.33 vs 1.32 µs and 2.90 vs 2.90 µs. Connection cases were within 0.1–0.2 µs. -
Fast path (clear result, ranges don't overlap): cursor-settings validation dropped as follows:
Validator Before (µs) After (µs) pandas CSV 1.02 0.14 pandas Parquet 1.79 0.14 Polars CSV 2.38 0.15 Polars Parquet 1.41 0.15 -
Native import: reader-setting validation improved by 0.20–0.41 µs. That matches the measured cost difference: cached
import_moduleis about 0.30 µs vs about 0.07 µs for a native import (0.155 µs forpyarrow.parquet). The two Parquet imports explain the 0.41 µs pandas Parquet gain. -
Cursor-level gains are larger than the validator gains for Parquet: 2.3–2.5 µs vs 1.26–1.64 µs. The roughly 1 µs left over isn't explained by the evidence; it may be noise from the UUID/UNLOAD path or allocation effects.
-
Possible small cost on the no-reader-kwargs path: pandas/Polars
execute_{defaults,keywords,prebuilt}are about +0.18 µs on average, against about +0.06 µs for unchanged cursors. That fits the extra 13-keyword call, but the per-case ranges overlap and the control spread reaches +0.22 µs (AioCursor.execute_defaults). The evidence neither confirms nor rules out a regression of about 0.1–0.2 µs. -
Interrupt-mode cases (100 iterations, range 40–68 µs) support no conclusion.
Harness behavior:
- Retained layers: real execute bodies, option resolution, formatting/UNLOAD wrapping, validators and
_execute. - Faked layers: AWS client, start/poll, result sets, and
asyncio.to_thread. TheImmediateExecutorruns work synchronously, so Future scheduling and thread offload are not measured. - Warm-up: 30 calls for sync cases; one full batch of 2000 for asyncio.
- GC:
timeitdisables GC during timing. - Imports and caches: pandas, polars and pyarrow are imported, and signature caches warmed, before timing. Cold imports and first
inspect.signaturecosts are not measured. - Interpreter and cohorts: Python 3.13.1 on arm64, 9 fresh subprocesses per cohort, alternating order.
Provenance limitations:
- The harness prints
str(source)as a resolved absolute path (measure-api.py:245), but the samples say"source": "before"/"after"(samples.json:22, 1104). Either the samples were edited afterwards or a different harness revision produced them. - No file or harness hashes were recorded.
- "after" is a live git worktree, so uncommitted changes can't be ruled out.
- "before" is an exported synthesized merge-tree that the samples can't verify.
- Aio cursors were created from a sync
Connection. - Local timing like this says nothing about AWS end-to-end latency or how asynchronous scheduling performs.
Static-review limitations
I ran no tests, type checking, linting or benchmarks. Python import-machinery details are reasoned from CPython semantics, not observed. I assumed followup.patch matches the comparison tree → head diff stated in review-shas.txt.
Author verification and disposition of the non-blocking notes:
- Missing optional dependencies remain a lazy result-reading concern. The cursor-setting-only guard removes an incidental earlier dependency failure for some kwargs forms; this boundary is stated in the description. Unknown reader names still reject before submission. This is consistent with the existing no-extra-kwargs and constructor-settings paths. No additional optional-dependency preflight is introduced.
- Preparation-mode coverage is combined with the existing original/prepared-mode tests, as the reviewer noted. The extra pyarrow name in the Polars import spy is harmless. Ordinary imports and callable replacement are the supported paths; a divergent parent-module attribute versus sys.modules monkeypatch is outside that contract. No source repair is required.
- The statement that every new test passes on the baseline is too broad. Static comparison of baseline validators shows that non-empty cursor-only settings unconditionally call keyword_parameters; the new assert_not_called assertion rejects that baseline path. The state/mode/interrupt and replacement cases are intentional characterization coverage. No baseline test execution is claimed.
- The review copy deliberately normalizes only each sample's absolute source path to the before/after cohort label. Compared it with the retained raw results: every timing, aggregate, environment field and other value is identical. This explains the provenance observation; a different harness revision did not produce that difference.
- Verified all 293 tracked files in each source cohort against the comparison-tree/candidate Git blob IDs after the review. The reviewed harness/driver/provenance files match their retained pre-review SHA-256 manifest. Child harness:
0b10d65b9780c50232d71a5bd4994aa723f93197ebf725288e3ba64e65a8314a; driver:6b932d83a292ec0fc5cc5881e4a13b5657f05ed4cb261b190c5679a6b71990f5; retained raw samples:61ee45e9650ac845bf25174e0dfae5f81963235a65c4ced2f24499cc0def4cfe. These checks establish current file identity, not atomic immutability throughout the earlier measurement run. - The benchmark deliberately uses local fake providers and a sync Connection for aio cursor bodies; it does not cover asynchronous connection lifecycle or scheduling. The unexplained remainder of the Parquet execute differences is not attributed to the import optimization. Small default-path costs remain possible; overlapping observed ranges do not establish equal cost. Interrupt-mode timings and these local timings support no AWS end-to-end performance conclusion.
No actionable introduced defects were identified, so no source repair or independent repair follow-up was needed. Final-source format/lint and 683 offline tests have passed. Current-head AWS validation will be recorded separately after Ready.
Pandas and Polars readers already reject unknown keyword names. Checking them before the query required signature inspection, UNLOAD classification, Polars alias handling, and a signature cache, so keep the pandas and Polars cursors as on master. Move the execute() keyword check and the constructor keyword names onto BaseCursor, drop pyathena/_kwargs.py, and move the tests next to the keyword-only tests in the connection test modules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A custom default cursor class can consume its own options through **kwargs, which constructor introspection cannot see. Apply only the cursor_kwargs that the internal cursor class accepts. Document that cursor subclasses must not pass their own settings to the base constructor, and how pandas and Polars report unknown reader option names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| """The connection that created this cursor.""" | ||
| return self._connection | ||
|
|
||
| def _validate_execute_kwargs( |
There was a problem hiding this comment.
Self-review round one (implementation behavior) — FINDINGS, repaired.
Base: f3532e9fb57fd769441e533f0b8375fd854ece69
Head: a8d79da40e176d553555233c0e5fa5cbb110569d (initial pass at 45639cd638ea3f3d71a7fe105084619607fa0296)
Scope: the reworked PR after removing pandas/Polars reader pre-validation. Covered every execute() override in pyathena/ (12 validate; the 6 pandas/Polars variants forward to readers unchanged from master), all constructor chains reaching BaseCursor.__init__ (Spark, WithResultSet, dict cursors), executemany() delegation, Connection.cursor()/_cursor()/_internal_cursor(), the sync and async SQLAlchemy adapters, and the new tests in tests/pyathena/test_connection.py, tests/pyathena/aio/test_connection.py, and tests/pyathena/sqlalchemy/test_base.py. Also ran /code-review (medium) on the PR.
Repaired in a8d79da4:
pyathena/connection.py:669:_internal_cursor()filtered out only names declared by the default class's constructors, so a custom default cursor that consumescursor_kwargsvia**kwargs(e.g.kwargs.pop("trace_id")) broke SQLAlchemy reflection with TypeError. Now it applies only the namesCursoraccepts; regression testtest_internal_cursor_leaves_out_custom_cursor_options.docs/usage.md:47: the pandas/Polars statement overclaimed; names are not checked when no result file is read and the reader error surfaces asOperationalError.
Deferred with reasons:
cursor_kwargs={"arraysize": 5000}with a DataFrame cursor makes the internalCursorraise ProgrammingError: pre-existing on master (it already forwardedarraysize).executemany(op, [], work_gruop=...)does not raise: no query runs; adding per-method validation for an empty batch is not worth the extra surface.- Explicit keyword-only result-set parameters instead of allow-lists: blocked by the abstract
BaseCursor.execute(**kwargs)signature; a separate API design change. - Caching
_constructor_keyword_names(): only the SQLAlchemy reflection path calls it, next to an Athena query. AsyncSparkCursorbuilds its executor beforesuper().__init__()can raise: no threads are started before submission.
Validation at a8d79da4: just format, just lint, just docs lint passed; the offline selection in the PR body passed (434).
There was a problem hiding this comment.
Round-one repair follow-up at 2bc1ef0e09ce17fb2a81adbf1dfcccaaa61ec26b (range a8d79da40e176d553555233c0e5fa5cbb110569d..2bc1ef0e09ce17fb2a81adbf1dfcccaaa61ec26b): CLEAN. Connection._cursor() takes the cursor class positional-only; both callers pass it positionally, AioConnection inherits the path, and test_cursor_kwargs_named_cursor fails without the change ("multiple values for argument 'cursor'"). Offline selection 435 passed, lint passed.
There was a problem hiding this comment.
Round-one repair follow-up at dd296bf5d231c5025f2f428384eec44a35ae876b (range 2bc1ef0e09ce17fb2a81adbf1dfcccaaa61ec26b..dd296bf5d231c5025f2f428384eec44a35ae876b, merge-base unchanged f3532e9fb57fd769441e533f0b8375fd854ece69): CLEAN. The deferred arraysize item is now fixed at the maintainer's request: Connection._internal_cursor() also leaves out arraysize, so cursor_kwargs={"arraysize": 5000} on a DataFrame/Arrow connection no longer makes the internal Cursor/AioCursor raise ProgrammingError; cursor_kwargs itself is not mutated and user cursors still receive it. The sync, aio, and both SQLAlchemy dialect tests fail without the change (4 failed) and pass with it. executemany([]) is filed as #1112; the Polars dtypes alias (outside this PR) as #1111. Lint passed; offline selection 435 passed.
|
|
||
| Unknown keyword arguments raise `TypeError` in cursor constructors and in `execute()` instead of being ignored. | ||
| The connection's `cursor_kwargs` are passed to every cursor that it creates, so the requested cursor class must accept them. | ||
| A cursor subclass that receives settings of its own must not pass them on to the base class constructor. |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations) — FINDINGS, repaired.
Base: f3532e9fb57fd769441e533f0b8375fd854ece69
Head: a8d79da40e176d553555233c0e5fa5cbb110569d
Claims checked:
- Readers reject unknown names: verified locally with pandas 3.0.6 / Polars 1.44.2 / PyArrow 25.0.1 that
pd.read_csv,pd.read_parquet(pyarrowread_table),pl.read_csv,pl.scan_csv,pl.read_parquet, andpl.scan_parquetraise TypeError; the result sets wrap it asOperationalError(pyathena/pandas/result_set.py,pyathena/polars/result_set.pyexcept Exception)._read_csvreturns an empty frame before calling the reader when there is no readable output, so the docs now say so. - Validation happens before state change or AWS: tests snapshot
vars(cursor)and fail on any_make_api_call; Future-based cursors validate before_execute/submit; Spark before_calculate. on_start_query_execution/on_pollstill accepted by every cursor:test_cursor_accepts_connection_callbacks(sync and aio).- Existing caller: dbt-athena
connections_legacy.py(dbt-labs/dbt-adapters main) buildsAthenaCursor(**kwargs)→Cursor.__init__(**kwargs)withcursor_kwargs={"debug_query_state", "num_iceberg_retries"}and reads them back fromconnection.cursor_kwargs. That now raises TypeError. It pinspyathena>=2.25,<3.35, and 4.0.0 already requires it to change (BREAKING: Make connection and cursor arguments keyword-only #1101, Remove the legacy keyword arguments from the private _execute() #1088). Added the subclass rule todocs/usage.md:45and the example to the PR's release-note text. - AWS operator: no request-count or retry change; unknown names fail before any API call, otherwise the query path is the master path.
No unmeasured performance claim remains: the reworked PR adds a dict-key loop to non-reader execute() and no signature inspection on query paths.
There was a problem hiding this comment.
Round-two repair follow-up at 2bc1ef0e09ce17fb2a81adbf1dfcccaaa61ec26b: CLEAN. Claim corrected in docs/usage.md:47 and the PR body: managed query results (_fetch_all_rows_as_csv) invoke the reader without a result file, so the unchecked case is "no results to read" (no output and empty UNLOAD manifests). No other claim changed.
There was a problem hiding this comment.
Round-two repair follow-up at dd296bf5d231c5025f2f428384eec44a35ae876b: CLEAN. Docstring claim checked: DataFrame/Arrow/S3FS cursors have no arraysize upper bound (WithResultSet), while Cursor/AioCursor cap it at DEFAULT_FETCH_SIZE (1000). The internal cursor only reads reflection rows, so using its default page size changes no user-visible result. PR body updated.
Make the cursor class parameter of Connection._cursor() positional-only
so that cursor_kwargs={"cursor": ...} still reaches a custom cursor.
Managed query results are read without a result file, so describe the
unchecked case as having no results to read.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return self._cursor(cursor, **{**self.cursor_kwargs, **kwargs}) | ||
|
|
||
| def _cursor( | ||
| self, cursor: type[FunctionalCursor] | None, /, **kwargs: Any |
There was a problem hiding this comment.
Relayed independent review — Codex CLI (codex-companion task, default model and effort), read-only static review. Not authored by this reviewer.
Initial review: base f3532e9fb57fd769441e533f0b8375fd854ece69, head a8d79da40e176d553555233c0e5fa5cbb110569d, detached snapshot of tracked files (no .env), prompt without PR number, description, commits, or prior findings; no edits, builds, tests, GitHub, or memory access. Snapshot remained clean.
Covered: all cursor families across sync, Future-based Async, and native Aio (constructor forwarding, supported options, rejection timing, cursor state), pandas/Polars readers, SQLAlchemy reflection and async adaptation, docs, tests, conventions.
FINDINGS:
- P2
pyathena/connection.py:615—Connection.cursor()now calls_cursor(cursor, **merged), so a custom cursor option namedcursorfromcursor_kwargsraises "multiple values for argument 'cursor'" (Connection and AioConnection). Verified; repaired in2bc1ef0eby making the selector positional-only (connection.py:618), withtest_cursor_kwargs_named_cursor. - P3
docs/usage.md:47— managed result storage serializesGetQueryResultsrows and invokes the reader without a result file, so "not checked when no result file is read" was wrong. Verified; reworded to "no results to read".
Note (non-blocking): the supported-keyword tests stop at_execute; forwarding is covered by existing result-settest_read_optionstests.
Follow-up: bounded review of a8d79da40e176d553555233c0e5fa5cbb110569d..2bc1ef0e09ce17fb2a81adbf1dfcccaaa61ec26b traced into Connection.cursor, _internal_cursor, AioConnection, and the SQLAlchemy adapters — CLEAN; both findings repaired. Static review only.
Author validation at 2bc1ef0e: just format, just lint, just docs lint passed; offline selection in the PR body 435 passed.
There was a problem hiding this comment.
Relayed independent follow-up — Codex CLI (codex-companion task, default model/effort), read-only static review of 2bc1ef0e09ce17fb2a81adbf1dfcccaaa61ec26b..dd296bf5d231c5025f2f428384eec44a35ae876b on a detached snapshot, traced into Connection, AioConnection, both SQLAlchemy reflection paths, the async adapter, and the updated tests: CLEAN. Static review only; snapshot unchanged.
A DataFrame cursor allows an arraysize above Cursor's page size limit of 1000, so applying it to the internal API cursor made SQLAlchemy reflection raise ProgrammingError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Spark work_group rejection now comes from the generic unknown-keyword check added in #1102, so one case covers it. Fallback cases use explicit expected values instead of normalizing them in the test body. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Spark work_group rejection now comes from the generic unknown-keyword check added in #1102, so one case covers it. Fallback cases use explicit expected values instead of normalizing them in the test body. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
WHAT
Cursor constructors and
execute()raiseTypeErrorfor unknown keyword arguments instead of ignoring them.BaseCursor.__init__no longer accepts**kwargs, so an unknown constructor name, given directly or through the connection'scursor_kwargs, raisesTypeError.on_start_query_executionandon_pollremain accepted by every cursor.execute()of the default, dict, Arrow, S3FS, and Spark cursors (sync, Future-based, and native asyncio) checks its extra keyword names against the result-set options it supports before changing cursor state or starting a query or calculation.execute()keyword arguments they do not use themselves to the reader (pandas.read_csv(),polars.read_csv(), ...). The reader rejects an unknown name when it reads the query results, raised asOperationalError; the name is not checked when there are no results to read.Connection._internal_cursor(), which applies only thecursor_kwargsthatCursoraccepts, leaving out options of the default cursor class such asunloadandchunksizeof the pandas dialect, andarraysize(a DataFrame cursor allows values aboveCursor's 1000 limit, which made reflection raiseProgrammingError).docs/usage.mddocuments the behavior in the existing PyAthena 4.0 keyword-argument section.WHY
Closes #1093.
Typos and unsupported options were silently dropped.
Breaking change for the 4.0.0 release note: unknown cursor constructor and
execute()keywords now raiseTypeError, andconnection.cursor(OtherCursor)raises if the connection'scursor_kwargscontain options thatOtherCursordoes not accept.A cursor subclass that passes its own settings from
cursor_kwargson to the base constructor now raisesTypeError; for example, dbt-athena's legacy PyAthena connection manager (connections_legacy.py, currently pinned topyathena<3.35) passesdebug_query_stateandnum_iceberg_retriesthis way.An earlier revision of this PR also checked pandas/Polars reader option names against the installed readers' signatures before running the query.
That duplicated the readers' own check and needed signature inspection with a cache, UNLOAD statement classification, Polars deprecated-alias handling, and a second check for the chunked Polars CSV fallback, so it was removed.
TEST
Tested commit:
dd296bf5d231c5025f2f428384eec44a35ae876b.just format,just lint,just docs lint: passed.uv run --env-file .env pytest --noconftest -p no:rerunfailures -q tests/pyathena/test_options.py tests/pyathena/test_connection.py tests/pyathena/aio/test_connection.py tests/pyathena/pandas/test_result_set.py tests/pyathena/polars/test_result_set.py tests/pyathena/sqlalchemy/test_base.py::TestAthenaDialect: 435 passed.cursor_kwargs) and unknownexecute()names with preserved cursor state for every cursor family, supported result-set options, shared callbacks, and the internal API cursor for sync and async connections.dtypesalias), executemany() does not check keyword names for an empty parameter list #1112 (executemany()with an empty parameter list).🤖 Generated with Claude Code