Repository navigation
Backport bug fixes to 3.x for v3.38.0 - #1104
Conversation
(cherry picked from commit ad1232b) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…imary (cherry picked from commit 0c26c7e) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 37999c3) Conflict resolution: pyathena/spark/common.py conflicted in the _exists_session() docstring, which master gained with the metadata work of #803 (not backported to 3.x). #831 only extended that docstring's Raises entry, so 3.x keeps its existing undocumented method there. The _wait_for_idle_session() fix and its new docstring apply unchanged, as do the tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ermination fails (cherry picked from commit 2a107ad) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tup fails (cherry picked from commit 5766a13) Conflict resolution: pyathena/spark/common.py conflicted in _terminate_session(), whose docstring and dict[str, Any] request annotation master gained with the metadata work of #803 (not backported to 3.x). Only #832's own changes are applied there: _terminate_session() delegates to the new __terminate_session(), which takes the session ID, keeps 3.x's unannotated request dict, and logs the session ID. The __init__ reordering, the _start_session() cleanup, the AsyncSparkCursor executor setup, and the tests apply unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d result sets (cherry picked from commit 5415c05) Conflict resolution: pyathena/aio/common.py conflicted because 3.x still has the pre-#883 WithAsyncFetch, which also subclasses CursorIterator and carries default synchronous fetch methods, and its imports and docstring differ. Only #899's own changes are applied: NoReturn is added to 3.x's typing import, the class docstring gains "Synchronous iteration raises ``TypeError``.", and __iter__() is added after the synchronous fetch methods. Every 3.x asyncio SQL cursor overrides the fetch methods with coroutines, so a synchronous for loop hung there too. pyathena/aio/result_set.py and the tests apply unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 8679f04) 3.x resolution: only the decimal256 part of #900 is applied. 3.x allows pyarrow>=10.0.0, but pyarrow.types.Type_DECIMAL32 and Type_DECIMAL64 first appear in pyarrow 19.0.0; referencing them would raise AttributeError in get_athena_type() for every type that reaches the decimal check on older pyarrow. The check therefore lists Type_DECIMAL128 and Type_DECIMAL256 (replacing the Decimal256Type class that never matched a type id), and the decimal32/decimal64 test cases are dropped. The decimal128 and decimal256 cases are kept unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 7be04bd) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r block size (cherry picked from commit f8e6b86) Conflict resolution: pyathena/filesystem/s3.py conflicted in the S3File.__init__() docstring, which master gained with #919 (not backported to 3.x). #929 only reworded that docstring's append-mode sentence, so 3.x keeps its undocumented __init__(). The append, upload, and discard fixes and the tests apply unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rt part (cherry picked from commit e0e85da) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit c9d6875) Conflict resolution: tests/pyathena/filesystem/test_s3_async.py conflicted in its imports. Master already imported unittest.mock there from #948 (not backported to 3.x), next to which #955 added SimpleNamespace. #955's new test uses both, so both imports are added. The runtime changes and the other tests apply unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| _UTC_OFFSET_PATTERN: re.Pattern[str] = re.compile(r"([+-])(\d{2}):(\d{2})") | ||
|
|
||
|
|
||
| def _parse_utc_offset(value: str) -> timezone | None: |
There was a problem hiding this comment.
Self-review round 1: implementation behavior: CLEAN (no actionable findings)
Scope: git diff f09cf4269052b7dfa4157f10a5f37a5cbfa6148a..5bf0490e450e56ca9a1be3840e94d86234beae4f, the 20 commits, 36 files.
Covered:
- Faithfulness. Each commit's added and removed lines were compared with its master PR's first-parent diff, using a line-multiset diff.
- Clean picks match exactly: Find an existing table in to_sql whatever the name's case #800, Assert cache hits in the test work group instead of primary #817, Shut down the AsyncSparkCursor executor when session termination fails #830, Fix stale S3FileSystem listing and object caches #928, Merge a short trailing block into the previous multipart part #947, Do not convert ArrowCursor fallback values twice #1023, Keep NULL rows of single-column CSV results in ArrowCursor #1031, Read multi-line CSV values that cross a block in ArrowCursor #1045.
- Every other difference is one of the resolutions recorded in the commit messages:
- docstrings from Answer throttled metadata requests from Glue in the cursor #803/Document every public API in pyathena/ and check docstrings with ruff #919 left out (Stop Spark session readiness polling on failure states #831, Keep the existing object when appending within a larger block size #929, Return the whole result from as_pandas() when auto chunking applies #950, Skip the result cache for qmark queries with parameters #959);
- 3.x's unannotated
request(Terminate a newly started Spark session when cursor setup fails #832); - 3.x's typing import (Reject synchronous iteration of the asyncio cursors and result sets #899);
- the
mockimport for Keep multipart copy parts within the S3 part size limits #955's test; timezone.utcinstead ofdatetime.UTC(Skip the result cache for qmark queries with parameters #959);- the new 3.x test file (Stop a closed PolarsDataFrameIterator for every reader kind #1029);
- the partial ports Map Arrow decimal32/64/256 to Athena decimal #900, Reflect the field and value types of top-level STRUCT and MAP columns #995, Convert time zone values to aware times and empty TIME/JSON text to NULL #1033 and Use the C engine for Pandas DDL text results #1075.
- Behavior and failure paths. Each item below was traced into its 3.x callers.
- Reject synchronous iteration of the asyncio cursors and result sets #899: every 3.x asyncio SQL cursor (
AioCursor,AioDictCursor,AioArrowCursor,AioPandasCursor,AioPolarsCursor,AioS3FSCursor) overridesfetchone()with a coroutine. Before this change,CursorIterator.__next__got a coroutine that is neverNone, so the loop never ended. - Skip the result cache for qmark queries with parameters #959: 3.x's
_build_start_query_execution_requestsetsExecutionParametersonly for qmark parameters (common.py:263), sorequest.get("ExecutionParameters")guards the same case as master. - Terminate a newly started Spark session when cursor setup fails #832:
_start_session()calls the name-mangled__terminate_session()synchronously, soAioSparkCursorandAsyncSparkCursor, which override_terminate_session()as a coroutine or future, still release the session.
- Reject synchronous iteration of the asyncio cursors and result sets #899: every 3.x asyncio SQL cursor (
- Data and resource boundaries.
- Convert time zone values to aware times and empty TIME/JSON text to NULL #1033:
_parse_utc_offset(tz) or gettz(tz)keeps zone names. - Map Arrow decimal32/64/256 to Athena decimal #900:
Type_DECIMAL128andType_DECIMAL256exist on pyarrow>=10. - Return the whole result from as_pandas() when auto chunking applies #950: the
CategoricalDtypehandling also works on pandas 2.2.3 (3.x lock).
- Convert time zone values to aware times and empty TIME/JSON text to NULL #1033:
- Tests. The live and offline runs listed in the PR body pass on Python 3.10.16.
tests/pyathena/spark/test_async_cursor.py's new offline tests also pass (4).
Recorded limitations (3.x consequences of the agreed scope, not defects of this PR) are in the threads below.
| result_set = cast(AthenaResultSet, self.result_set) | ||
| return result_set.fetchall() | ||
|
|
||
| def __iter__(self) -> NoReturn: |
There was a problem hiding this comment.
Round 1 note (#899):
- 3.x's
WithAsyncFetchstill carries the default synchronous fetch methods, which master removed in Share the pure cursor-base logic between the sync and asyncio cursors #883. - A user subclass that keeps them and relied on synchronous iteration now gets this
TypeError. - No shipped 3.x cursor does that; all of them override the fetch methods with coroutines.
- Recorded rather than changed: Reject synchronous iteration of the asyncio cursors and result sets #899's contract is that asyncio cursors are not synchronously iterable.
| except (TypeError, ValueError): | ||
| util.warn(f"Did not recognize type '{type_}'") | ||
| return types.NullType() | ||
| if not _nested and name in ("map", "row", "struct") and length: |
There was a problem hiding this comment.
Round 1 note (#995):
visit_nullis kept, soNullTypeis not rejected in DDL on 3.x.- A reflected MAP or STRUCT with an unrecognized nested type renders
NULLinCREATE TABLE, and Athena rejects the statement. Master raisesCompileErrorat compile time instead. - Before this change, the whole top-level column reflected as
String. - 3.x's
AthenaArray(NullType)already behaved this way. - This is the agreed partial port: removing
visit_nullwithout Split SQLAlchemy type rendering into Hive DDL and Trino DML compilers #961 would breakCAST(x AS NULL).
(cherry picked from commit f3ef301) Conflict resolution: 3.x's BaseCursor._execute() and AioBaseCursor._execute() still build the request inline, without the docstring and _build_execute_request() that master gained with #883 and the _start_execution() from #853 (neither backported to 3.x). #959's own change is applied to 3.x's code: the _find_previous_query_id() lookup runs only when the request has no ExecutionParameters, with the same comment. The docstring sentence it added to _execute() has no 3.x docstring to go into; the ExecuteOptions docstring line applies unchanged. docs/usage.md gains the same sentence after 3.x's (older) cache paragraph. In tests/pyathena/aio/test_cursor.py only #959's test_execute_qmark_parameters_skip_cache is added, without the #853 interrupt tests around it. In tests/pyathena/test_cursor.py, test_cache_size_with_qmark_parameters uses datetime.now(timezone.utc) as the rest of 3.x's file does, because master's datetime.UTC comes from #884 and needs Python 3.11; the tests otherwise apply unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nking applies (cherry picked from commit 700fa30) Conflict resolution: pyathena/pandas/result_set.py conflicted in the AthenaPandasResultSet.as_pandas() docstring, which master gained with #919 (not backported to 3.x); 3.x keeps its undocumented method, and the code change and the other docstring updates apply unchanged. docs/aio.md conflicted because master's paragraph on the synchronous convenience methods was rewritten by #940 (not backported); #950's sentence is added after 3.x's paragraph. docs/pandas.md and the tests apply unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and MAP columns (cherry picked from commit 6be7de3) 3.x resolution: the reflection fix for #857 is applied unchanged (pyathena/sqlalchemy/base.py and its docstring). The part of #995 that removed AthenaTypeCompiler.visit_null, so that NullType raises CompileError in DDL, is left out by maintainer decision. It relies on the DDL/DML type compiler split of #961 (master only): 3.x still renders CAST types through AthenaTypeCompiler, so without visit_null CAST(x AS NULL) would raise. pyathena/sqlalchemy/compiler.py is therefore unchanged, and so are the DDL-rejection documentation and tests: - docs/sqlalchemy.md: the "DDL and CAST types" sentence on NullType is dropped together with the #961 section it belongs to; the paragraphs on reflected STRUCT/ROW and MAP columns apply unchanged. - tests/pyathena/sqlalchemy/test_compiler.py: only test_null_type_cast_is_unchanged is added, after test_ddl_and_cast_types as on master; test_null_type_is_rejected_in_ddl is dropped. - tests/pyathena/sqlalchemy/test_base.py: the new dialect tests go into 3.x's TestAthenaDialect (from #839) after its existing tests, without the master-only metadata tests around them, and need the AthenaDialect import. test_unrecognized_field_type_reflects_null_type_and_blocks_ddl keeps its reflection assertions without the CompileError check and is named test_unrecognized_field_type_reflects_null_type. The added assertion in test_columns_from_information_schema is dropped, because that test comes from #777 (not backported). The TestSQLAlchemyAthena changes apply unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 52ea5a7) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r kind (cherry picked from commit 9a373fd) Conflict resolution: tests/pyathena/polars/test_result_set.py does not exist on 3.x; master created it with #828 (not backported). The file is created with master's header, the imports #1029's test needs, and #1029's TestPolarsDataFrameIterator; #828's chunk-reader tests are not included. pyathena/polars/result_set.py applies unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ursor (cherry picked from commit 3323784) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…H TIME ZONE values (cherry picked from commit ec5323e) 3.x resolution: only the #854 fix is ported from #1033, by maintainer decision: a TIMESTAMP WITH TIME ZONE value with a numeric UTC offset (+05:30, -08:00) converts to an aware datetime with a fixed-offset time zone instead of a naive one. #1033's other changes are behavior changes for 4.0.0 and build on #1005 and #1010 (not backported), so they are left out: TIME WITH TIME ZONE conversion, empty TIME, JSON and TIMESTAMP WITH TIME ZONE text as NULL, the time zone converters of the pandas, Arrow and Polars cursors, the type-signature and JSON element changes in parser.py, the docs, and the live CONVERTED_VALUES tests. Applied from #1033: - pyathena/converter.py: _UTC_OFFSET_PATTERN and _parse_utc_offset() as on master, and _to_datetime_with_tz() uses _parse_utc_offset(tz) or gettz(tz). It keeps 3.x's strptime() parsing (master parses with _parse_datetime() from #818, not backported) and its None check. - tests/pyathena/test_converter.py: test_to_datetime_with_tz_offsets_and_zone_names with the imports it needs. The "" case (empty text as NULL) and the case without fractional seconds (needs #818's parser) are dropped; Athena renders these values with milliseconds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wCursor (cherry picked from commit 50b8718) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit bf6f299) Conflict resolution: the _get_csv_engine() fix in pyathena/pandas/result_set.py applies unchanged. The other files conflicted with text from PRs that are not backported to 3.x: - docs/pandas.md: master's paragraph on when engine="pyarrow" applies was added by #902 and extended by #1033 and #1057; 3.x has none of that text. #1075 added a ".txt" condition to its first sentence, which has no 3.x counterpart, and a sentence on DDL results; only that sentence is added, after the read_csv options. - tests/pyathena/pandas/test_cursor.py: only #1075's result_set._query_execution = None is added; the _metadata and _kwargs lines around it come from other PRs. - tests/pyathena/pandas/test_result_set.py: 3.x's file (from #950) has no TestAthenaPandasResultSet, so the class is created with #1075's test_get_csv_engine_result_format and the imports it needs. test_read_csv_ddl_preserves_numeric_looking_names is dropped: it stubs result_set._fs.open() and asserts that the stream is closed, but 3.x's _read_csv() passes the S3 path to pandas.read_csv(), and reading through the result set's filesystem comes from #1001 (not backported). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5bf0490 to
683658f
Compare
| if type_.id == types.Type_TIMESTAMP: # 18 | ||
| return "timestamp", 3, 0 | ||
| if type_.id in [types.Type_DECIMAL128, types.Decimal256Type]: # 23, 24 | ||
| if type_.id in [types.Type_DECIMAL128, types.Type_DECIMAL256]: # 23, 24 |
There was a problem hiding this comment.
Self-review round 2: claims, callers and operations: FINDINGS, repaired (wording in commit messages and the PR body only; no code change)
Scope: git diff f09cf4269052b7dfa4157f10a5f37a5cbfa6148a..683658f24dff765e0cc6df1bffcfebdd9f7797a8, the PR body, and the 20 commit messages.
Claims checked against evidence:
- pyarrow (Map Arrow decimal32/64/256 to Athena decimal #900): measured with
uv run --with pyarrow==X.pyarrow.lib.Type_DECIMAL32is absent on 10.0.1 and 18.1.0 and present on 19.0.0.Type_DECIMAL128= 23 andType_DECIMAL256= 24 exist on all three.- On pyarrow 10.0.1 with this branch,
get_athena_type()returns('decimal', 38, 4),('decimal', 40, 5),('array', 0, 0)and('varchar', …)for decimal128, decimal256, list and string.
- Provenance in the resolution notes: checked with
git log --first-parent -Son master.- Confirmed:
- Answer throttled metadata requests from Glue in the cursor #803:
_exists_sessiondocstring,_terminate_sessionannotation - Document every public API in pyathena/ and check docstrings with ruff #919:
S3File.__init__andas_pandasdocstrings - Accept max_workers in S3FileSystem.open() and pipe_file() #948:
unittest.mockimport - Require polars>=1.39.0 for chunked PolarsCursor reads #828: polars
test_result_set.py - Correct user guide claims that contradict the implementation #940: aio paragraph
- Keep result-set filesystems out of the fsspec instance cache and read pandas results through one filesystem #1001:
_fs.openin_read_csv - Support SQLAlchemy datetime literals and microsecond timestamps #818:
_parse_datetime
- Answer throttled metadata requests from Glue in the cursor #803:
- Corrected (three commit messages, rewritten with
--force-with-lease; tree unchanged against 5bf0490):- Skip the result cache for qmark queries with parameters #959:
_execute()'s docstring and_build_execute_request()come from Share the pure cursor-base logic between the sync and asyncio cursors #883 and_start_execution()from Re-raise and stop interrupted queries in the SQL cursors, sharing the Spark handling #853. The message wrongly also named Replace name-mangled helpers with single-underscore methods #881/Document every public API in pyathena/ and check docstrings with ruff #919. - Reflect the field and value types of top-level STRUCT and MAP columns #995:
test_columns_from_information_schemacomes from Fix metadata reflection throttling and reuse listed metadata #777, not Read the dialect's own queries through an API cursor and resolve federated absence from information_schema #798. - Use the C engine for Pandas DDL text results #1075: master's PyArrow-engine paragraph was added by Fix PandasCursor performance tuning examples #902 (extended by Convert time zone values to aware times and empty TIME/JSON text to NULL #1033/Read CSV results for the pandas PyArrow engine with pyarrow.csv #1057), not Read CSV results for the pandas PyArrow engine with pyarrow.csv #1057.
- Skip the result cache for qmark queries with parameters #959:
- The PR table row for Use the C engine for Pandas DDL text results #1075 was corrected the same way.
- Confirmed:
- Callers:
- Versions: no picked diff uses an API newer than 3.x's floors. Checked: Python 3.10 (no
datetime.UTC,Self,TaskGroup,Task.cancelling,@override), pandasCategoricalDtype(ordered=), polars, and thepyarrow.csv.ParseOptions(newlines_in_values=)/ignore_empty_linesoptions. - New imports: no new fsspec imports.
- Versions: no picked diff uses an API newer than 3.x's floors. Checked: Python 3.10 (no
- CI claim: 3.x
test.yamlpath filters select the pyathena, sqla and spark suites for this diff, since it touchespyathena/*.py,pyathena/sqlalchemy/**andpyathena/spark/**. They run once the PR is Ready. - Evidence:
- The local AWS and offline results in the PR body belong to tree 5bf0490, which equals the head.
- The Spark live suite and
just test sqla/sqla-asyncwere not run locally; that is stated in the PR body.
|
|
||
| The `as_pandas()`, `as_arrow()`, and `as_polars()` convenience methods operate on | ||
| already-loaded data and remain synchronous. | ||
| With a chunk size chosen by `auto_optimize_chunksize`, `as_pandas()` reads every remaining chunk. |
There was a problem hiding this comment.
Round 2 deferral (docs reader):
- 3.x's sentence above, that the convenience methods "operate on already-loaded data", was already inaccurate for chunked results before this PR. Return the whole result from as_pandas() when auto chunking applies #950's added sentence makes that visible.
- Master corrected the paragraph in Correct user guide claims that contradict the implementation #940, a docs-only PR that is not in this backport's scope.
- Left as is. The added sentence is accurate.
| for row in zip(*dict_rows.values(), strict=False) | ||
| ] | ||
| else: | ||
| processed_rows = list(zip(*dict_rows.values(), strict=False)) |
There was a problem hiding this comment.
Independent review (relayed): Codex CLI 0.160.0, model gpt-6.1-sol, codex exec -s read-only, effort high. Session 01a11134-c83c-7aa2-96a0-26d06ed51cb0. Static review only: no builds, tests, edits or network.
- Snapshot: detached worktree at head
683658f24dff765e0cc6df1bffcfebdd9f7797a8, basef09cf4269052b7dfa4157f10a5f37a5cbfa6148a. The prompt contained the diff range, the mapping from commit to master merge commit, the 3.x dependency floors and the required scope. It contained no PR text, commit messages or prior findings. - The snapshot and the PR worktree were unchanged afterwards.
- Coverage reported:
- all 20 commits against their master patches, including the three partial ports;
- sync, threaded and asyncio cursors; pandas, Arrow, Polars and S3FS result sets;
- Spark lifecycle;
- S3 caches, append, multipart upload/copy and failure paths;
- SQLAlchemy reflection and compiler, dependency bounds, tests and docs.
- Verdict:
FINDINGS, with two P2 findings: one introduced (this thread) and one pre-existing (next thread).
Finding (introduced, P2):
- With managed result storage (no output location), the GetQueryResults fallback no longer applies the result set's converter mapping in the fetch methods.
- So a custom mapping such as
DefaultArrowTypeConverter().set("varchar", str.upper)stops applying:fetchall()returns("hello",)instead of("HELLO",). The same holds on master since Do not convert ArrowCursor fallback values twice #1023.
Author verification: confirmed, deferred.
- Before Do not convert ArrowCursor fallback values twice #1023,
_fetch()appliedself.convertersto values that_fetch_all_rows()had already converted withDefaultTypeConverter. - With the default Arrow converter, that raised
TypeErrorfor TIME, VARBINARY and JSON objects and decoded JSON twice (ArrowCursor converts GetQueryResults fallback values twice in the fetch methods #1021, measured since v3.28.0). A custom mapping received already-converted values, not the strings the Converter contract specifies. as_arrow()never applied custom mappings on this path, before or after.- Master removed the double path in Read managed query results as a CSV result file in the pandas, Arrow, and Polars cursors #1061, which reads managed results as a CSV result file through the cursor's converter. That is a 4.0.0 behavior change outside this backport.
- So 3.x keeps Do not convert ArrowCursor fallback values twice #1023 as ported: the default conversions work, and custom mappings on the managed-storage fetch path are a known limitation, to be release-noted together with Do not convert ArrowCursor fallback values twice #1023.
| self._sync_fs.max_workers, | ||
| block_size, | ||
| ) | ||
| ranges = self._sync_fs._get_copy_ranges(size1, block_size) |
There was a problem hiding this comment.
Finding (pre-existing, P2), relayed from the independent review:
- Location:
pyathena/filesystem/s3_async.py:276,AioS3FileSystem._copy_object_with_multipart_upload. - After
CreateMultipartUpload, anUploadPartCopyerror, a cancellation, or a completion failure propagates withoutAbortMultipartUpload, so the uploaded parts are left behind.
Author verification: pre-existing at the review base, not introduced.
- Keep multipart copy parts within the S3 part size limits #955 only changed the part ranges here.
- Master fixed it in Cancelling an async multipart copy leaves its multipart upload behind #1046 / Abort a cancelled async multipart copy after its running parts #1056, which builds on Copy metadata, tags and annotations in multipart copies #1036's copy rework (botocore>=1.43.31). That work is excluded from 3.x along with the other filesystem chain, by maintainer decision. Deferred.
| def as_pandas(self) -> PandasDataFrameIterator | DataFrame: | ||
| if self._chunksize is None: | ||
| return next(self._df_iter) | ||
| return self._df_iter.as_pandas() |
There was a problem hiding this comment.
Second independent review (relayed, requested by the maintainer): Claude Code CLI, model claude-fable-5-1, max profile (first-party Max auth verified), effort high, --permission-mode plan.
- Tools: read and git only (
Read/Grep/Glob,git show/diff/log/grep/blame). Session40ff7b56-3d85-4a99-88a5-7cebd4deb23d. - Static review on a detached snapshot at
683658f24dff765e0cc6df1bffcfebdd9f7797a8(basef09cf4269052b7dfa4157f10a5f37a5cbfa6148a). The snapshot was unchanged afterwards. - The prompt matched the Codex one, plus a compatibility audit:
- breaking changes;
- every user-visible behavior change, classified (a) fix of a wrong or failing result, or (b) change to previously correct, working behavior.
Coverage reported:
- all 20 commits against master, hunk by hunk;
- cursor bases and the cache;
- aio iteration, including
pyathena/aio/sqlalchemy, which awaitsfetchall()and never iterates; - Spark lifecycle;
- the Arrow, pandas and Polars result sets;
- the converters, with
gettz("+05:30") is Noneconfirmed on dateutil 2.9; - SQLAlchemy reflection traces;
- the filesystem cache, append, multipart and copy paths, against the fsspec 2024.12
flush()contract; - the tests (each traced to fail without its fix);
- the docs.
Result:
- Correctness, faithfulness, tests and docs:
CLEAN. No defects introduced. - Breaking changes: none. No public name removed or renamed; no signature, default, return shape or dependency floor changed. New exceptions occur only on paths that used to hang, loop forever or return wrong data.
Category (b) behavior changes it listed:
- Skip the result cache for qmark queries with parameters #959: a qmark query with the same parameters is no longer served from the client cache. This is documented in
docs/usage.mdandoptions.py. - Merge a short trailing block into the previous multipart part #947 / Keep multipart copy parts within the S3 part size limits #955: part boundaries change when the tail is short, so multipart ETags of identical content can differ (for example a
-3suffix becomes-2). - Return the whole result from as_pandas() when auto chunking applies #950:
PandasDataFrameIterator.as_pandas()keeps the chunks' index. With an explicitchunksizeandindex_col, the index is theindex_colvalues instead ofRangeIndex(0..n-1). - Return the whole result from as_pandas() when auto chunking applies #950: with
chunksize=None, a secondas_pandas()call (or one afterfetchone()) returns an empty DataFrame instead of raising a bareStopIteration. - Do not convert ArrowCursor fallback values twice #1023: managed-storage ArrowCursor results no longer pass through a custom converter (already recorded in the thread on
arrow/result_set.py). - Read multi-line CSV values that cross a block in ArrowCursor #1045:
newlines_in_values=Truecan slow multi-threaded Arrow CSV parsing. This is the same trade-off as on master, measured there at about 0.03–0.04 s per 250 MiB.
Pre-existing 3.x defects it noted (not introduced; fixed on master by excluded PRs):
_ls_from_cachevs tuple listing keys (Answer info() and exists() from cached parent listings and fix bucket lookups #1006);CreateMultipartUploadreceiving non-input fields (Per-file request parameters are not applied consistently to multipart upload requests #946 / Route S3 request parameters to the operations that accept them #1000);commit()dropping a failed upload (Keep a multipart upload whose abort fails so that discard() retries it #1047);- no 10,000-part limit (Keep multipart uploads within the 10,000-part limit #968);
- top-level
varchar(x)raising inget_columns; - an unreachable base
__anext__withoutawait.
Minor docs note: docs/aio.md:192-194, already recorded in round 2.
Author verification and actions:
- The Return the whole result from as_pandas() when auto chunking applies #950 second-call behavior was checked against the code: 3.x used
next(self._df_iter), andPandasDataFrameIterator.as_pandas()returnspd.DataFrame()when nothing remains. - All category (b) items were added to the release notes in the PR body.
- No code change: each item is either the documented contract of its fix or the same behavior as master.
WHAT
This PR backports bug fixes from master to the
3.xmaintenance branch for v3.38.0. It includes the fixes for the externally reported issues #854 and #857.Each master PR is cherry-picked with
-xin master merge order, one commit per PR. When a conflict came from a master PR that is not backported, only the backported PR's own changes are applied. Each commit message records how its conflict was resolved.to_sqlfinds an existing table whatever the case of its name (#799)ENV.work_groupinstead ofprimary(#816)TERMINATED,DEGRADED, orFAILED(#792)AsyncSparkCursor.close()shuts down its executor when session termination fails (#795)TypeErrorinstead of looping forever (#898)WithAsyncFetchdecimal256maps to Athenadecimalinstead ofstring(#888)write()no longer leaves a sub-5 MiB non-last part, which made the upload fail withEntityTooSmall(#942)qmarkqueries with parameters (#941)_execute()as_pandas()returns the whole result whenauto_optimize_chunksizechose a chunk size (#924)AthenaMap/AthenaStruct(#857)visit_nullkept (see below)PolarsDataFrameIteratorstops for every reader kind (#1019).txtresults withengine="pyarrow"(#1073)Ported in part, by maintainer decision:
pyarrow>=10.0.0, butType_DECIMAL32/Type_DECIMAL64first appear in pyarrow 19.0.0.AttributeErroringet_athena_type()on older pyarrow.Type_DECIMAL256replaces theDecimal256Typeclass, which never matched a type id.visit_nullremoval, which makesNullTyperaiseCompileErrorin DDL, is left out.AthenaTypeCompiler, so removingvisit_nullwould makeCAST(x AS NULL)raise.test_null_type_cast_is_unchangedis ported and guards this._parse_utc_offset()and its use in_to_datetime_with_tz()are ported, together with the converter test.strptime()parsing (Support SQLAlchemy datetime literals and microsecond timestamps #818 is not backported).Not backported:
WHY
master is 4.0.0 development. Many bugs fixed there since v3.36.0 also exist on 3.x.
v3.38.0 ships the fixes that are safe for the 3.x line, without its breaking changes. It includes the fixes for the issues @aminghadersohi reported: #854 (TIMESTAMP WITH TIME ZONE with a numeric offset came back naive) and #857 (top-level MAP columns reflected as
String).Release notes for v3.38.0 (in addition to the unreleased #839, #905, #906, #913 already on 3.x):
map<...>columns areAthenaMap(key_type, value_type)instead ofString. Top-levelstruct<...>/row(...)columns areAthenaStructwith their field types (Top-level map columns reflect as String #857, Reflect the field and value types of top-level STRUCT and MAP columns #995).+05:30convert to aware datetimes with a fixed offset (TIMESTAMP WITH TIME ZONE values with a numeric UTC offset come back naive #854, Convert time zone values to aware times and empty TIME/JSON text to NULL #1033).forloop over an asyncio cursor or result set raisesTypeErrorinstead of never ending (Reject synchronous iteration of the asyncio cursors and result sets #899).cache_size/cache_expiration_timehave no effect onqmarkqueries with parameters, because Athena does not return the parameters of earlier executions (Skip the result cache for qmark queries with parameters #959). A repeated qmark query with the same parameters is therefore also executed again.as_pandas()returns the whole result whenauto_optimize_chunksizechose a chunk size; it used to return only the first chunk (Return the whole result from as_pandas() when auto chunking applies #950).chunksize, a secondas_pandas()call, or one after the rows were fetched, returns an empty DataFrame instead of raisingStopIteration(Return the whole result from as_pandas() when auto chunking applies #950).PandasDataFrameIterator.as_pandas(), as returned byiter_chunks()or byas_pandas()with an explicitchunksize, keeps the chunks' index and categorical dtypes. Withindex_col, the joined DataFrame is indexed by that column instead of a newRangeIndex(Return the whole result from as_pandas() when auto chunking applies #950).EntityTooSmall(Merge a short trailing block into the previous multipart part #947);as_arrow()holds and no longer convert them a second time. TIME, VARBINARY and JSON values used to raiseTypeError(Do not convert ArrowCursor fallback values twice #1023). A custom Arrow converter mapping is therefore not applied on this path, the same as foras_arrow().decimal256(Map Arrow decimal32/64/256 to Athena decimal #900), NULL rows (Keep NULL rows of single-column CSV results in ArrowCursor #1031) and multi-line CSV values (Read multi-line CSV values that cross a block in ArrowCursor #1045;newlines_in_values=Truecost about 0.03–0.04 s per 250 MiB when measured on master), the Polars iterator close (Stop a closed PolarsDataFrameIterator for every reader kind #1029), and PandasCursor DDL results withengine="pyarrow"(Use the C engine for Pandas DDL text results #1075).TEST
All runs were at 5bf0490. The branch head 683658f has the same tree; only three commit messages changed afterwards.
just lint(ruff check, ruff format --check, mypy): passed.markdownlint-cli2on the changed docs: 0 errors.--noconftest: 342 passed. The run covered:tests/pyathena/test_converter.pyaio/test_common.pyandaio/test_result_set.pyspark/test_common.pyarrow/test_util.pypandas/test_result_set.pyandpolars/test_result_set.pysqlalchemy/test_compiler.pyandsqlalchemy/test_base.py::TestAthenaDialect-n 8: 1178 passed.tests/pyathena/test_cursor.pyandaio/test_cursor.py,tests/pyathena/pandas/,arrow/,polars/,filesystem/,sqlalchemy/test_base.py,test_converter.py, the aio result-set and common tests, andspark/test_common.py.PandasDataFrameIterator.as_pandas()(Return the whole result from as_pandas() when auto chunking applies #950) emits pandas'FutureWarningabout concatenating all-NA chunks intest_as_pandas_matches_whole_read[category_null_chunk]. Master does not show it because it requires pandas 3; the test passes.tests/pyathena/suite, andjust test sqla/sqla-async. The PR's AWS jobs run them on the newest Python once the PR is Ready, and the release gate runs every Python version for the tag.🤖 Generated with Claude Code