Repository navigation
Remove and rewrite unnecessary Polars, Arrow, and S3FS cursor tests - #1252
Conversation
Remove the as_arrow() wrapper tests and the row-count-only chunk and chunked fetch tests, whose contracts test_whole_result_reused, test_iter_chunks_data_consistency, test_unload_chunks, and test_fetch_across_chunks check with values. The chunk-size bound joins test_iter_chunks_data_consistency. The offline reader tests build the result set through its constructor over local result files and read it through iter_chunks() instead of calling private readers on an instance without __init__. The mocked storage-options tests become a live test whose invalid credentials fail the read, and an offline test that the given options reach every Polars read of an UNLOAD result. The strict and ignore_errors timestamp reads are one test, which replaces the converter's own non-strict test. Refs #1246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove the as_polars() tests that check pl.from_arrow() on the tables that the as_arrow tests assert, and the timeout tests of private attributes; test_as_polars keeps one CSV query. The duplicate TIMESTAMP column of the offline result-set test joins the live test_duplicate_column_names, and the fetch-across-batches checks join test_fetch_across_empty_batches. test_fetch_after_close closes a result with rows left, so that it fails when close() keeps them. The Parquet chunk reader tests check that more files were opened than read and that reads continue after the first chunk, instead of exact lookahead counts and byte ranges. Refs #1246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove the single-cell NULL and empty-string tests, the basic reader queries, and the async copies of sync queries that read through the same result set; test_null_vs_empty_string checks both readers in one query. The tab, newline, and comma values are columns of one SELECT per reader, and test_executemany runs on the empty_table fixture instead of being skipped. The reader tests of Python's csv module and of the class declaration are removed. test_context_manager leaves the context before the rows run out, so that it fails when close() does not close the file. The CSV open options are checked through a result set built by its constructor. test_on_start_query_execution stays until #1243 makes the connection callback test run the callback for every cursor class. Refs #1246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The offline storage-options test also reads a chunked CSV result, and checks that the given options reach every Polars read unmerged. The nested-value test fails if the rows end instead of raising, the mixed fetch case checks the row fetchone() returns after fetchmany(), and the duplicate-name query has a NULL timestamp. Refs #1246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The local result sets get their columns from a mocked GetQueryResults response instead of a patched _pre_fetch(), and the tests read them with as_polars(). The live storage-options test keeps the whole CSV read through fsspec, the only read that the offline test does not observe. The S3FS special values are columns of the NULL and empty-string query, and the Arrow reader tests share the fixture that records opened files. Refs #1246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| [(False, 1), (True, None), (True, 1)], | ||
| ids=["csv-chunks", "unload", "unload-chunks"], | ||
| ) | ||
| def test_storage_options_replace_defaults(self, tmp_path, unload, chunksize): |
There was a problem hiding this comment.
Self-review round 1 (implementation behavior, contracts, simplicity, regression coverage) of 713d57bde5248f9a77b763b44eb4c6945ceb4361..6b8f9ea66f66a752bba7cb832ab6e9b5a357a6d4, with /code-review (high) at d894953 and /simplify at 5a8b810.
FINDINGS, repaired in 5a8b810 and 6b8f9ea: the live storage-options test could not tell replacing PyAthena's options from merging them on the native object-store reads, and the chunked CSV read (scan_csv) had no offline check. This test now reads chunked CSV, whole UNLOAD, and chunked UNLOAD results and checks that every Polars read gets the given dict itself. Mutations: merging the defaults under the given options failed all 3 cases; only _scan_csv_chunks using the defaults failed csv-chunks; earlier, each of _read_parquet/_scan_parquet_chunks using the defaults failed its case. Not kept: the old check that PyAthena's own options are never computed; nothing a caller observes depends on it.
| # Polars raises the failure within a struct as a BaseException of its own. | ||
| with pytest.raises(BaseException) as error: # noqa: PT011 | ||
| next(rows) | ||
| assert not isinstance(error.value, StopIteration) |
There was a problem hiding this comment.
Round 1 FINDINGS, repaired in 5a8b810: pytest.raises(BaseException) also accepted the StopIteration of rows that end early. Mutation: catching the failure in itertuples() and returning now fails this test.
| result_set._metadata = None | ||
| result_set._connection = MagicMock() | ||
| return result_set | ||
| def _local_result_set( |
There was a problem hiding this comment.
Round 1 simplification, 6b8f9ea: the local result sets get their columns from a mocked GetQueryResults response, so _process_metadata runs as it does for users, instead of a patched _pre_fetch; _get_content_length stays patched because the output location is a local path that parse_output_location rejects. The tests read through as_polars(), and the _read_all helper is gone. Re-checked mutations after the change: no timestamp text columns (6 failures), whole-read strict=False (fails [False-None]), new_columns not applied (fails [None-kwargs3-expected3]).
| ).as_polars() | ||
| assert df.to_dicts() == [{"number_of_rows": "1"}] | ||
|
|
||
| def test_storage_options_replace_defaults(self, polars_cursor): |
There was a problem hiding this comment.
Round 1 simplification, 6b8f9ea: only the whole CSV read through fsspec stays live, because the offline TestAthenaPolarsResultSet::test_storage_options_replace_defaults observes the native object-store reads (chunked CSV, UNLOAD). Saves 2 queries, one of them an UNLOAD. Not yet run against Athena.
| else: | ||
| rows = result_set.fetchmany(2) | ||
| row = result_set.fetchone() | ||
| assert row == ((expected[2],) if len(expected) > 2 else None) |
There was a problem hiding this comment.
Round 1 FINDINGS, repaired in 5a8b810: filter(None, [fetchone()]) hid a fetchone() that returned None between batches, because the following fetchall() returned the rest. Mutation: fetchone() returning None while rows remain (after fetchmany()) now fails [middle-mixed]. Kept over the /simplify suggestion to drop this assert, because the final rows == comparison passes under that mutation.
| "CAST('01:02:03' AS TIME) AS t" | ||
| "CAST('01:02:03' AS TIME) AS t, " | ||
| "CAST('2020-01-02 03:04:05.123456789' AS TIMESTAMP(9)) AS x, " | ||
| "CAST(NULL AS TIMESTAMP) AS x" |
There was a problem hiding this comment.
Round 1 FINDINGS, repaired in 5a8b810: the merge of test_read_csv_timestamps_with_the_same_name had dropped its NULL timestamp in a duplicate-name column. This column restores it in the same query. Not yet run against Athena.
| indirect=["s3fs_cursor"], | ||
| ) | ||
| def test_null_vs_empty_string(self, csv_reader, expected_empty): | ||
| def test_null_vs_empty_string(self, s3fs_cursor, expected_empty): |
There was a problem hiding this comment.
Round 1 simplification, 6b8f9ea: the tab, newline, tab+newline, and comma values joined the NULL/'' query, one SELECT per reader (2 queries instead of 4). Not yet run against Athena.
| with pytest.raises((OSError, ValueError)): | ||
| list(ParquetChunkReader(fs, [*paths, paths[0], paths[0]], 2)) | ||
| # A file after the failed third one was opened ahead. | ||
| assert len(opened) > 3 |
There was a problem hiding this comment.
Round 1 deferrals, with reasons. Read-ahead depth (> 3/> 1 instead of exact counts), the prefetched file's cache_type, and the per-row-group byte ranges are not asserted, as #1246 rows decided: they pin implementation choices that affect only performance. No test remains for the except close in AthenaArrowResultSet._open_csv (pyathena/arrow/result_set.py:236-238) or for the Arrow timeouts reaching pyarrow.fs.S3FileSystem (pyathena/arrow/result_set.py:163-166); both are stated in the PR body. AsyncPolarsCursor's own executor size is no longer checked, as the row decided (it checked a stored constructor argument).
| ).as_polars() | ||
| assert df.to_dicts() == [{"number_of_rows": "1"}] | ||
|
|
||
| def test_storage_options_replace_defaults(self, polars_cursor): |
There was a problem hiding this comment.
Self-review round 2 (claims, caller compatibility, AWS operation, evidence) of 713d57bde5248f9a77b763b44eb4c6945ceb4361..6b8f9ea66f66a752bba7cb832ab6e9b5a357a6d4.
FINDINGS in the PR body, corrected in the description: it still described the pre-/simplify state (a live storage-options test of CSV, chunked CSV, and UNLOAD; reads through iter_chunks() only; a separate tab-character query; 242 functions, 426 collected, about 32 queries, tested at d894953). It now states one live CSV case, the offline chunked CSV/UNLOAD test, the merged S3FS query, 241 functions, 423 collected, about 36 queries, and 187 offline tests passing at 6b8f9ea.
AWS operation of this test: the invalid key/secret reach PyAthena's filesystem (registered for s3, which turns them into botocore credentials). RetryConfig retries by error code (throttling and service errors), so the 403 fails the read at once instead of retrying. Not yet run against Athena.
| [(False, 1), (True, None), (True, 1)], | ||
| ids=["csv-chunks", "unload", "unload-chunks"], | ||
| ) | ||
| def test_storage_options_replace_defaults(self, tmp_path, unload, chunksize): |
There was a problem hiding this comment.
Round 2 claim check: the docstring says Polars' native object store ignores the options for local files. Checked on Polars 2.0.0 for read_parquet, scan_parquet, and scan_csv with storage_options on a local path: all read the file. If a Polars version rejected them, this test would fail instead of passing silently.
| else: | ||
| rows = result_set.fetchmany(2) | ||
| row = result_set.fetchone() | ||
| assert row == ((expected[2],) if len(expected) > 2 else None) |
There was a problem hiding this comment.
Round 2 claim checks: re-ran at 6b8f9ea the mutation that removes self._batches = iter([]) from Arrow's close(); it fails TestAthenaArrowResultSet::test_fetch_after_close and passes the base test_result_set.py::TestAthenaResultSet::test_fetch_after_close, as the PR states. git diff --stat 713d57bd..6b8f9ea6 -- pyathena/ is empty. Correction to the round 1 record: the untested except close is at pyathena/arrow/result_set.py:242-244, and the timeouts at :162-166.
| @pytest.mark.skip(reason="Requires insert_test table to exist in test environment") | ||
| def test_executemany(self, s3fs_cursor): | ||
| rows = [(1, "foo"), (2, "bar"), (3, "jim")] | ||
| def test_executemany(self, s3fs_cursor, empty_table): |
There was a problem hiding this comment.
Round 2 AWS-operation check: this test was skipped and now runs 6 queries per run (the empty_table CREATE and DROP, 3 INSERTs, 1 SELECT), 3 of them against the DML quota. It keeps the three rows of the Arrow, Polars, and pandas test_executemany, whose jim o'rourke row checks the quoting. test_on_start_query_execution stays, because test_connection.py::test_cursor_accepts_connection_callbacks (master, line 209) asserts only cursor._on_start_query_execution is .... Not yet run against Athena.
ArrowCursor.as_polars() is checked on two rows and columns. The Polars storage-options test reads with explicit options while PyAthena's own credential lookup fails, and compares the options by value. The S3FS open options are checked against the documented 32 MiB blocks and two cached blocks, and the Parquet chunk reader test checks that a streamed file is opened with the requested cache type. Refs #1246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| [{"cursor_kwargs": {"unload": False}}, {"cursor_kwargs": {"unload": True}}], | ||
| indirect=["arrow_cursor"], | ||
| ) | ||
| def test_as_polars(self, arrow_cursor): |
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 01a149cf-467f-78a3-9798-57123cbadabe. Static review of 713d57bde5248f9a77b763b44eb4c6945ceb4361..6b8f9ea66f66a752bba7cb832ab6e9b5a357a6d4 in a clean detached snapshot, 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 11 changed files, the removed tests at the base, the cursor and result-set code, remaining tests under tests/pyathena/, the three changed helpers, live-query expectations, IDs, marks, fixture scopes, and cleanup. Result: FINDINGS (7).
Finding 1 (P2): with test_many_as_polars removed, only single-row results exercise AthenaArrowResultSet.as_polars(); pl.from_arrow(self._table.slice(0, 1)) at pyathena/arrow/result_set.py:448 would pass. The removed complex tests were the only multi-column inputs.
There was a problem hiding this comment.
Repaired in 738069e: test_as_polars reads VALUES (1, 'a'), (2, 'b') (two rows, two columns, one query) and compares to_dicts(). A truncating as_polars() now fails it. Not yet run against Athena.
| else: | ||
| rows = result_set.fetchmany(2) | ||
| row = result_set.fetchone() | ||
| assert row == ((expected[2],) if len(expected) > 2 else None) |
There was a problem hiding this comment.
Independent review finding 2 (P2): the deleted test_open_csv_closes_file_when_stream_fails (base tests/pyathena/arrow/test_result_set.py:195) was the only test of the file.close() at pyathena/arrow/result_set.py:243 when file.size() or ReadAheadStream raises.
There was a problem hiding this comment.
Not repaired, deliberately: #1246 decided REMOVE for this test (criteria 1 and 7). It called the private _open_csv on an instance without __init__ and asserted file.close on a mock. The gap is stated in the PR body; the current lines are pyathena/arrow/result_set.py:242-244.
| [(False, 1), (True, None), (True, 1)], | ||
| ids=["csv-chunks", "unload", "unload-chunks"], | ||
| ) | ||
| def test_storage_options_replace_defaults(self, tmp_path, unload, chunksize): |
There was a problem hiding this comment.
Independent review finding 3 (P2): the rewrite lost the check that explicit storage_options do not compute PyAthena's options. self._kwargs.get("storage_options", default()) evaluates default() eagerly and passes, because _local_result_set makes the credential lookup harmless; a failing credential refresh would then fail reads that have valid explicit credentials.
There was a problem hiding this comment.
Repaired in 738069e: _local_result_set takes credentials_error, and this test reads with explicit options while the session's credential lookup raises OSError. Mutation: evaluating default() eagerly in _storage_options() now fails all three cases (offline).
| assert result_set.fetchall() == [(1,), (2,)] | ||
| calls = read_parquet.call_args_list + scan_parquet.call_args_list + scan_csv.call_args_list | ||
| assert calls | ||
| assert all(call.kwargs["storage_options"] == storage_options for call in calls) |
There was a problem hiding this comment.
Independent review finding 6 (P3): is storage_options and the exact call count are not caller contracts; a copy of the options or a different schema discovery would fail the test without changing behavior.
There was a problem hiding this comment.
Repaired in 738069e: the options are compared by value (==), and the exact call count is gone; the test asserts that at least one Polars read was observed. With the failing credential lookup, a read that falls back to PyAthena's options raises instead of passing.
| if prefetch: | ||
| assert options["cache_type"] == "background" | ||
| assert options["block_size"] == 32 * 2**20 | ||
| assert options["cache_options"] == {"maxblocks": 2} |
There was a problem hiding this comment.
Independent review finding 4 (P2): the rewrite compares the block size with the production constant and drops the two cached blocks, both documented in docs/s3fs.md:255; a 64 MiB CSV_BLOCK_SIZE or maxblocks: 64 passes.
There was a problem hiding this comment.
Repaired in 738069e: the prefetch cases assert the documented block_size == 32 * 2**20 and cache_options == {"maxblocks": 2} as literals, and the docstring states them. Mutation: maxblocks: 64 now fails [default] and [aio] (offline).
| # The later row groups are fetched after the first chunk. | ||
| assert len(fs.fetched) > fetched | ||
| # Without a cache type, only the ranges that pyarrow requests are read. | ||
| assert fs.cache_types == [cache_type or "none"] |
There was a problem hiding this comment.
Independent review finding 5 (P2): both cache parameters of test_streamed_file_row_groups only require more reads after the first chunk; opening every streamed file with cache_type="none" at pyathena/arrow/reader.py:224 passes and ignores an explicit "bytes".
There was a problem hiding this comment.
Repaired in 738069e: _RecordingFileSystem records the cache type each file is opened with, and the test asserts [cache_type or "none"] (the reader docstring: None means "none"). Mutations: opening every streamed file with "none" fails [bytes], and defaulting to "bytes" fails [default] (offline).
| # Polars raises the failure within a struct as a BaseException of its own. | ||
| with pytest.raises(BaseException) as error: # noqa: PT011 | ||
| next(rows) | ||
| assert not isinstance(error.value, StopIteration) |
There was a problem hiding this comment.
Independent review finding 7 (P3): with test_itertuples_without_column_names deleted (base tests/pyathena/polars/test_result_set.py:651), an unconditional zip(*blocks, strict=True) at pyathena/polars/result_set.py:237 would drop every row of PolarsDataFrameIterator(df, {}, []), which should give [(), ()] for two rows.
There was a problem hiding this comment.
Not repaired: the input is not reachable through PyAthena. A result always has columns; with columns=[] given to the read, Polars returns a (0, 0) frame (checked on Polars 2.0.0), so a chunk with rows and no column names never reaches PolarsDataFrameIterator. #1246 decided REMOVE on criterion 7.
| [(False, 1), (True, None), (True, 1)], | ||
| ids=["csv-chunks", "unload", "unload-chunks"], | ||
| ) | ||
| def test_storage_options_replace_defaults(self, tmp_path, unload, chunksize): |
There was a problem hiding this comment.
Self-review of the independent-review repair, 6b8f9ea6..738069ed (git range-diff is one added commit on the same base; both objects exist).
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. credentials_error defaults to None, so the other _local_result_set callers keep a session without credentials. The test_as_polars query still costs one query.
Round 2 (claims and operation): FINDINGS, corrected in the PR body: it now states the two-row test_as_polars, the cache-type and documented-block checks, the failing credential lookup in the offline storage-options test, and the tested head 738069e (187 offline tests passed). The literal 32 MiB and two blocks match docs/s3fs.md:255 and CSV_BLOCK_SIZE/cache_options at pyathena/s3fs/result_set.py:68,169. The "none" default matches the ParquetChunkReader docstring (pyathena/arrow/reader.py:73-75).
The Arrow CSV result file is closed when pyarrow's S3 filesystem fails to get its size, through the result set's constructor. The Polars storage-options test fails when a read goes through PyAthena's filesystem, PolarsDataFrameIterator keeps the rows of a chunk without column names, and the chunk reader test checks the cache type of every streamed open without counting them. Refs #1246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| **files, | ||
| chunksize=chunksize, | ||
| credentials_error=OSError("no credentials"), | ||
| file_system=MagicMock(side_effect=AssertionError("PyAthena's filesystem")), |
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 01a149dc-b711-7fe2-9529-1a8347bdcc23. Static review of the repair 6b8f9ea6..738069ed in a clean detached snapshot of 738069e, with the earlier findings, the repair patch, and the test policy. Result: FINDINGS (5). Repairs in 558f368; each was mutation-checked offline (--noconftest), and git diff -- pyathena/ stayed empty.
Finding 1 (P2): with the call count gone, removing not self._kwargs and from _reads_with_pyarrow() routed unload-chunks through PyAthena's filesystem and passed, because the helper's local stand-in ignores credentials.
Repaired: _local_result_set takes file_system, and this test makes PyAthena's filesystem raise. That mutation now fails [unload-chunks].
| } | ||
| with ( | ||
| # The HEAD request parses the output location, which is a local path here. | ||
| patch.object(AthenaPolarsResultSet, "_get_content_length", return_value=1), |
There was a problem hiding this comment.
Independent follow-up finding 2 (P2): the helpers patch private methods on the success path (_get_content_length here; _open and _get_query_results in tests/pyathena/s3fs/test_result_set.py:73,123).
Not repaired, with reasons: the HEAD request parses an s3:// location, but Polars' native readers need the local path, so no public client fake can answer it here. _open is fsspec's documented subclass hook of AbstractFileSystem, whose arguments are what PyAthena passes to the filesystem; the cache it builds belongs to the filesystem tests. The S3FS _get_query_results patch predates this PR (base helper).
| assert result_set.rownumber == 3 | ||
|
|
||
| @pytest.mark.parametrize("method", ["fetchone", "fetchmany", "fetchall", "iter"]) | ||
| def test_csv_file_closed_when_stream_fails(self): |
There was a problem hiding this comment.
Independent follow-up finding 3 (P2): the except close of _open_csv can be tested through public objects: open_input_file() succeeds and the returned file's size() raises.
Repaired: this test builds the result set through its constructor with mocked Athena and S3 clients and a patched pyarrow.fs.S3FileSystem, expects OperationalError, and checks that the file was closed. Removing file.close() at pyathena/arrow/result_set.py:243 fails it. This changes the #1246 row from REMOVE to REWRITE.
| # Polars raises the failure within a struct as a BaseException of its own. | ||
| with pytest.raises(BaseException) as error: # noqa: PT011 | ||
| next(rows) | ||
| assert not isinstance(error.value, StopIteration) |
There was a problem hiding this comment.
Independent follow-up finding 4 (P2): PolarsDataFrameIterator is in the public API docs (docs/api/polars.rst:34), so a chunk with rows and no column names is a caller input.
Repaired: test_itertuples_without_column_names (below, unchanged from the base) is kept (#1246 row REMOVE to KEEP). Dropping the repeat((), length) fallback at pyathena/polars/result_set.py:237 fails it.
| # The later row groups are fetched after the first chunk. | ||
| assert len(fs.fetched) > fetched | ||
| # Without a cache type, only the ranges that pyarrow requests are read. | ||
| assert set(fs.cache_types) == {cache_type or "none"} |
There was a problem hiding this comment.
Independent follow-up finding 5 (P3): == [cache_type or "none"] also required exactly one open.
Repaired: the test compares the set of cache types of the opens. The "none"-always and "bytes"-default mutations still fail [bytes] and [default].
| assert result_set.rownumber == 3 | ||
|
|
||
| @pytest.mark.parametrize("method", ["fetchone", "fetchmany", "fetchall", "iter"]) | ||
| def test_csv_file_closed_when_stream_fails(self): |
There was a problem hiding this comment.
Self-review of the follow-up repair, 738069ed..558f3686 (one added commit, same base).
Round 1 (behavior and test quality): CLEAN. Mutations, offline: dropping the except close fails test_csv_file_closed_when_stream_fails; routing chunks through pyarrow despite read options fails test_storage_options_replace_defaults[unload-chunks]; dropping the empty-row fallback fails test_itertuples_without_column_names. file_system defaults to the local stand-in, so the other _local_result_set callers are unchanged. The new Arrow test patches only public objects: the Athena and S3 clients and pyarrow.fs.S3FileSystem.
Round 2 (claims): FINDINGS, corrected in the PR body: the gap paragraph named the _open_csv close, which is now tested; the verdict table now lists the REMOVE to REWRITE and REMOVE to KEEP changes; the counts are 243 functions and 425 collected; 189 offline tests passed at 558f368.
The S3FS result-set helper gets its columns from a mocked GetQueryResults response instead of a patched _get_query_results(), as the Polars helper does, and the Arrow close test checks that the file was closed without counting the calls. Refs #1246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| arraysize=1, | ||
| retry_config=RetryConfig(), | ||
| ) | ||
| assert file.close.called |
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 01a149e4-c45c-7eb0-bc4c-7abb4fe24015. Static review of 738069ed..558f3686 in a clean detached snapshot of 558f368. It confirmed by static tracing that the repaired tests catch the four named mutations and accepted the S3FS _open patch as an fsspec extension boundary. Result: FINDINGS (2, plus a note that the pre-existing S3FS _get_query_results patch can use connection.client.get_query_results).
Finding 2 (P3): assert_called_once_with() fixed the number of close() calls, which is not the contract.
Repaired in 5b64fc7: assert file.close.called. Removing the except close still fails the test (offline).
| } | ||
| with ( | ||
| # The HEAD request parses the output location, which is a local path here. | ||
| patch.object(AthenaPolarsResultSet, "_get_content_length", return_value=1), |
There was a problem hiding this comment.
Independent follow-up review 2, finding 1 (P2): the Polars helper still patches _get_content_length() on the success path; a real method returning 0 for every nonempty result would pass these tests.
Not repaired, with reasons: chunked CSV results are scanned by Polars' native reader at the output location, so the location must be a local path, which parse_output_location cannot parse for the HEAD request; an s3:// location with a mocked head_object cannot serve those reads. A HEAD that returned 0 would make every live CSV test of PolarsCursor (for example test_fetchone, test_complex) return no rows and fail.
| connection = MagicMock() | ||
| connection.s3_client.head_object.return_value = {"ContentLength": len(data.encode())} | ||
| response = { | ||
| connection.client.get_query_results.return_value = { |
There was a problem hiding this comment.
Independent follow-up review 2, note on the pre-existing _get_query_results patch: repaired in 5b64fc7. The S3FS helper sets connection.client.get_query_results to a GetQueryResults response with the full ColumnInfo fields, as the Polars helper does, so _process_metadata runs. 189 offline tests passed at 5b64fc7; just lint passed.
| connection = MagicMock() | ||
| connection.s3_client.head_object.return_value = {"ContentLength": len(data.encode())} | ||
| response = { | ||
| connection.client.get_query_results.return_value = { |
There was a problem hiding this comment.
Independent follow-up review 3 (relayed): Codex CLI 0.163.0, model gpt-6-astra, ChatGPT login, codex exec -s read-only, session 01a149ea-398d-7d32-9f33-b1bcb492f7e5. Static review of 558f3686..5b64fc78 in a clean detached snapshot of 5b64fc7. It found that the Arrow close repair stays meaningful (removing the close at pyathena/arrow/result_set.py:243 fails it) and that the mocked GetQueryResults response omits no field that the S3FS paths read. Result: FINDINGS (3).
1 and 2 (P2): test_managed_results_preserve_api_values (tests/pyathena/s3fs/test_result_set.py:220) and test_reader_closes_at_end_of_results (:148) pass without their behavior. Both are unchanged from the base; deleting the managed branch at pyathena/s3fs/result_set.py:132-133 or the explicit close() at :237 passes them (confirmed offline). Deferred to #1255 so that this PR keeps its scope.
3 (P2): the _get_content_length patch in the Polars helper (tests/pyathena/polars/test_result_set.py:95). Deferred with the reason in the earlier thread: chunked CSV results are scanned by Polars' native reader at the output location, which must be a local path; redirecting that read at the Polars boundary would replace one private patch with a rewrite of polars.scan_csv arguments.
1 participant
WHAT
Apply the rows of #1246 to the Polars, Arrow, and S3FS cursor tests (
tests/pyathena/polars/,tests/pyathena/arrow/,tests/pyathena/s3fs/). Each row was re-checked on 713d57b, after #1233, #1236, 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/.as_arrow()wrapper tests and the row-count-only chunk and chunked fetch tests; the chunk-size bound joinstest_iter_chunks_data_consistency. The offline reader tests buildAthenaPolarsResultSetthrough its constructor over local result files (_local_result_set(), with the columns from a mocked GetQueryResults response) and read throughas_polars()anditer_chunks(). The mocked storage-options tests become a liveTestPolarsCursor::test_storage_options_replace_defaults, in which invalid credentials fail the whole CSV read through fsspec, and an offlineTestAthenaPolarsResultSet::test_storage_options_replace_defaults, which checks that the given options reach every Polars read of a chunked CSV or UNLOAD result unmerged, while PyAthena's own credential lookup fails; Polars ignores the options for local files, so those reads are observed.test_read_csv_invalid_timestampscovers the strict failure andignore_errors, replacingtest_to_datetimes_not_strict.as_polars()tests that checkpl.from_arrow()and the private timeout tests;test_as_polarskeeps one CSV query, of two rows and two columns. ATIMESTAMP(9)and a NULLTIMESTAMPcolumn namedxjoin the livetest_duplicate_column_names, and the fetch-across-batches checks jointest_fetch_across_empty_batches. The Parquet chunk reader tests no longer pin the lookahead count or PyArrow's byte ranges, and check that a streamed file is opened with the requested cache type.''tests, the basic reader queries, and the async copies of sync queries; the tab, newline, tab+newline, and comma values join the NULL/''query oftest_null_vs_empty_string, one SELECT per reader;test_executemanyruns onempty_tableinstead of being skipped; the reader tests of Python'scsvmodule are removed;test_csv_open_optionsbuilds the result set through its constructor and checks the documented 32 MiB blocks and two cached blocks.match=text that is not a documented contract and the imports that become unused.Verdicts changed from the issue after the checks:
arrow/test_cursor.py::TestArrowCursor::test_fetch_converts_each_value_once("a!",); only the call count shows it.arrow/test_async_cursor.py::TestAsyncArrowCursor::test_binary_null_vs_emptyAsyncArrowCursor.get_default_converteris its own code, and no other async Arrow test reads a converted CSV column.arrow/test_result_set.py::TestAthenaArrowResultSet::test_fetch_after_closeself._batches = iter([])from Arrow'sclose()passed every test, including the base cover. It now closes a 3-row result after one fetch.arrow/test_result_set.py::TestAthenaArrowResultSet::test_open_csv_closes_file_when_stream_failstest_csv_file_closed_when_stream_failspolars/test_result_set.py::TestPolarsDataFrameIterator::test_itertuples_without_column_namesPolarsDataFrameIteratoris in the public API documentation, and dropping the empty-row fallback fails only this test.s3fs/test_reader.py::{TestEmptyStringAsNullCSVReader, TestAthenaCSVReader}::test_closeclose()that does nothing passedtest_context_manager, whose rows had run out.test_context_managernow leaves the context after one row and checks that the file is closed.s3fs/test_cursor.py::TestS3FSCursor::test_contain_tab_charactertest_null_vs_empty_strings3fs/test_cursor.py::TestS3FSCursor::test_on_start_query_executiontest_connection.py::test_cursor_accepts_connection_callbackschecks only that the callback is stored.polars/test_result_set.py::{test_storage_options_replace_defaults, test_csv_storage_options_replace_defaults}test_as_polars_with_read_kwargstest_as_polars_with_read_kwargsstays: a test that only expects a failure passes if fsspec options reach Polars' native reader. The live UNLOAD read fails at the schema scan before the data reads, so the offline test observes each Polars read instead.After this change no test checks whether the Arrow timeouts reach
pyarrow.fs.S3FileSystem; the removed timeout tests did not check it either.The changes remove about 36 Athena queries per run (Polars 14, Arrow 7, S3FS 15), fewer than the issue's 44 because of the kept tests and the rewritten
test_executemany, which adds 6 (empty_tableDDL, 3 INSERTs, 1 SELECT).WHY
Closes #1246. 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, Polars 2.0.0. At 5b64fc7:
just format: no changes.just lint: passed.Offline, with
--noconftestand placeholder settings:polars/{test_result_set,test_converter,test_util}.py,arrow/{test_result_set,test_reader,test_converter}.py,s3fs/{test_result_set,test_reader}.py, and the sixtest_read_options: 189 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 5b64fc7, with no Test workflow running:
uv run --env-file .env pytest -n 4 tests/pyathena/polars tests/pyathena/arrow tests/pyathena/s3fs: 422 passed, 3 skipped (the three Polars 1.x cases oftest_unload_timestamps_with_read_options_polars_1), in 246 s, including the managed-results parameters.CI after Ready, at 5b64fc7: Test run 39034854811 passed (
test / run (3.15), lint).test-sqlaandtest-sqla-asyncwere skipped by thechangespath filter, since this PR changes no SQLAlchemy path.Not run: the live mutations of the covers that are live tests.
🤖 Generated with Claude Code