Repository navigation
Build the pandas result-set tests through the constructor - #1268
Conversation
The json-column CSV tests, the Parquet NULL dtype test, the footer retry-stop test, and the UNLOAD chunk engine test read through _local_result_set() instead of a result set made with __new__ and private attributes, and _read_csv_result(), _unload_result_set(), and _read_unload_result() are removed. The footer reads now throttle through the filesystem's open() instead of a patched _read_parquet_footer(). A new test reads the CSV result file through PyAthena's real S3FileSystem over an in-memory S3 object, so that block_size and cache_type show in the ranges requested from S3. Refs #1257 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Parquet chunk test compares the whole description and the chunk index, the stopped footer reads raise the ThrottlingException of the stopped attempt, the chunk engine test reads with pyarrow as its control, and the in-memory S3 object answers only closed ranges of its own key. Refs #1257 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| chunks = list(result_set.iter_chunks()) | ||
| whole = pd.read_parquet(tmp_path / "bucket" / "unload") | ||
| assert result_set.description == [ | ||
| ("i", "bigint", None, None, 19, 0, "NULLABLE"), |
There was a problem hiding this comment.
Self-review round 1 (/code-review high at 35a9cd3) of 3dda0dcf9d6b94fe51f6df0b4aef004c6daeab2c..a2c6b6e0fddd1c2ca428619cf5a965b083d3da9e: FINDINGS, repaired in a2c6b6e.
- This test compared only the names and types of
descriptionand concatenated the chunks withignore_index=True; it now compares the whole description and the chunk index. Not advancing the chunk start index (pyathena/pandas/result_set.py:1587) fails it (offline). - Deferred: the moved test no longer asserts that
_read_parquet_schemais not called (no listing of the unload location for chunked reads); that was an internal call check, and the read results are the same either way.
| result_set._read_parquet_footers() | ||
| _local_result_set(fs, tables=[pa.table({"i": [1]}), pa.table({"i": [2]})], chunksize=1) | ||
| # The error of the stopped attempt is raised. | ||
| assert isinstance(error.value.__cause__, ClientError) |
There was a problem hiding this comment.
Round 1, repaired: the moved stopped-footer test and the chunk engine test had dropped their match= arguments. The footer test now checks that the OperationalError is caused by the ThrottlingException ClientError of the stopped attempt, and the engine test reads with engine="pyarrow" as its control, so its ProgrammingError comes from the engine; removing the chunk engine check (pyathena/pandas/result_set.py:1730-1731) fails [c] (offline).
| pytest.param({"cache_type": "none"}, False, id="cache_type"), | ||
| ], | ||
| ) | ||
| def test_csv_read_options_reach_file_system(self, kwargs, read_in_blocks): |
There was a problem hiding this comment.
Round 1, repaired: _InMemoryS3Object answered any key and any range form; it now asserts the bucket/result.csv key and answers only closed bytes=a-b ranges within the object. Deferred, with reasons: the botocore _make_api_call patch is the same seam as tests/pyathena/util.py::answered_requests; max(sizes) == block_size depends on the workers splitting a range into block-sized parts, now stated in a comment, and fails rather than passes if that changes; the duplicated constructor setup and the disk-backed CSVs of _read_local_csv are kept (171 offline tests run in about 10 s).
Round 2 (claims): the PR body's moved-test list, mutation results, and counts hold at this head; 171 tests in pandas/test_result_set.py pass offline (the engine test gained a pyarrow case); just lint passed.
The Parquet chunk test reads through a filesystem that cannot list, as credentials that may read the manifest and its files but not list the bucket. The S3 range test answers the client's public head_object and get_object instead of its private _make_api_call. Refs #1257 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| manifest without listing the unload location, which credentials may not allow. | ||
| """ | ||
|
|
||
| class UnlistableFileSystem(LocalBucketFileSystem): |
There was a problem hiding this comment.
Independent review (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14cbb-4e9f-7a23-9d95-1cd6fc4f55b1. Static review of 3dda0dcf..a2c6b6e0 in a clean detached snapshot, with the diff, the old tests at the base, and the test policy, and no PR text or author findings. It found that the nine json migrations keep their decorators, IDs, parameters, marks, and assertions, that the stopped-footer and engine tests stay sensitive to their mutations, and that the new S3 test observes the actual GetObject ranges. Result: FINDINGS (2), both repaired in 69ab1e9.
Finding 1 (P2): the move dropped the check that chunked UNLOAD reads take the metadata from the footers without listing the unload location; listing can fail for credentials that may read the manifest and files but not list the bucket. Repaired: the test reads through a filesystem whose ls and find raise PermissionError; replacing to_column_info(schema) with self._read_parquet_schema(engine) (pyathena/pandas/result_set.py:1732) now fails it (offline).
| with ( | ||
| patch.object( | ||
| connection.s3_client, | ||
| "head_object", |
There was a problem hiding this comment.
Independent review finding 2 (P2): the S3 range test patched botocore's private _make_api_call on a success path. Repaired: it patches the client's public head_object and get_object, which S3Core calls (pyathena/filesystem/s3_core.py:675,744), and keeps the real filesystem and cache. Dropping default_block_size or default_cache_type forwarding still fails [block_size] or [cache_type].
Self-review of the repair: round 1 CLEAN (171 offline tests pass; git diff -- pyathena/ empty; just lint passed); round 2: the PR body's claims hold, and the listing check is added to the description below.
| manifest without listing the unload location, which credentials may not allow. | ||
| """ | ||
|
|
||
| class UnlistableFileSystem(LocalBucketFileSystem): |
There was a problem hiding this comment.
Independent follow-up review (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14cc1-cdcd-7880-8e8a-a96205c68cdc. Static review of a2c6b6e0..69ab1e91 in a clean detached snapshot. Result: CLEAN. It found that reading the chunked metadata with _read_parquet_schema() reaches the denied find() and fails the test, that the unlistable filesystem still allows the manifest read, the direct opens, and size(), and that the S3 range test enters through the public head_object/get_object with both forwarding mutations failing.
WHAT
Build the offline tests of
tests/pyathena/pandas/test_result_set.pythroughAthenaPandasResultSet's constructor, and test that a result set'sblock_sizeandcache_typereach PyAthena'sS3FileSystem._read_csv_result(),_unload_result_set(), or_read_unload_result()onto_local_result_set(), and delete those three helpers. The nine json-column tests read through a thin_read_local_csv()wrapper; their CSV text that is not quoted as Athena writes it reads the same data through the constructor.test_iter_parquet_chunks_null_in_later_filechecks the wholedescriptioninstead of the private_metadata, and reads through a filesystem that cannot list, as credentials that may not list the bucket, instead of asserting that the private_read_parquet_schemais not called;test_read_parquet_footers_stoppedthrottles through the filesystem's publicopen()instead of patching_read_parquet_footer;test_as_pandas_parquet_chunks_enginepassesengine="c"to the constructor. Test IDs and parameters are unchanged.test_csv_read_options_reach_file_system[block_size, cache_type]. It reads a CSV result through PyAthena's realS3FileSystemover a boto3 S3 client whosehead_objectandget_objectserve the file from memory, and checks theGetObjectranges: withblock_size=1 MiBthe largest range is 1 MiB, and withcache_type="none"the file is never read whole._fetching_result_set()stays, as #1257 proposes._pyarrow_read_csv_kwargs()also still uses__new__, to build the options for the tests that call the private_read_csv_with_pyarrow()directly; #1257 does not name it.WHY
Closes #1257. Part of #1239: the bypass helpers tested a result set that
__init__never builds, so a change to the constructor's state could pass them and fail for callers, and no test checked thatblock_sizeandcache_typereach the filesystem (pyathena/pandas/result_set.py:869-870).TEST
Python 3.15.0, pandas 3.0.6, pyarrow 26.0.0. At the PR head, on 3dda0dc:
just format: no changes.just lint: passed.--noconftestand placeholder settings:pandas/test_result_set.py,pandas/test_converter.py, and the offlinepandas/test_util.pytests: 175 passed (171 inpandas/test_result_set.py).git diff -- pyathena/was empty after each:default_block_sizeordefault_cache_typeforwarding of_create_s3_file_system()fails[block_size]or[cache_type]._read_parquet_schema(), which lists the unload location, failstest_iter_parquet_chunks_null_in_later_file.parse_datesbranch of the json columns passes, as it did before the move: dates that pandas parses are kept by the later branch too.The tests are offline, so no live tests were run locally. CI after Ready, at 69ab1e9: Test run 39089607580 passed (
test / run (3.15), lint);test-sqlaandtest-sqla-asyncwere skipped by thechangespath filter. Merged locally onto 82754d2, after #1262 changedpyathena/filesystem/s3.pyand #1259 changedtests/pyathena/util.py:just lintpassed and the same offline tests passed (175).🤖 Generated with Claude Code