Skip to content

Replace name-mangled helpers with single-underscore methods - #881

Merged
laughingman7743 merged 1 commit into
masterfrom
refactor/879-single-underscore-helpers
Sep 28, 2026
Merged

laughingman7743 merged 1 commit into
masterfrom
refactor/879-single-underscore-helpers

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

WHAT

Replace the 16 non-dunder name-mangled (__name) helpers in pyathena/ with single-underscore methods, and share the non-I/O parts that the sync and async copies repeated.
No behavior change, apart from one log message noted below. The only public addition is the TERMINAL_STATES class constant on two execution models.

Renames (async classes override the sync method with an async def of the same name, as _poll / _cancel / _get_query_execution already do):

Before After
__poll in BaseCursor, AioBaseCursor, SparkBaseCursor, AioSparkCursor _poll_until_terminal
SparkBaseCursor / AioSparkCursor __cancel_and_wait _cancel_and_wait
SparkBaseCursor / AioSparkCursor __start_calculation _start_calculation_execution
SparkBaseCursor.__wait_for_start _wait_for_calculation_start
SparkBaseCursor.__terminate_session(session_id) _terminate_session_by_id
AthenaResultSet.__get_query_results / __fetch _get_query_results; __fetch inlined
AthenaAioResultSet.__async_get_query_results / __async_fetch _async_get_query_results; __async_fetch inlined
AthenaArrowResultSet / AthenaPandasResultSet __s3_file_system _create_s3_file_system, the name AthenaS3FSResultSet already uses

Shared non-I/O parts:

  • New public class constants AthenaQueryExecution.TERMINAL_STATES and AthenaCalculationExecutionStatus.TERMINAL_STATES (inherited by AthenaCalculationExecution) replace the state lists repeated in the four polling loops.
  • AthenaResultSet._build_get_query_results_request holds the GetQueryResults checks and request for the sync and async result sets. The closed-result-set message stays per class, and the checks run in the same order as before.
  • AioSparkCursor._terminate_session now logs Failed to terminate session: <session ID>. like the sync cursor, instead of Failed to terminate session..

Each _poll keeps its current interrupt handling. The signatures of _execute, _poll, _cancel, _get_query_execution, _find_previous_query_id, and _terminate_session are unchanged (#734).
The broader sync / async duplication is tracked separately in #880.

WHY

Closes #879.
Subclasses cannot override mangled helpers and can reuse them only through the mangled name, so each class kept its own copy.
#853 and #871 will be rebased onto this instead of adding more mangled helpers.

TEST

Tested commit: f4cc848.

  • just lint: passed.
  • git grep -nE "def __[a-z0-9_]*[a-z0-9]\(" -- pyathena/: 16 → 0; no _ClassName__name access in pyathena/ or tests/.
  • AWS, before the shared-constant / request-builder / log changes (rename only): uv run --env-file .env pytest -n 1 tests/pyathena/test_cursor.py tests/pyathena/aio/test_cursor.py tests/pyathena/spark/ tests/pyathena/aio/spark/ tests/pyathena/arrow/test_cursor.py tests/pyathena/pandas/test_cursor.py tests/pyathena/polars/test_result_set.py tests/pyathena/s3fs/test_cursor.py tests/pyathena/aio/arrow/test_cursor.py tests/pyathena/aio/pandas/test_cursor.py: 582 passed, 3 skipped.
  • AWS, on f4cc848: uv run --env-file .env pytest -n 1 tests/pyathena/test_cursor.py tests/pyathena/aio/test_cursor.py: 182 passed.
  • Not run locally on f4cc848: the Spark, format-cursor, and SQLAlchemy suites; the Ready CI covers them.

🤖 Generated with Claude Code

Rename the 16 non-dunder `__name` helpers so subclasses can reuse and
override them, and inline the one-line fetch wrappers:

- `__poll` in the SQL and Spark cursors (sync and async) becomes
  `_poll_until_terminal`.
- Spark: `_cancel_and_wait`, `_start_calculation_execution`,
  `_wait_for_calculation_start`, `_terminate_session_by_id`.
- Result sets: `_get_query_results`, `_async_get_query_results`, and
  `_create_s3_file_system` in the Arrow and pandas result sets.

Share the non-I/O parts that the sync and async copies repeated: the
terminal states become `TERMINAL_STATES` on the execution models, and
`_build_get_query_results_request` holds the GetQueryResults checks and
request. The async Spark cursor now logs the session ID when terminating
a session fails, as the sync cursor does.

Closes #879

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/spark/common.py
raise OperationalError(*e.args) from e

def __poll(self, query_id: str) -> AthenaQueryExecution | AthenaCalculationExecution:
def _poll_until_terminal(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one: implementation behavior. Result: CLEAN.

Scope: git diff 7b77a536342b00694a9e76c4edf9b4811d47bca0..f4cc848b8502cb4292ca932b8341cf898728454d, all 9 files.

Covered:

  • Dispatch change. Mangled calls were bound to the defining class; the renamed ones dispatch on the instance. Traced every definition and caller of the new names in pyathena/, and every subclass of BaseCursor, AioBaseCursor, SparkBaseCursor, AthenaResultSet, and AthenaAioResultSet.
    • No sync method can reach an async override. AioBaseCursor overrides _poll. AioSparkCursor overrides _poll, _calculate, and _cancel, so the sync _poll, _calculate, and _cancel_and_wait never run on it.
    • _terminate_session_by_id has no override and stays sync during _start_session in every variant.
    • AsyncCursor and AsyncSparkCursor (thread-based) inherit only sync overrides.
  • Failure order. _get_query_results and _async_get_query_results still check, in order: query ID, then state, then closed. Each class keeps its own closed message. After close(), _fetch still raises NextToken is none or empty. first (result_set.py:728 clears the token).
  • Interrupt semantics. Each _poll is unchanged apart from the helper name: SQL returns the cancelled execution; Spark re-raises with __cause__.
  • Terminal states. TERMINAL_STATES holds the same states as the replaced lists; only the membership test changes, from a list to a tuple.
  • No mangled access left. No _ClassName__name or __name helper references remain in pyathena/, tests/, or docs/.
  • Tests. No new tests: behavior is unchanged, and the existing interrupt, cancel, and session-cleanup tests exercise these paths.

Out of scope: self.__dtypes in arrow/, pandas/, and polars/ converter.py are mangled attributes, not helper methods, so neither #879 nor this PR covers them.

Limitations:

  • The Spark and format-cursor AWS tests ran only on the rename-only tree, before TERMINAL_STATES, _build_get_query_results_request, and the log change.
  • On f4cc848, only test_cursor.py and aio/test_cursor.py ran. The Ready CI covers the rest.

Comment thread pyathena/model.py
STATE_SUCCEEDED: str = "SUCCEEDED"
STATE_FAILED: str = "FAILED"
STATE_CANCELLED: str = "CANCELLED"
TERMINAL_STATES: tuple[str, ...] = (STATE_SUCCEEDED, STATE_FAILED, STATE_CANCELLED)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two: claims, callers, and operational behavior. Result: FINDINGS in the PR description only; corrected. No code changes.

Scope: git diff 7b77a536342b00694a9e76c4edf9b4811d47bca0..f4cc848b8502cb4292ca932b8341cf898728454d, plus the PR body, commit message, and changed docstrings.

Claims checked:

  • "16 non-dunder name-mangled helpers", "16 → 0". git grep -nE "def __[a-z0-9_]*[a-z0-9]\(" <sha> -- pyathena/ returns 16 at the base and 0 at the head.
  • Async overrides "as _poll / _cancel / _get_query_execution already do". aio/common.py:90, :139, and :166 are async def with # type: ignore[override].
  • Signatures of _execute, _poll, _cancel, _get_query_execution, _find_previous_query_id, and _terminate_session unchanged (Backwards incompatible changes in 3.35 breaks dbt projects #734). The diff adds or removes no def line for any of them.
  • _create_s3_file_system "the name AthenaS3FSResultSet already uses". See s3fs/result_set.py:107.
  • _terminate_session_by_id docstring: "session startup calls this synchronously in every cursor variant". AioSparkCursor also starts its session in SparkBaseCursor.__init__, which runs sync, and it does not override _terminate_session_by_id.
  • "Checks run in the same order as before". Query ID, then state, then closed. The request dict is built before the closed check, which does no I/O.

Corrections to the PR body:

  1. "Mangled helpers cannot be reused or overridden" overstated it: a subclass can still call self._Class__name. Reworded to "cannot override mangled helpers and can reuse them only through the mangled name".
  2. The body did not say that TERMINAL_STATES is a public addition. It now does: "The only public addition is the TERMINAL_STATES class constant", noting that AthenaCalculationExecution inherits it.

Existing and downstream callers:

  • The new single-underscore names become visible to subclasses.
  • GitHub code search of dbt-labs/dbt-adapters (dbt-athena), run on 2026-09-28, found no definitions or uses of _poll_until_terminal, _get_query_results, _async_get_query_results, _build_get_query_results_request, _cancel_and_wait, _start_calculation_execution, _wait_for_calculation_start, _terminate_session_by_id, or _create_s3_file_system.
  • The same batch found TERMINAL_STATES only as dbt-spark's and dbt-bigquery's own constants, which shows the search was working.
  • Limitation: code search covers only the default branch. Other downstream subclasses were not surveyed.

AWS operator: the same APIs are called the same number of times. Polling cadence, retries, and interrupt and cancel paths are unchanged, and no quota or cost change is claimed.

Documentation: no docs page references the removed names; git grep over docs/ finds none.

Evidence:

  • The AWS results in the PR body are local runs and are labelled by tree.
  • The rename-only run used an uncommitted working tree, before the shared changes.
  • On f4cc848 only the SQL cursor and aio cursor tests ran.
  • The Spark, format-cursor, and SQLAlchemy suites on the published head come from the Ready CI.

Comment thread pyathena/result_set.py
return cast("Connection[Any]", self._connection)

def __get_query_results(
def _build_get_query_results_request(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review (relayed): Codex. Result: CLEAN.

  • Reviewer: Codex CLI 0.157.1 (ChatGPT account); model gpt-6-astra, reasoning effort high, sandbox read-only; session 01a0e892-a299-7a83-a46d-bc5d6aab91fa.
  • Scope: git diff 7b77a536342b00694a9e76c4edf9b4811d47bca0..f4cc848b8502cb4292ca932b8341cf898728454d, reviewed in a detached worktree at f4cc848.
  • Constraints: review only, with no edits, builds, tests, GitHub access, or network. The prompt contained the diff and the stated intent, but not the PR number, PR text, commit message, or earlier self-review conclusions.
  • After the review, the snapshot and the PR worktree were both clean and at f4cc848.
  • This is a static review; no validation ran.

Reviewer output, verbatim:

Covered all nine changed files and their direct callers, including:

  • Sync, asyncio, and thread-pool cursor dispatch; AthenaAioDictResultSet MRO; pandas/arrow/polars/s3fs result construction and fetching.
  • GetQueryResults validation order, exception messages, wrapping, pagination, and API fallback.
  • Query/Spark polling, interrupts, cancellation, startup cleanup, session termination, and exception chaining.
  • Name collisions across pyathena/ and tests/, changed docstrings, terminal-state constants, and all six protected signatures.

CLEAN

No actionable regression found. No traced sync call newly reaches an async override. Spark startup cleanup still uses the synchronous _terminate_session_by_id; async result fetching retains its separate awaited path. Validation precedence and protected signatures remain unchanged. All non-dunder name-mangled helper definitions were removed.

No separate pre-existing defect is reported. Existing runtime imports in the renamed filesystem factories are unchanged.

Static review only: no builds, tests, writes, or network access. HEAD remains f4cc848b8502cb4292ca932b8341cf898728454d, and the working tree is clean.

@laughingman7743
laughingman7743 marked this pull request as ready for review September 28, 2026 15:29
@laughingman7743
laughingman7743 merged commit a86a180 into master Sep 28, 2026
12 checks passed
@laughingman7743
laughingman7743 deleted the refactor/879-single-underscore-helpers branch September 28, 2026 15:41
laughingman7743 added a commit that referenced this pull request Sep 29, 2026
…tup fails

(cherry picked from commit 5766a13)

The session termination helper uses the single-underscore name _terminate_session_by_id that master adopted in #881.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace name-mangled helper methods with shared single-underscore helpers

1 participant