Repository navigation
Remove and rewrite unnecessary pandas cursor tests - #1256
Conversation
Drop the parameter cases that repeat the same read: the pyarrow engine case of UNLOAD, which "auto" already reads with pyarrow, the single-chunk chunksize of 1,000,000, and the unload and engine cases of DDL results. Remove the tests that set private chunking attributes or compare private predicates with constants, and the binary and pyarrow tests whose values other tests check. The binary stream close tests run offline through a cursor whose Athena and S3 clients and filesystem are local, and check that the files they opened are closed. The auto-optimized chunk sizes, the storage options, and the pyarrow engine fallbacks are checked through the values and chunk sizes that the cursor returns. Refs #1244 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AsyncPandasCursor and AioPandasCursor read through the same AthenaPandasResultSet as PandasCursor, so their copies of the NA, DDL, and many-rows tests are removed, and their parameter cases keep unload False and True. The forwarding of engine and na_values joins test_read_options. The binary NULL and empty tests stay, because each cursor chooses its own default converter. test_cancel_initial and test_invalid_arraysize of AioPandasCursor stay until #1245 adds their shared aio tests. Refs #1244 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The offline result-set tests build AthenaPandasResultSet over local result files, with the columns from a mocked GetQueryResults response, instead of asserting private predicates and the exact arguments of internal read_csv() and read_parquet() calls. They check the values, chunk sizes, and open options of the reads, and the pyarrow engine options move here from the cursor tests. The to_sql partition test compares its rows instead of asserting a non-empty tuple, and the unload converter's identity test is removed. Refs #1244 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… reads The DDL and small-file pyarrow tests compare the result with explicit names, values, and dtypes instead of a read through the C engine of the same result set. A whole binary read closes its stream after the read and after a failed read, the duplicate-name binary case keeps a boolean column's own converter, the pyarrow engine options include na_values, and the CSV test helper no longer records the internal read options. Refs #1244 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| ], | ||
| ids=["filesystem", "storage_options", "storage_options-none", "filesystem-none"], | ||
| ) | ||
| def test_unload_filesystem_options(self, tmp_path, option, value): |
There was a problem hiding this comment.
Self-review round 1 (implementation behavior, contracts, simplicity, regression coverage) of 713d57bde5248f9a77b763b44eb4c6945ceb4361..ecf6f4d064a81a4aa263da6741b956257f0c0e89, with /code-review (high) at 3eb4184. Mutations were run offline with --noconftest; git diff -- pyathena/ stayed empty.
Assembly repair before the review, in 3eb4184: the rewrite first made pandas open S3 at AWS_ENDPOINT_URL=http://127.0.0.1:9 and expected a failure, which took about 6 s per None case through pyarrow's retries and depended on how the installed pandas and pyarrow honor that variable. The test now observes the pandas.read_parquet() call: the S3 location with its scheme and the given option, without PyAthena's filesystem. Mutations: always using PyAthena's filesystem fails 4 cases; treating storage_options=None or filesystem=None as absent fails its case. The 4 cases take 1.2 s.
| return manifest | ||
|
|
||
|
|
||
| def _local_result_set(fs, types=(), data=None, tables=None, name="result.csv", **kwargs): |
There was a problem hiding this comment.
Round 1, helper: _local_result_set gets its columns from a mocked GetQueryResults response, its size from a mocked head_object, and its filesystem by patching pyathena.pandas.result_set.S3FileSystem, instead of patching _pre_fetch, _get_content_length, and _create_s3_file_system (3eb4184). 163 offline tests passed after the change.
FINDINGS from /code-review, partly repaired in ecf6f4d: _read_csv_result returned the internal read options that no caller used, through wraps of pandas.read_csv and _read_csv_with_pyarrow; both are gone. Deferred: the base helpers _read_csv_result, _unload_result_set, and _read_unload_result still build the result set without __init__; moving their KEEP tests onto _local_result_set would rewrite tests outside the rows of #1244.
| assert df.columns.tolist() == names | ||
| assert df.iloc[:, 0].tolist() == expected_names | ||
|
|
||
| def test_read_small_csv_with_pyarrow_engine(self, tmp_path): |
There was a problem hiding this comment.
Round 1 FINDINGS, repaired in ecf6f4d: this test and test_read_csv_ddl_preserves_numeric_looking_names (line 302) built their expected frames by reading the same result set with engine="c", against the repository's rule that expected values are independent of the implementation. Both now assert explicit names, values, and dtypes. Mutations: removing the file-size check fails this test (the PyArrow path reads the timestamp at datetime64[ns], the C engine at datetime64[us]); removing the .txt fallback fails 12 DDL cases.
|
|
||
| @pytest.mark.parametrize("engine", ["c", "python"]) | ||
| @pytest.mark.parametrize("fails", [False, True], ids=["read", "failed-read"]) | ||
| def test_binary_csv_stream_close_after_whole_read(self, tmp_path, engine, fails): |
There was a problem hiding this comment.
Round 1 FINDINGS, repaired in ecf6f4d: no test checked that a whole (not chunked) binary read closes its file, after test_binary_null_vs_empty dropped the private _csv_stream.closed assert. This test checks it after a read and after a failed read. Limit: a mutation that only skips entering the stream into the ExitStack still passes, because the stream is closed when CPython releases it; no caller can observe the difference.
| ], | ||
| ids=["c", "pyarrow", "duplicate_name"], | ||
| ) | ||
| def test_binary_without_converter(self, engine, removed, second_column, second_value): |
There was a problem hiding this comment.
Round 1 FINDINGS, repaired in ecf6f4d: the merged duplicate_name case paired the binary column with a varchar value, which has no converter, so a renamed column taking the binary column's converter was not detected. The case now selects true AS value and expects True, as the removed test_binary_duplicate_name_without_converter did. Not yet run against Athena.
| ), | ||
| pytest.param({"parse_dates": ("d",)}, OperationalError, id="parse_dates_tuple"), | ||
| pytest.param( | ||
| {"na_values": ("", "NA")}, [None, "null", "x", None, "y", "z", "w"], id="na_values" |
There was a problem hiding this comment.
Round 1 claim check on a /code-review finding: it said that accepting na_values=("", "NA") in _reads_csv_with_pyarrow() would leave "NA" as text. Checked offline: with that mutation, PyAthena's own PyArrow read and pandas' read give the same values, so the condition only chooses the reader. This na_values case checks that the option reaches the read ("NA" becomes NaN). Also not repaired: a test that engine="pyarrow" chooses PyArrow for a compatible read, because the values are the same either way.
| # The bucket resource, and with threads also the workers' resources. | ||
| expected = 1 if executor_class is SpawnProcessPoolExecutor else 3 | ||
| assert len(resources.call_args_list) == expected | ||
| # Every S3 resource of this process uses s3_config. |
There was a problem hiding this comment.
Round 1 deferrals, with reasons, from /code-review:
- The resource count of this test (1 for processes, 3 for threads) is an internal call count, which Remove, merge, and trim unnecessary tests in the pandas tests #1244 decided to drop; every recorded resource still gets
max_pool_connections == 37. test_iter_parquet_chunks_large_integers(files without Parquet statistics) stays removed: Athena's UNLOAD always writes statistics, and the exact BIGINT is checked against Athena bytest_unload_chunks.dtype_backendin chunk conversion: the livetest_unload_chunks[1-numpy_nullable]compares chunks with the whole read, so ignoringdtype_backendfails it.- The
low_memorykwarg tests are performance-only, as Remove, merge, and trim unnecessary tests in the pandas tests #1244 decided. - The dropped
match=arguments are undocumented message text;ProgrammingErrorstays asserted.
The offline cursor and result-set tests read their local result files through one LocalBucketFileSystem and build the GetQueryResults metadata with query_results_response() from tests/pyathena/util.py. The CSV engine fallback test moves to the result-set tests, where the engine is chosen, and the auto-optimized chunk sizes run offline except one live case. test_csv_open_options keeps five cases, and a user's filesystem for UNLOAD results has its own test. Refs #1244 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| } | ||
|
|
||
|
|
||
| class LocalBucketFileSystem(DirFileSystem): |
There was a problem hiding this comment.
Round 1 /simplify (four angles) at ecf6f4d, applied in 3c543d7: the two offline helpers (_offline_cursor with _ResultFileSystem, _local_result_set with _BucketFileSystem) built their own recording filesystems and GetQueryResults metadata. They now share LocalBucketFileSystem and query_results_response(), added at the end of this file. Both helpers patch the public class name pyathena.pandas.result_set.S3FileSystem, not the private _create_s3_file_system. Skipped: inlining the nested read() in test_read_csv_json_conversion_error, which ruff PT012 rejects; deriving sizes from expected in test_binary_custom_quoting, which is clearer as explicit values; dropping the auto and pyarrow-fallback cases of test_read_csv_json_null_without_dtype_warning, which check the paths that engine selects. Deferred to #1257: moving the tests that use _read_csv_result, _unload_result_set, and _read_unload_result onto the constructor.
| ) | ||
| def test_pandas_cursor_auto_optimize_chunksize_enabled( | ||
| self, pandas_cursor, chunked, monkeypatch | ||
| def test_auto_optimize_chunksize( |
There was a problem hiding this comment.
Round 1 /simplify, applied in 3c543d7: the auto-optimize test ran 5 live queries that differ only in monkeypatched class attributes. test_pandas_cursor_auto_optimize_chunksize_enabled keeps the live medium case, and this offline test has small, large, medium-as_pandas, and explicit_chunksize. Mutations, offline: returning the medium size from the large branch fails [large]; ignoring an explicit chunksize fails [explicit_chunksize]; as_pandas() keeping only the first chunk fails [medium-as_pandas]. Saves 4 queries.
| result_set._block_size = block_size | ||
| result_set._cache_type = cache_type | ||
| assert result_set._get_csv_open_options() == expected | ||
| def test_pyarrow_engine_incompatible_options( |
There was a problem hiding this comment.
Round 1 /simplify, applied in 3c543d7: the engine fallback test is offline result-set behavior, so it moved from TestPandasCursor to TestAthenaPandasResultSet and reads through _local_result_set. Removing each fallback condition of _get_csv_engine (chunksize, quoting, converters, name resolution) still fails exactly its case. test_csv_open_options (line 582) keeps 5 cases: the three data cases under default options, and block_size and cache_type on one.
| # The bucket resource, and with threads also the workers' resources. | ||
| expected = 1 if executor_class is SpawnProcessPoolExecutor else 3 | ||
| assert len(resources.call_args_list) == expected | ||
| # Every S3 resource of this process uses s3_config. |
There was a problem hiding this comment.
Self-review round 2 (claims, callers, AWS operation, evidence) of 713d57bd..3c543d73.
FINDINGS in the PR body, corrected in the description: it still listed 147 functions, 389 collected tests, about 150 queries, and test_pyarrow_engine_incompatible_options in the cursor tests. It now states 150 functions, 390 collected, about 154 queries and 8 external tables, the moved tests, the shared helpers, the boolean duplicate-name merge, #1257, and 177 + 30 offline tests passing at 3c543d7.
Checked claims: test_to_sql_with_multiple_partitions now compares sorted(SHOW PARTITIONS) with col_int=0..4/col_string=a then 5..9/b, which is also the lexicographic order, so the assertion holds for a correct write. The invalid-credential case of test_csv_storage_options fails at S3 with a 403, which PyAthena's RetryConfig does not retry. The 8 external tables are the 3 test_empty_result_ddl cases trimmed in the sync tests and the 5 of the removed async test_empty_result_ddl. Live tests have not run yet.
The UNLOAD chunk test checks the dtypes of each dtype_backend and an empty column list, which reads the result whole, and a repeated column is fetched twice. The CSV open-options test checks the documented 32 MiB blocks and two cached blocks and the options that PyAthena's filesystem is created with. The storage-options test reads a binary NULL and an empty binary value, and engine="pyarrow" is checked through the values of numeric-looking, multiline, and NULL varchar values. Refs #1244 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| assert [len(chunk) for chunk in chunks] == sizes | ||
| assert [tuple(row) for chunk in chunks for row in chunk.itertuples(index=False)] == expected | ||
|
|
||
| def test_read_csv_with_pyarrow_engine_values(self, tmp_path): |
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 01a14a07-423e-7161-b942-fd4974b80a10. Static review of 713d57bd..3c543d73 in a clean detached snapshot of 3c543d7, with the diff, the docs/testing.md "Choose what to test" section, and no PR text or author findings; no edits, builds, tests, network, or GitHub access. Covered: all seven changed files, the removed tests at the base, the pandas sync, async, and aio code, the shared result-set and cursor code, the helpers, live queries, marks, fixture scopes, and cleanup. Result: FINDINGS (12).
Finding 1 (P2): with the live test_pyarrow_engine_string_values and test_pyarrow_engine_multiline_values_across_blocks removed, replacing the _read_csv_with_pyarrow() dispatch (pyathena/pandas/result_set.py:1090) with pd.read_csv() would pass; the remaining direct tests call the helper itself.
There was a problem hiding this comment.
Checked offline in efbdb7c: replacing the dispatch with pd.read_csv() gives the same values, because pandas' PyArrow engine with PyAthena's string dtypes also keeps "007", "1e3", and multiline text, so the dispatch only chooses the reader and is not a caller-observable regression. Added anyway: test_read_csv_with_pyarrow_engine_values reads numeric-looking, multiline, and NULL varchar values with engine="pyarrow" through the constructor, replacing the value checks of the removed live tests; the multi-block case stays in the offline test_read_csv_with_pyarrow_multiline_values_across_blocks.
There was a problem hiding this comment.
Correction in 102e971, after the follow-up review: the dispatch is caller-observable. For a varchar column of only numeric-looking values ("007", "1e3", "10"), pandas' own PyArrow reader returns numbers, so replacing the dispatch with pd.read_csv() now fails test_read_csv_with_pyarrow_engine_values[numeric_looking] (offline). The earlier input passed only because its text values made pandas infer strings.
| pytest.param({"dtype_backend": None}, None, None, id="invalid_dtype_backend"), | ||
| ], | ||
| ) | ||
| def test_unload_chunks_with_read_options(self, tmp_path, kwargs, sizes, columns): |
There was a problem hiding this comment.
Independent review finding 2 (P2): the dtype_backend case checks only sizes and column names; always using numpy_nullable in _convert_parquet_chunk() (pyathena/pandas/result_set.py:1616) passes.
Finding 9 (P2): with test_reads_parquet_in_chunks removed, nothing reads with columns=[]; removing len(columns) > 0 at :1559 sends it to ParquetChunkReader, which rejects it.
There was a problem hiding this comment.
Repaired in efbdb7c: every case asserts that the chunk dtypes are pd.ArrowDtype exactly when dtype_backend="pyarrow"; forcing numpy_nullable now fails [dtype_backend]. A no_columns case (columns=[]) reads the result whole as one 3-row chunk; removing len(columns) > 0 now fails it with ValueError: columns must not be empty. (offline).
| for dtype in chunk.dtypes | ||
| ) | ||
|
|
||
| def test_unload_repeated_columns(self, tmp_path): |
There was a problem hiding this comment.
Independent review finding 5 (P2): with test_fetch_duplicate_column_names removed, selecting columns by label instead of position (pyathena/pandas/result_set.py:463) would break fetching a repeated Parquet projection; the repeated-columns case only inspects DataFrames.
There was a problem hiding this comment.
Repaired in efbdb7c: test_unload_repeated_columns fetches columns=["a", "a"] and expects [(1, 1), (2, 2)]; selecting by label instead of iloc now fails it (offline).
| result.close() | ||
| open_options.assert_called_once_with(ANY) | ||
| assert fs.open.call_args.kwargs["cache_type"] == "sentinel" | ||
| def test_csv_open_options(self, tmp_path, data, types, chunksize, kwargs, prefetch): |
There was a problem hiding this comment.
Independent review finding 11 (P2): _local_result_set replaces PyAthena's filesystem and drops its constructor arguments, so removing the default_block_size/default_cache_type forwarding of _create_s3_file_system() passes test_csv_open_options.
There was a problem hiding this comment.
Repaired in efbdb7c: _local_result_set records the arguments that PyAthena's S3FileSystem was created with, and the block_size and cache_type cases assert default_block_size/default_cache_type; dropping that forwarding now fails both (offline). The prefetch cases also assert the documented 32 MiB blocks and two cached blocks (docs/pandas.md:598) as literals.
There was a problem hiding this comment.
Revised in 102e971, after the follow-up review: the recorded constructor arguments of the mocked filesystem are gone. They were a stored-argument check, and the 5 MiB and "bytes" values equal the filesystem defaults, so no read changed without them. block_size and cache_type only change how PyAthena's filesystem reads, which a local fake cannot observe; their forwarding from each cursor to the result set stays checked by test_read_options. The test keeps the documented 32 MiB blocks and two cached blocks.
| } | ||
| if with_options | ||
| else None | ||
| def test_csv_storage_options(self, pandas_cursor, query, expected): |
There was a problem hiding this comment.
Independent review finding 7 (P2): with test_binary_as_pandas removed, bypassing BinaryCSVReader only on the storage_options branch of _open_binary_csv_stream() would turn binary NULL into empty bytes unnoticed; this test read only a nonempty binary value.
There was a problem hiding this comment.
Repaired in efbdb7c: the binary case selects X'01', CAST(NULL AS VARBINARY), and X'', and expects (b"\x01", None, b"") through the valid storage options. Not yet run against Athena.
| @@ -648,6 +380,7 @@ def test_read_options(self, execute_kwargs): | |||
| No AWS calls; the query and its result set are mocked. | |||
| """ | |||
| cursor_kwargs = { | |||
There was a problem hiding this comment.
Independent review finding 12 (P3): test_read_options patches the private _execute/_poll on the success path and checks the constructor arguments of a mocked result set; the added engine/na_values cases read no data.
There was a problem hiding this comment.
Not repaired: #1244's Notes keep the three test_read_options as the only check that each cursor class forwards its read options with its own code, with execute() overriding the cursor; no offline public seam observes that. The added engine and na_values keys replace the forwarding checks of the removed async tests, as their verdict rows describe.
| ], | ||
| ) | ||
| def test_unload_chunks_with_read_options(self, tmp_path, kwargs, sizes, columns): | ||
| """Only chunksize with supported read options reads an UNLOAD result in chunks.""" |
There was a problem hiding this comment.
Independent review findings 3, 4, 6, 8, 10 (P2), deferred with reasons:
- 3:
chunksize=0reachingParquetChunkReaderraisesValueErrorinstead ofOperationalErrorif the reader moves out of thetry; Remove, merge, and trim unnecessary tests in the pandas tests #1244 removedtest_iter_parquet_chunks_reader_erroras a wrapper branch that matched pyarrow's message (criteria 7, 8). - 4: Parquet files without statistics are not Athena output; UNLOAD writes statistics, and the exact BIGINT is checked against Athena by
test_unload_chunks. - 6:
header=[0, 1]reads Athena's one-row header as two levels; the removed test checked that pandas leaves such labels and a skip branch. - 8: the
ImportErrorfallback needs pyarrow to be unimportable in the test process, which pandas and the other tests import; stated in the PR body as untested. - 10: an empty middle chunk is not reachable: UNLOAD never writes empty files, and pandas never yields an empty CSV chunk.
| result.close() | ||
| open_options.assert_called_once_with(ANY) | ||
| assert fs.open.call_args.kwargs["cache_type"] == "sentinel" | ||
| def test_csv_open_options(self, tmp_path, data, types, chunksize, kwargs, prefetch): |
There was a problem hiding this comment.
Self-review of the independent-review repair, 3c543d73..efbdb7c5 (one added commit, same base).
Round 1 (behavior and test quality): CLEAN. Each repaired test fails under the mutation named in its reply, offline with --noconftest; git diff -- pyathena/ stayed empty. fs.created_with is set by _local_result_set for every caller and read only by test_csv_open_options. The dtype check of test_unload_chunks_with_read_options holds for every case: without dtype_backend, no chunk column is a pd.ArrowDtype.
Round 2 (claims): FINDINGS, corrected in the PR body: 152 functions and 393 collected tests, the dtype and documented-block checks, and 180 offline tests passing at efbdb7c. The literal 32 MiB and two blocks match docs/pandas.md:598 and pyathena/pandas/result_set.py:600,1152-1154. The finding 1 claim that the dispatch is not observable was checked by running that mutation.
The engine="pyarrow" values test reads a varchar column of only numeric-looking values, which pandas' own PyArrow reader would read as numbers, and keeps the padding in another column. The CSV open-options test no longer records the arguments of the mocked filesystem. Refs #1244 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| result_set._block_size = block_size | ||
| result_set._cache_type = cache_type | ||
| assert result_set._get_csv_open_options() == expected | ||
| def test_read_csv_with_pyarrow_engine_values(self, tmp_path, values): |
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 01a14a17-6f1e-7e81-a527-292e11e538ca. Static review of 3c543d73..efbdb7c5 in a clean detached snapshot of efbdb7c. It confirmed by static tracing that the repairs for findings 2, 5, 7, and 9 catch their mutations through public results. Result: FINDINGS (2), both repaired in 102e971:
1 (P2): the dispatch mutation still passed test_read_csv_with_pyarrow_engine_values, because its text values made pandas infer strings. The test now has a numeric_looking case and keeps the padding in another column; the mutation fails that case (offline).
2 (P2): created_with copied the mock's call arguments, a stored-argument check, and the explicit values equal the filesystem defaults. Removed; see the reply on the finding 11 thread.
Self-review of efbdb7c5..102e9712: round 1 CLEAN (181 offline tests passed; git diff -- pyathena/ empty); round 2: the PR body is updated with the counts of this head.
| result.close() | ||
| open_options.assert_called_once_with(ANY) | ||
| assert fs.open.call_args.kwargs["cache_type"] == "sentinel" | ||
| def test_csv_open_options(self, tmp_path, data, types, chunksize, kwargs, prefetch): |
There was a problem hiding this comment.
Independent follow-up review 2 (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a14a1d-91a8-7113-9a2f-123650649e44. Static review of efbdb7c5..102e9712 in a clean detached snapshot of 102e971. It confirmed that the numeric_looking case fails under the dispatch mutation through fetchall() values, and found no other breakage. Result: FINDINGS (1).
P2: removing default_block_size/default_cache_type from _create_s3_file_system() (pyathena/pandas/result_set.py:869-870) passes, because the test's filesystem ignores construction options; the reviewer asks for coverage with nondefault settings through the real filesystem.
Deferred, with reasons: no test at the base checked this forwarding either (the base default_cache_type asserts are for storage_options), so this PR removes no coverage of it; the recorded-arguments check was removed as a stored-argument check. Observing it needs PyAthena's real S3FileSystem over a stubbed S3 client, which belongs with the filesystem-backed helpers that #1257 moves onto the constructor. Stated in the PR body.
1 participant
WHAT
Apply the rows of #1244 to the pandas cursor tests (
tests/pyathena/pandas/,tests/pyathena/aio/pandas/test_cursor.py). Each row was re-checked on 713d57b, after #1233 and #1238. Where the cited cover runs offline, the behavior was broken in the source and the cover was run with--noconftest; where it is a live test, what it asserts was checked by reading it. Every source change was reverted, and this diff changes no file underpyathena/.PandasCursor: drop the parameter cases that repeat a read (the(unload, "pyarrow")case, which"auto"reads with pyarrow;chunksize=1_000_000, a single chunk for these queries; the unload and engine cases of DDL results) while keepingtest_unload_chunkswithBIGINT '9007199254740993'andcheck_exact=True,test_many_as_pandas[chunksize=1000], andtest_as_pandas[unload=False-auto-None]. Remove the tests that set private chunking attributes or compare private predicates with constants. The 20 livetest_binary_csv_stream_close*cases run offline through_offline_cursor(), a realPandasCursorwhose Athena and S3 clients return real responses and whose filesystem serves a local file; they keep their IDs, and a whole binary read is checked the same way. The auto-optimized chunk sizes run offline except one live case, and the storage options (valid options read the rows, invalid credentials raiseOperationalError) are checked through the rows that the cursor returns.AsyncPandasCursorandAioPandasCursor: remove the copies of sync tests that read through the sameAthenaPandasResultSet(NA values, DDL, empty results, 10,000 rows), and keep unloadFalseandTruein the parameter cases; the forwarding ofengineandna_valuesjoinstest_read_options. The aiotest_execute_returns_self,test_description, andtest_as_pandas_unloadjoin the existingtest_fetchoneandtest_fetchone_unloadqueries.AthenaPandasResultSet: the offline tests build the result set through its constructor over local result files (_local_result_set(), with the columns from a mocked GetQueryResults response and the size from a mocked HEAD response), and check values, dtypes, chunk sizes, and open options (the documented 32 MiB blocks and two cached blocks) instead of private predicates and the exact arguments of internalread_csv()/read_parquet()calls. The pyarrow engine options test and the engine fallback test (test_pyarrow_engine_incompatible_options) move here from the cursor tests. The local files and the GetQueryResults metadata of both helpers come fromLocalBucketFileSystemandquery_results_response()intests/pyathena/util.py.to_sql:test_to_sql_with_multiple_partitionscompares its rows (,became==), andtest_to_sql_session_credentials_and_s3_configno longer counts internal resource calls.Verdicts changed from the issue after the checks:
pandas/test_cursor.py::TestPandasCursor::test_get_csv_engine_explicit_specificationpandas/test_result_set.py::TestAthenaPandasResultSet::test_pyarrow_engine_incompatible_optionspandas/test_cursor.py::TestPandasCursor::test_binary_custom_dialecttest_binary_custom_quotingpandas/test_cursor.py::TestPandasCursor::test_binary_duplicate_name_without_converterpandas/test_async_cursor.py::TestAsyncPandasCursor::test_binary_null_vs_empty,aio/pandas/test_cursor.py::TestAioPandasCursor::test_binary_null_vs_emptyget_default_converter, and no other test of these cursors reads a column that the converters read differently.pandas/test_async_cursor.py::TestAsyncPandasCursor::test_empty_and_null_stringtest_read_optionsna_values.pandas/test_result_set.py::TestPandasDataFrameIterator::test_as_pandas_single_dataframetest_as_pandas_matches_whole_read(single_chunk)pandas/test_result_set.py::TestAthenaPandasResultSet::{test_get_csv_open_options, test_read_csv_open_options}test_csv_open_optionstest_read_optionspasses when the open options are dropped.pandas/test_result_set.py::TestAthenaPandasResultSet::test_read_parquet_filesystemtest_unload_filesystem_optionstest_unload_with_filesystem_optionspasses when the options are ignored, since PyAthena's filesystem reads the same files.pandas/test_result_set.py::TestAthenaPandasResultSet::test_read_csv_json_with_dtypeDeferred until #1245:
aio/pandas/test_cursor.py::TestAioPandasCursor::test_cancel_initial(its coveraio/test_common.py::TestWithAsyncFetch::test_cancel_without_querydoes not exist yet) andTestAioPandasCursor::test_invalid_arraysize(removed with or after #1245's arraysize merge).The older helpers
_read_csv_result(),_unload_result_set(), and_read_unload_result(), which build the result set without__init__, stay for the tests outside the rows of #1244; #1257 tracks moving them onto the constructor.After this change no test covers the
ImportErrorfallback of the CSV engine choice (pyarrow not installed), thena_valueslist condition of the pyarrow read (both read paths give the same values), or the empty-chunk skip offetchall()across chunks, which Athena results never reach. No test checked before this change, or checks now, that a cursor'sblock_sizeandcache_typereach PyAthena'sS3FileSystemfor the result file.The changes remove about 154 Athena queries and 8 external tables per run: about 86 queries from the sync cursor tests (including the 20 binary stream queries and 4 auto-optimize cases), about 63 from the async cursor tests, and 6 from the aio cursor tests.
WHY
Closes #1244. Part of #1239: these tests restated the code, patched or asserted private state, or repeated a contract that another test checks with the same kind of evidence, as described in
docs/testing.md("Choose what to test").TEST
Python 3.15.0, pandas 3.0.6, pyarrow 26.0.0. At 102e971:
just format: no changes.just lint: passed.Offline, with
--noconftestand placeholder settings:pandas/{test_result_set,test_converter,test_reader}.py, the offlinepandas/test_util.pytests, and the async and aiotest_read_options: 181 passed; the offlinepandas/test_cursor.pytests (test_binary_csv_stream_close*,test_auto_optimize_chunksize,test_read_options): 30 passed.Mutation checks: each source mutation named in the rows and in the review repairs failed the kept, merged, or rewritten offline test that covers it, and
git diff -- pyathena/was empty after each. For a cover that is a live test, what it asserts was checked by reading it; those mutations have not been run against Athena.Live, at 102e971, with no Test workflow running:
uv run --env-file .env pytest -n 4 tests/pyathena/pandas tests/pyathena/aio/pandas: 392 passed, 2 xfailed (the strict Make PandasCursor's temporal dtypes and fetched types depend only on the Athena type #1219 xfails oftest_temporal_types, unchanged), in 279 s, including the managed-results parameters and the liveto_sqltests.CI after Ready, at 102e971: Test run 39038656609 passed (
test / run (3.15),test-sqla,test-sqla-async, lint). The SQLAlchemy jobs ran becausetests/pyathena/util.pyis in thecorepath filter.Not run: the live mutations of the covers that are live tests.
🤖 Generated with Claude Code