Skip to content

Do not convert ArrowCursor fallback values twice #1023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laughingman7743 merged 2 commits into from
Oct 3, 2026
Merged

Do not convert ArrowCursor fallback values twice #1023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70a1b87
Do not convert ArrowCursor fallback values twice
laughingman7743 Oct 3, 2026
808cc34
Resolve the Arrow converters when the rows are fetched
laughingman7743 Oct 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions pyathena/arrow/result_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ def __init__(
import pyarrow as pa

self._table = pa.Table.from_pydict({})
# The fetch methods convert only the values read from a result file.
# GetQueryResults values are already converted.
self._convert_rows = bool(self.output_location)

@laughingman7743 laughingman7743 Oct 3, 2026

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 regression, 3 pre-existing)

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a101ff-9276-7291-8c1f-de87bbbae0cc. Static review only. Snapshot: detached worktree at head 70a1b877bb8abb3d25f07fd1459ab9dc6ebbe548, base 33d3a0700b90f3fe73a98768bbc01d1581a17960. The prompt contained the literal diff and the intended behavior only. Afterwards, the snapshot and the PR worktree were clean at 70a1b877.

Covered (reviewer): S3 CSV, UNLOAD Parquet and metadata replacement, API fallback, failed and empty construction, sync/async/aio callers, fetch batching, exhaustion, and close, column names, converters, and the changed test and comment.

Findings and author verification:

  1. Regression, P2 pyathena/arrow/result_set.py:151: caching self.converters in __init__ ignores converter.set(...) calls made after execute(), whereas the base resolved the converters at fetch time. Verified by reading the code. Action: fix.
  2. Pre-existing, P2 pyathena/result_set.py:748: _rows_to_columnar() keys the columns by name, so on the fallback SELECT 1 AS x, 2 AS x collapses into one column. Verified by reading the code. Action: report to the maintainer; out of scope.
  3. Pre-existing, P2 pyathena/result_set.py:721: _fetch_all_rows() checks every page for a header row, while _pre_fetch() checks only the first page. A later page whose first row equals the column names loses that row. Verified by reading the code. Action: report to the maintainer; out of scope.
  4. Pre-existing, P3 pyathena/arrow/result_set.py:432: close() assigns a list to _batches, so a later _fetch() raises TypeError. Already noted in round one; out of scope.

@laughingman7743 laughingman7743 Oct 3, 2026

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: 808cc34 (finding 1)

__init__ now records only self._convert_rows = bool(self.output_location). _fetch() resolves self.converters once per record batch when it converts rows, and leaves the rows unchanged on the fallback. A converter.set() after execute() reaches the next fetched batch again, and the converters are still built once per batch instead of once per cell.

Validation:

  • just lint passed.
  • Live, S3 path: with DefaultArrowTypeConverter, execute(json_parse(...)), then converter.set("json", lambda v: ("raw", v)), fetchone() returned (('raw', '{"a":1}'),).
  • tests/pyathena/arrow and tests/pyathena/aio/arrow: 105 passed, including test_fetch_all_rows[managed].

Self-review of the repair: round one (behavior) and round two (claims) found nothing. The comment ("The fetch methods convert only the values read from a result file") matches both branches. The fix is now one boolean, with no cached state that a converter change can miss.

@laughingman7743 laughingman7743 Oct 3, 2026

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 (relayed): CLEAN for 70a1b877..808cc34ffaec7b2d433d7232966a6716bd0bdc5a.

Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a10207-a155-73d2-a876-602195cfec74. Static review; afterwards, the snapshot was clean at 808cc34f. Covered: S3 CSV/TXT, UNLOAD Parquet and schema metadata, the GetQueryResults fallback, failed and empty construction, converter mutation, batch buffering, fetch methods, exhaustion, close, and the comment. The S3 converters are resolved per fetched batch, so a converter.set() after execute() applies again. Rows already buffered keep their conversion, as on master. Fallback rows get no second conversion. No regressions.

self._batches = iter(self._table.to_batches(arraysize))

def _create_s3_file_system(self):
Expand Down Expand Up @@ -251,11 +254,15 @@ def _fetch(self) -> None:
return
else:
dict_rows = rows.to_pydict()
column_names = dict_rows.keys()
processed_rows = [
tuple(self.converters[k](v) for k, v in zip(column_names, row, strict=False))
for row in zip(*dict_rows.values(), strict=False)
]
if self._convert_rows:
converters = self.converters
column_names = dict_rows.keys()
processed_rows = [
tuple(converters[k](v) for k, v in zip(column_names, row, strict=False))
for row in zip(*dict_rows.values(), strict=False)
]
else:
processed_rows = list(zip(*dict_rows.values(), strict=False))
self._rows.extend(processed_rows)

@override
Expand Down
15 changes: 13 additions & 2 deletions tests/pyathena/arrow/test_cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,5 +969,16 @@ def test_null_vs_empty_string(self, arrow_cursor):
indirect=["arrow_cursor"],
)
def test_fetch_all_rows(self, arrow_cursor):
arrow_cursor.execute("SELECT 1 AS col")
assert arrow_cursor.fetchall() == [(1,)]
arrow_cursor.execute(
"""
SELECT
1 AS col
,CAST('12:34:56' AS TIME) AS col_time
,X'0102' AS col_varbinary
,json_parse('{"a": 1}') AS col_json
,CAST('{"a": 1}' AS JSON) AS col_json_string
"""
)
assert arrow_cursor.fetchall() == [
(1, datetime(2017, 1, 1, 12, 34, 56).time(), b"\x01\x02", {"a": 1}, '{"a": 1}')
]
Loading