Skip to content

Read managed query results as a CSV result file in the pandas, Arrow, and Polars cursors - #1061

Merged
laughingman7743 merged 3 commits into
masterfrom
fix/1028-managed-csv-parity
Oct 4, 2026
Merged

laughingman7743 merged 3 commits into
masterfrom
fix/1028-managed-csv-parity

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

WHAT

With managed query result storage, PandasCursor, ArrowCursor, and PolarsCursor (and their async and aio variants, which share the result sets) now read the GetQueryResults rows as they read a CSV result file. The types and values match the S3 path, and the cursor's converter applies, including a custom one.

  • AthenaResultSet._fetch_all_rows_as_csv() fetches all pages and writes them in the format of Athena's CSV result file. The format is a quoted header of the column labels, each value quoted with its quotes doubled, NULL as an empty unquoted field, and \n after each row. Each cursor's _read_csv() reads these bytes in place of the S3 file, with the same options. That includes the pandas binary NULL handling, na_values, parse_dates, the PyArrow-engine reader, and the column labels for repeated names (Keep columns with the same name, and key CSV column types by the reader's column labels #1050); the Arrow column_types; and the Polars schema_overrides and new_columns.
  • The managed-only code is removed: _as_pandas_from_api(), _as_arrow_from_api(), _as_polars_from_api(), _text_value_converter()/_TEXT_VALUE_TYPES, _text_value_converters(), Arrow's _convert_rows, and the pandas _INTEGER_TYPES dtypes from Keep integer and JSON values exact in PandasCursor results with NULL #1044.
  • Managed results are still read as a whole, not in chunks, as before.
  • _fetch_all_rows() (used by S3FSCursor) and the new method share _iter_all_row_pages(), which skips the column labels only on the first page, as master does since Keep columns with the same name, and key CSV column types by the reader's column labels #1050.
  • Timestamps. Athena writes up to 12 fractional digits; Athena reports precision 3 for every timestamp column, so the readers cannot pick a unit per column. The old managed path truncated the fractions to microseconds through DefaultTypeConverter.
    • ArrowCursor: DefaultArrowTypeConverter reads timestamp as timestamp[us] instead of timestamp[ms]. Timestamp columns with a timestamp type are always read as text and cast by _to_timestamp(), which truncates the text when a value is longer than the unit holds and treats an empty string as NULL. pyarrow does not parse fractions finer than the unit. On Linux, the "%Y-%m-%d %H:%M:%S %Z" strptime fallback of timestamp_parsers accepts such a value and drops the whole fraction (measured with pyarrow 25.0.1 in python:3.14-slim; macOS raises instead).
    • _to_timestamp() (pyathena/arrow/converter.py) and _to_datetimes() (pyathena/polars/converter.py) sit with the other conversions; the text lengths per unit are shared in pyathena.converter.
    • PolarsCursor: when a whole read raises ComputeError (Polars 1.39.0 and 1.44.2), the timestamp columns with a Datetime dtype are read again as text. _to_datetimes() truncates and parses them with an explicit format, with an empty string as NULL. They are not read as text when execute() was given schema_overrides or with_column_names, or for headerless .txt results; those reads fail as before. Unselected columns are skipped. Chunked scan_csv() reads are unchanged; managed results are not read in chunks.

Release notes (behavior changes, 4.0.0)

  • Managed query result storage, pandas/Arrow/Polars cursors: the results have the types and values of an S3 result file, and the cursor's converter applies to them, including a custom one (Custom converters do not apply to pandas, Arrow, and Polars results with managed query result storage #1028). For example:
    • pandas: date is datetime64 instead of datetime.date; array/map/row are the Athena text ('[1, 2]', '{k=1}') instead of parsed lists and dicts (PandasCursor returns different date, array, map, and row values on managed query result storage #1042); an empty string is NaN with the default na_values.
    • Arrow: as_arrow() columns have the converter's column_types, for example date as timestamp[ms] and decimal, time, and array as strings. Polars: as_polars() columns have the converter's schema_overrides, for example decimal as Decimal(p, s) with the column's precision and scale. In both, the fetch methods apply the converter as on the S3 path.
    • result_set_type_hints no longer applies to these cursors on managed storage, as on the S3 path.
    • A result without rows has its columns. The results of UPDATE, DELETE, MERGE, and VACUUM are empty, as on the S3 path.
    • Polars new_columns given to execute() now rename managed results too.
  • ArrowCursor: timestamp columns are timestamp[us] (as_polars(): Datetime("us")) instead of timestamp[ms]. date stays timestamp[ms].
  • ArrowCursor and PolarsCursor (unless chunked) read timestamps with more than 6 fractional digits, truncated to the type's unit (microseconds by default), on both paths. Before, on the S3 path, Arrow raised for more than 3 digits on macOS and dropped the whole fraction on Linux. Polars raised for 12 digits (measured; 9 read).

Known limits

  • Managed storage reads every row through GetQueryResults inside execute(), as before.
  • ArrowCursor parses timestamp columns as text: the local parse of a 1M-row × 8-column file with one timestamp column took 116–134 ms instead of 88–104 ms (S3 download not included).
  • PolarsCursor reads a result with more than 6 fractional digits twice; from S3, that means downloading the file again. Other parse errors of a result with timestamp columns also read the data a second time before they raise. Chunked PolarsCursor reads of S3 results still fail on 12 fractional digits, as before.

WHY

Closes #1028. Closes #1042.

The maintainer chose this design on 2026-10-04: render the managed rows as the CSV result file and read them with the same readers, instead of keeping the mapped columns as text (the option in #1028). The maintainer also chose to fold #1042 into this PR and to fix the Arrow timestamp(6) parsing here. Then:

  • The independent review found that managed TIMESTAMP(7)–(12) regressed, and the maintainer chose to fix it here too.
  • The first AWS CI run showed that Arrow's re-read-on-failure did not work on Linux, because the strptime fallback accepts the value. The maintainer chose to always read Arrow timestamps as text.

Measured on Athena (28 types plus an all-NULL row): the GetQueryResults VarCharValue text equals the CSV result file byte for byte for every type. The types were boolean, integers, real/double, char, varchar with commas, quotes, and newlines, the empty string, date, timestamp(3)/(6), time, time and timestamp with time zone, decimal, varbinary, array, map, row, json, interval, ipaddress, uuid, and a nested array.

The branch was squashed into one commit and rebased onto master 8c9c201 (after #1050, #1057, and #1060), and then onto a89a4f7 after #1066. The maintainer chose to merge #1066 (#1051) first: through the shared readers, managed results would otherwise fail on columns with the same name and different types, as S3 results did. The review records below refer to the earlier commits 03c9f3f, 46bf916, 301b22e, and ec2bad9.

TEST

Tested commit: d376f51 (base a89a4f7).

  • just lint: passed. just docs lint: passed (on 03c9f3f; the docs have not changed since).
  • New test_managed_results_match_result_file for pandas, Arrow, and Polars runs one query on the S3 path and on the managed work group, both with a custom varchar converter. It asserts equal as_*() output (schema and values; pandas assert_frame_equal) and fetched rows, plus the converted value. Arrow and Polars also assert the truncated TIMESTAMP(6) and TIMESTAMP(12) values, and Arrow asserts timestamp[us]. On master these fail: the managed path ignores the converter, and Arrow fails on TIMESTAMP(6) (macOS) or drops its fraction (Linux).
  • New offline tests:
    • TestAthenaResultSet.test_fetch_all_rows_as_csv: the CSV text (quoting, NULL, the empty string, and a labels-equal row on page 2) with GetQueryResults mocked.
    • test_to_timestamp (Arrow; units s/ms/us) and test_to_datetimes (Polars; Datetime, "ms", "us"): 0 to 12 fractional digits, year 1, NULL, and an empty string; an unselected column for Polars. _to_timestamp() was also run on Linux (pyarrow 25.0.1, python:3.14-slim) with the same values.
    • TestAthenaArrowResultSet.test_read_csv_timestamps_with_the_same_name: timestamp columns converted by position when names repeat.
    • TestAthenaPolarsResultSet.test_read_csv_truncates_timestamps: the retry through _read_csv() with the rows mocked; with columns, with new_columns (renamed after reading), and with with_column_names (fails as before).
  • On d376f51 (moves the two conversions to the converter modules): offline 223 passed; pytest -n 4 tests/pyathena/{arrow,polars} tests/pyathena/aio/{arrow,polars} -k "managed or timestamp or duplicate or fetch_all_rows or as_arrow or as_polars": 57 passed.
  • On 1878a8c: uv run --env-file .env pytest -n 6 tests/pyathena/pandas tests/pyathena/arrow tests/pyathena/polars tests/pyathena/s3fs tests/pyathena/aio/{pandas,arrow,polars,s3fs} tests/pyathena/test_result_set.py tests/pyathena/test_converter.py: 894 passed, 1 skipped (s3fs/test_cursor.py:248, which needs a table that the test environment does not have). This includes Read columns with the same name by their own types from CSV result files #1066's test_duplicate_column_names with its managed params, which repeat names with different types.
  • The first AWS CI run (on 301b22e) failed TestArrowCursor.test_managed_results_match_result_file on Linux: TIMESTAMP(12) came back without its fraction. That led to the Arrow change above.
  • Manual checks on Athena (before the rebase):
    • 27 types plus an all-NULL row, with a custom varchar converter: pandas, Arrow, and Polars give identical as_*() and fetchall() on both paths.
    • SELECT 1 AS a, 'x' AS b WHERE false gives columns a, b and no rows for all three cursors on both paths. On master, the managed path returned no columns (_as_*_from_api() returned an empty frame when there were no rows; from the source, not measured).
  • Offline benchmarks (local parsing only, S3 download not included, medians):
    • Managed read with GetQueryResults mocked, 200k rows × 8 columns, before the rebase, master → this branch: Arrow 3.40 s → 0.59 s, pandas 2.52 s → 1.29 s.
    • Arrow timestamp text parse: see Known limits.

Not run locally: the rest of just test pyathena and the SQLAlchemy suites; CI runs them once the PR is Ready.

🤖 Generated with Claude Code

@laughingman7743 laughingman7743 added this to the 4.0.0 milestone Oct 4, 2026
Comment thread pyathena/result_set.py
columns: list[str],
) -> dict[str, list[Any]]:
"""Convert row-oriented data to columnar format.
def _fetch_all_rows_as_csv(self) -> bytes:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round 1 (implementation behavior): CLEAN

Base a57325b (merge-base with master), head 03c9f3f, full diff (14 files).

Covered:

  • Behavior and failure paths. __init__ of the pandas, Arrow, and Polars result sets: the managed branch now calls _read_csv(); failed and cancelled queries still get empty results. Read errors are now wrapped in OperationalError, as on the S3 path; GetQueryResults errors propagate as before. The async and aio cursors construct the same result sets, so they share these paths. S3FSCursor keeps _fetch_all_rows() with its own converter and type hints.
  • Data boundaries. Quoting of ", ,, and \n in values and labels; NULL as an empty unquoted field vs ""; a single NULL column as an empty line (Arrow ignore_empty_lines=False, pandas skip_blank_lines=False); empty results (header only gives the columns, as with the S3 file); no description (DML substatements) gives b"" and an empty result. pandas: binary NULL via BinaryCSVReader over a StringIO(newline=""); storage_options popped, because pandas raises ValueError for it with a buffer (checked offline). Polars: storage_options removed; Polars ignores it for bytes but the intent is explicit. _finish_csv_frame() restores json NULL and truncates time on the managed path too.
  • Chunking. Managed reads stay unchunked: pandas forces effective_chunksize=None and skips auto-optimization, and Polars takes the _df branch. iter_chunks()/as_*() semantics are unchanged for managed storage, as their docstrings describe.
  • Simplicity. The managed-only helpers are removed. _iter_all_row_pages() is the single pagination loop for _fetch_all_rows() and the CSV writer. The metadata check sits in both callers, because the generator would raise only lazily.
  • Tests. The parity tests compare each cursor's managed results with its S3 results, plus a converted value, so they fail on master (converter ignored; Arrow TIMESTAMP(6) raises). The offline CSV test covers quoting, NULL, the empty string, and a labels-equal row on page 2 (measured live: master returns 1000 of 1001 rows).

Pre-existing, out of scope: fractions with 7 to 12 digits on Arrow (and 12 on Polars) fail on both paths; recorded under Known limits in the PR body.

Comment thread docs/usage.md
With managed query result storage, query results are retrieved via the `GetQueryResults` API
(1000 rows per request) instead of reading S3 files directly. This may be slower for large
result sets. For large datasets, consider using customer-managed storage or the `UNLOAD` statement.
The pandas, Arrow, and Polars cursors read these rows as they read a CSV result file, with the

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round 2 (claims, callers, operations): FINDINGS, fixed in the PR body

Base a57325b, head 03c9f3f. Claims came from the PR body, the commit message, docstrings and comments, docs/usage.md, and docs/arrow.md.

Findings and corrections (PR body only, no code change):

  1. The release note said Arrow and Polars read date as a timestamp and decimal as a string. DefaultPolarsTypeConverter maps date to pl.Date and decimal to pl.Decimal(p, s), so that was false for Polars. The note now describes Arrow's column_types and Polars' schema_overrides separately.
  2. The Known limits said Polars fails on 7 to 12 digits. Only 12 digits was measured to fail; 9 digits reads. It is narrowed to the measurement, and the Arrow 9-digit failure is labeled as measured offline with pyarrow.
  3. The TEST evidence (114/116/12 passed) came from the tree before the final tidy of _fetch_all_rows()/_fetch_all_rows_as_csv(). Rerun on 03c9f3f: the combined targeted run, 190 passed, and the Arrow suite, 116 passed. The body now cites only those.

Verified claims:

  • GetQueryResults text equals the CSV file for 28 types: measured earlier on Athena.
  • The async and aio cursors share the result sets: pyathena/aio/{pandas,arrow,polars}/cursor.py construct the same Athena*ResultSet classes.
  • Empty results have their columns on both paths: measured on Athena for all three cursors. The claim that master had none comes from the source and is labeled as such.
  • The labels-equal row on page 2 was dropped on master (1000 of 1001 rows) and is kept here: measured with S3FSCursor and PandasCursor.
  • result_set_type_hints: master's managed path passed hints through _get_rows(), and the S3 path never used them, so the note is accurate. The docs line in the Constraints section is updated.
  • Docs: no other page states the old managed conversion. docs/aio.md (rows read inside execute()) stays true.

Callers and operations: the removed _as_*_from_api(), _rows_to_columnar(), and _text_value_converter() are private, and only this package calls them. GetQueryResults traffic is unchanged: the same MaxResults=1000 pagination from the start, after the one-row pre-fetch, with the same retry config. Peak memory is the CSV text (list, joined string, and bytes) in place of the Python objects of every converted value.

self._table = self._as_arrow_from_api()
# Without a result file, as with managed query result storage, the rows from
# GetQueryResults are read as a CSV result file.
self._table = self._read_csv()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent review (relayed): FINDINGS (1)

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, codex exec -s read-only, session 01a104da-4dc0-74d2-9bea-0f16596844ec. Static review only: no tests, builds, or network. Snapshot: detached worktree at head 03c9f3f, diff from base a57325b. The prompt had no PR number, description, or prior findings. The snapshot and the PR worktree stayed unchanged (both at 03c9f3f, clean).

Covered: pagination and the header row; CSV quoting and newlines; NULL vs the empty string; empty and no-column results; DML and DDL; the pandas, Arrow, and Polars dtypes and converters; binary, JSON, and temporal values; duplicate and odd names; read options; sync, threaded async, and aio callers; exceptions, cursor state, fetch and close, resource ownership; tests and docs.

P2: Managed Arrow results regress for timestamps exceeding microsecond precision (pyathena/arrow/result_set.py:145). For a managed query returning TIMESTAMP(9) text such as 2020-01-01 00:00:00.123456000, the new CSV path uses timestamp[us]. Arrow's parser rejects fractions longer than six digits, even trailing zeros, so result-set construction raises OperationalError. Previously, the API fallback used _parse_datetime(), which truncates fractions to six digits. Classification: a newly introduced regression for managed results; the S3 CSV limitation already existed. The new comparison test covers only 3- and 6-digit timestamps.

The reviewer found that the added tests assert observable bytes, schemas, values, and custom conversion, and can fail against the old implementation.

Author verification: confirmed. Master's managed Arrow path converted with DefaultTypeConverter (_parse_datetime() accepts 7 to 12 digits). pyarrow rejects more than 6 digits for timestamp[us], as measured offline. The PR body already lists this under Known limits; the resolution is pending the maintainer's decision.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Repair: 46bf916 (the maintainer chose to fix this in this PR rather than defer it)

  • ArrowCursor (pyathena/arrow/result_set.py): when csv.read_csv() raises ArrowInvalid and the result has timestamp columns with a timestamp type, the data is read again with those columns as text. _to_timestamp() truncates the text to the unit's length (s 19, ms 23, us 26, ns 29), turns "" into NULL (the CSV NULL with strings_can_be_null=False), and casts it. The successful parse path is unchanged.
  • PolarsCursor (pyathena/polars/result_set.py): _get_timestamp_dtypes() picks the timestamp columns whose dtype is Datetime. They are read as pl.String, and _to_datetimes() slices and parses them (%Y-%m-%d %H:%M:%S%.f, the dtype's time unit) for read_csv() and lazily for chunked scan_csv(). Skipped: schema_overrides given to execute(), headerless .txt results, and columns absent from the frame.

Self-review of the repair

  • Round 1 (behavior):
    • A NULL timestamp is null with strings_can_be_null=True (binary columns present) and "" otherwise. Both give NULL (offline check with and without a varbinary column).
    • The binary fill_null("") loop runs after the conversion, so the converted columns are no longer strings and are skipped.
    • The retry opens a new stream (S3) or BufferReader (managed). A second failure is wrapped in OperationalError as before.
    • Polars failures inside the chunk iterator are raised within the existing try and wrapped in OperationalError.
    • Cost: the first design (always text) made Arrow's local whole-file parse +25–40% (1M rows × 8 columns), so Arrow reads as text only after a failure. Polars' explicit-format parse is faster than the inferred one (133–144 → 81–83 ms).
  • Round 2 (claims): the PR body now states the fix, the pre-PR thresholds (Arrow failed with more than 3 digits under timestamp[ms]; Polars with 12, measured, while 9 read), and the double read on failure, and drops the old known limit. The commit message's claim of no cost in the common case matches the unchanged first read.
  • Tests: test_managed_results_match_result_file (Arrow and Polars) now includes TIMESTAMP(12) on both paths and asserts the truncated values; removing the retry fails it. New offline tests: test_to_timestamp and test_to_datetimes. On 46bf916: 251 passed (Arrow and Polars, including aio), 105 passed (pandas, S3FS, aio pandas, filtered), 24 offline passed.

An independent follow-up review of the repair is pending.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up review (relayed): FINDINGS (2)

Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, codex exec -s read-only, session 01a104ee-cfcd-71e3-9817-e72a9564e3ac. Static review of 03c9f3f..46bf916 on a detached snapshot at 46bf916 (left unchanged). It found the earlier Arrow precision finding resolved for the default converter.

  1. P2: Renamed Polars timestamp columns remain strings (pyathena/polars/result_set.py:70). With new_columns=["renamed"], t is read as String, Polars renames it, and _to_datetimes() cannot find t, so as_polars()["renamed"] holds strings. Introduced by the repair.
  2. P2: The missing-string option now breaks NULL timestamps (pyathena/polars/result_set.py:77). With missing_utf8_is_empty_string=True, a NULL timestamp read as String becomes "", and strict str.to_datetime() raises, so the read fails with OperationalError (eager and chunked). Introduced by the repair.

Author verification: both reproduce offline on Polars 1.44.2. The same happens with empty_string_is_null=False, the name that missing_utf8_is_empty_string=True became in 1.43.

Repair: 301b22e. Polars now follows the Arrow design: the first read is unchanged, and only when read_csv() raises ComputeError (measured on Polars 1.39.0 and 1.44.2) are the timestamp columns read again as text. _to_datetimes() treats "" as NULL (finding 2). The text read is skipped when execute() was given new_columns or with_column_names (finding 1); such a read fails as before this PR instead of returning strings. Chunked scan_csv() is back to the master code, because managed results are never chunked. This leaves the pre-existing 12-digit failure of chunked S3 reads, listed under Known limits.

Self-review of the repair:

  • Round 1: common reads are byte-for-byte the old path. The retry rebuilds schema_overrides from the result set's dtypes, because the text read is skipped whenever the user gave schema_overrides. Errors in the retry are wrapped in OperationalError.
  • Round 2: the PR body (WHAT, release notes, Known limits, TEST) is updated. The commit message says empty_string_is_null; the exact case is empty_string_is_null=False, formerly missing_utf8_is_empty_string=True.
  • Tests: test_to_datetimes adds "". The new test_read_csv_truncates_timestamps drives the retry through _read_csv() offline, with columns and with new_columns. On 301b22e: live Polars, Arrow, and aio 251 passed; offline 24 passed.

A second independent follow-up on 46bf916..301b22e is pending.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up review #2 (relayed): CLEAN

Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, codex exec -s read-only, session 01a104f8-7629-7b71-b1b7-014f91a9df99. Static review of 46bf916..301b22e on a detached snapshot at 301b22e (left unchanged).

Covered: retry eligibility and OperationalError wrapping; S3 path vs in-memory bytes on the re-read; read kwargs precedence; column selection and renaming; Datetime classes and ms/us/ns/time-zone handling; the chunked-path reversion; Arrow's analogous retry; the changed tests.

Both previous findings are resolved: the initial read uses the original dtypes and new_columns disables the fallback (pyathena/polars/result_set.py:446); the fallback maps "" to null before strict parsing (:73). The retry is bounded to one additional read; S3 paths are reopened, and managed bytes are reused without refetching rows. The excluded override and headerless cases and the chunked precision failures keep their baseline limitations; they are not new regressions.

Test gaps noted: no direct coverage of the S3 retry, the empty-string read option end-to-end, nanoseconds, or zoned dtypes.

Author note on the gaps: test_managed_results_match_result_file (Arrow and Polars) reads TIMESTAMP(12) on the S3 path too, so it goes through the S3 retry live. The empty-string mapping is covered by test_to_datetimes; I did not add an end-to-end case, because the option's name differs across the supported Polars versions (missing_utf8_is_empty_string before 1.43, empty_string_is_null after). Nanosecond and zoned dtypes come only from custom converters and are not covered.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 03:36
@laughingman7743
laughingman7743 marked this pull request as draft October 4, 2026 03:48
@laughingman7743
laughingman7743 force-pushed the fix/1028-managed-csv-parity branch from 301b22e to ec2bad9 Compare October 4, 2026 04:11
Comment thread pyathena/result_set.py

if not next_token:
break
def _fetch_all_rows_as_csv(self) -> bytes:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round 1 after rebase and the Arrow change: CLEAN

Base 8c9c201 (merge-base with master after #1050, #1057, #1060), head ec2bad9, full diff (16 files). This is a full pass: the branch was squashed and rebased, with conflicts in all four result set modules and two test files.

Conflict resolution checked against master's new contracts:

The Arrow change (always text): the timestamp columns, read as string, are cast by _to_timestamp() before the binary fill_null("") loop, so they are not filled. A tz-aware custom timestamp type fails both before and after (direct parse and cast both reject naive text; checked offline). Duplicate names: the set_column loop converts every field with the name.

Tests on ec2bad9: 885 passed, 1 skipped (pre-existing @pytest.mark.skip), across pandas, Arrow, Polars, S3FS, their aio variants, the base result set, and the converter tests. Offline: 219 passed.

Comment thread pyathena/arrow/result_set.py Outdated
}

try:
# Timestamp columns are read as text: pyarrow does not parse fractions finer

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round 2 after rebase and the Arrow change: CLEAN after corrections to the PR body

Base 8c9c201, head ec2bad9. Claims checked:

  • "On Linux, the %Z strptime fallback accepts a value with a finer fraction and drops it": measured in python:3.14-slim with pyarrow 25.0.1, using master's timestamp_parsers. With timestamp[ms], .123 → .123000 and .123456 → 03:04:05, so master already silently drops TIMESTAMP(6) fractions on Linux. With timestamp[us], 7 to 12 digits drop the fraction the same way.
  • "Polars raises ComputeError": measured on 1.39.0 and 1.44.2.
  • The Arrow cost (88–104 → 116–134 ms local parse for 1M × 8 columns with one timestamp column): measured locally, medians of 7, interleaved.
  • The release note on the labels-equal later-page row was removed from this PR: master fixed it in Keep columns with the same name, and key CSV column types by the reader's column labels #1050 (d724287).
  • new_columns now renames managed Polars results: from _get_frame_column_names() and _get_csv_params(); covered offline by test_read_csv_truncates_timestamps[new_columns].
  • The TEST section cites only runs on ec2bad9, except the manual Athena checks and the managed-read benchmark, which are labeled as before the rebase.

Callers: no public signature changed. timestamp_parsers and column_types are unchanged, and only the internal read uses string for timestamp columns. AWS traffic is unchanged.

self._table = self._as_arrow_from_api()
# Without a result file, as with managed query result storage, the rows from
# GetQueryResults are read as a CSV result file.
self._table = self._read_csv()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent review after rebase (relayed): FINDINGS (1)

Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, codex exec -s read-only, session 01a1051e-1a39-7253-8146-7d763045a88a. Static full review of 8c9c201..ec2bad9 on a detached snapshot (unchanged). The prompt carried no PR number, description, or earlier findings.

Covered: API pagination; CSV quoting and newlines; NULL vs empty values; empty and no-column results; DML and DDL; the pandas, Arrow, and Polars types, converters, and timestamp handling; duplicate labels; reader kwargs; sync, Future-based async, and aio callers; exceptions, cursor state, fetch and close, resource ownership; S3-path performance; test sensitivity to the old implementation.

P2: Managed Arrow results now fail for duplicate labels with different types (pyathena/arrow/result_set.py:171). Managed results go through CSV parsing, whose dtype mapping is keyed by column name. With SELECT 'abc' AS x, 1 AS x, the second column overwrites the first column's dtype with int32, both CSV columns parse as integers, and execute() raises OperationalError. Previously, _as_arrow_from_api() built columns positionally and returned ('abc', 1). A newly introduced managed-results regression exposing a pre-existing S3-path defect.

Author verification: confirmed from the source. It is the S3-path defect filed as #1051 (pandas dtype and pyarrow column_types keyed by name). Its table shows the managed path working on master for pandas and Arrow. This PR moves managed reads onto the same reader, so pandas regresses the same way, not only Arrow. A fix of #1051 in the shared readers fixes both paths. Handling is pending the maintainer's decision; another session has a fix/1051-duplicate-column-types worktree.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Repair: rebased onto master a89a4f7 (#1066, fixing #1051) → fd80f98, plus test 1878a8c. The maintainer chose to merge #1051 first and rebase this PR onto it.

An independent follow-up on ec2bad9 → 1878a8c is pending.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up review (relayed): CLEAN

Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, codex exec -s read-only, session 01a10569-0548-7af2-b7cf-1d20d4ced045. Static review of git range-diff 8c9c201a..ec2bad9d a89a4f7d..1878a8c8 and the full current diff a89a4f7d..1878a8c8b4349acadb61f0f390325537aeeb1466, on a detached snapshot at 1878a8c (left unchanged).

Covered: managed CSV rendering and pagination; pandas label resolution, header skipping, and the in-memory binary stream; Arrow positional types, timestamp conversion, binary NULL handling, and renaming; .txt parsing; related Polars changes and tests.

The previous finding is resolved for both managed pandas and Arrow results. Arrow keeps master's #1051 positional names, types, and binary columns, skips the CSV header as a parsed row, converts timestamps by position, then restores repeated names (pyathena/arrow/result_set.py:341). pandas applies the resolved labels to per-column options and keeps master's header-as-labels handling. The binary configuration comes before the header skipping, and the managed binary stream keeps the original header and newlines (pyathena/pandas/result_set.py:844). No actionable regression found in repeated timestamp names, timestamp and binary combinations, .txt handling, or the managed-source interactions.

laughingman7743 and others added 2 commits October 4, 2026 14:24
With managed query result storage, the pandas, Arrow, and Polars cursors
built their results from GetQueryResults values converted by
DefaultTypeConverter, so the cursor's converter, dtypes, and date parsing
did not apply and the results differed from those of an S3 result file.

GetQueryResults returns the same text for each value as the CSV result
file, so the rows are now written in the format of that file and read
with the same reader. The cursor's converter applies to them, including
a custom one, and the types and values match the S3 path.

Athena writes up to 12 fractional digits for timestamp columns, which
the GetQueryResults fallback used to truncate to microseconds:

- ArrowCursor reads timestamp columns as timestamp[us] instead of
  timestamp[ms], and always reads them as text and casts them,
  truncating the text when a value is longer than the unit holds. A
  6-digit fraction failed to parse as timestamp[ms], and on Linux the
  "%Y-%m-%d %H:%M:%S %Z" strptime fallback accepted such values without
  their fraction.
- PolarsCursor reads the timestamp columns again as text when a whole
  read fails to parse them, and truncates them to the time unit.

Closes #1028
Closes #1042

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 force-pushed the fix/1028-managed-csv-parity branch from ec2bad9 to 1878a8c Compare October 4, 2026 05:34
@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 05:48
_to_timestamp() and _to_datetimes() convert values, so they belong with
the other conversions in the Arrow and Polars converter modules rather
than in the result sets. The text lengths for each time unit are shared
in pyathena.converter.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 marked this pull request as draft October 4, 2026 06:10
}


def _to_timestamp(column: ChunkedArray, type_: TimestampType) -> ChunkedArray:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review of d376f51 (move to the converter modules), rounds 1 and 2: CLEAN

The maintainer asked to keep conversions out of the result sets, as in #1010. Range: 1878a8c..d376f51 (base a89a4f7 unchanged).

  • Round 1 (behavior):
    • _to_timestamp() moved byte for byte to pyathena/arrow/converter.py, and _to_datetimes() to pyathena/polars/converter.py; checked with a diff of the old and new function bodies. The only change is one comment that named execute(), now generic.
    • _TIMESTAMP_TEXT_LENGTHS is defined once in pyathena/converter.py (s/ms/us/ns). Polars looks up only ms/us/ns, the Datetime units.
    • The result sets import the converter modules; the converter modules do not import the result sets, so there is no import cycle (both import cleanly).
    • pyarrow and polars are still imported inside the functions, and only for type checking at module level, as for the other optional-dependency code.
    • They stay module functions, not methods of the default converters, because a custom converter need not subclass them.
  • Round 2 (claims): the commit message and the PR body (WHAT, tested commit) name the new locations. The unit tests moved to tests/pyathena/{arrow,polars}/test_converter.py, mirroring the source.
  • Tests on d376f51: offline 223 passed (the same tests, relocated). Live Arrow, Polars, and their aio variants filtered to managed/timestamp/duplicate/fetch: 57 passed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent review of d376f51 (relayed): CLEAN

Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, codex exec -s read-only, session 01a10589-d007-7432-b6d5-dd9a514d0337. Static review of 1878a8c..d376f51 on a detached snapshot (left unchanged).

Covered all nine changed files. The helper bodies preserve behavior; Polars changes only a comment. The shared timestamp lengths keep all the values used before. The result set callers and the relocated tests import the helpers from the converter modules, and nothing references the old locations. No import cycles. Deferred annotations, TYPE_CHECKING imports, and lazy optional-dependency imports follow the existing conventions. The moved tests keep identical parameters, inputs, and assertions. The placement matches the converter modules' responsibilities and the source-mirroring test layout. CLEAN: no actionable findings.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 06:14
@laughingman7743
laughingman7743 merged commit d989767 into master Oct 4, 2026
14 checks passed
@laughingman7743
laughingman7743 deleted the fix/1028-managed-csv-parity branch October 4, 2026 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant