Skip to content

Keep NULL rows of single-column CSV results in ArrowCursor #1031

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laughingman7743 merged 1 commit into from
Oct 3, 2026
Merged

Keep NULL rows of single-column CSV results in ArrowCursor #1031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion pyathena/arrow/result_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,8 @@ def _read_csv(self) -> Table:
parse_opts = csv.ParseOptions(
delimiter=",",
quote_char='"',
ignore_empty_lines=not binary_columns,
# Athena writes a single-column row with a NULL value as an empty line.

@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.

Self-review round two (claims, callers, operations)

Base de8cc52ac2a43cdba72bb4571c185883287741fc, head 6b924fe7161e6d0e4b61bc601beeadb78f075793.

Result: FINDINGS (PR description only; no code change).

Claims checked:

  • Affected since v2.7.0 — git show v2.7.0:pyathena/arrow/result_set.py builds the CSV ParseOptions without ignore_empty_lines (pyarrow default True). Holds.
  • Except for varbinary columns — incorrect as written: ignore_empty_lines is absent in v3.34.0–v3.36.0 and present from v3.37.0 (and on 3.x). The release note now says v3.37.0 kept the rows only for varbinary columns and that 3.x has the same code.
  • Async/aio share the result set — pyathena/arrow/async_cursor.py:166 and pyathena/aio/arrow/cursor.py:177 construct AthenaArrowResultSet. Holds; their tests ran in the 106-test local run, but the new regression test is sync only.
  • NULL string → '' — matches docs/null_handling.md (ArrowCursor CSV row); no documentation becomes obsolete.
  • Evidence revision — the full Arrow run used an earlier test draft on the same source; the TEST section now says so and names the run on 6b924fe.

Callers/operators: no API, default, or exception change; no extra AWS requests (one CSV read as before). Callers that relied on dropped rows now see the NULL rows, which is the fix.

ignore_empty_lines=False,

@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.

Self-review round one (implementation behavior)

Base de8cc52ac2a43cdba72bb4571c185883287741fc, head 6b924fe7161e6d0e4b61bc601beeadb78f075793.
Covered: AthenaArrowResultSet._read_csv() CSV branch and its only caller (__init__ → as_arrow()/fetch*()), shared by ArrowCursor, AsyncArrowCursor, and AioArrowCursor; the .txt branch is unchanged; pandas and Polars have their own _read_csv().

Result: CLEAN.

  • Binary results already used ignore_empty_lines=False, so their path is unchanged.
  • Multi-column rows always contain a delimiter, so they never form empty lines; a trailing newline does not add a row (checked locally with pyarrow).
  • Quoted values containing blank lines ("a\n\nb") stay one value with either setting (checked locally).
  • Types without an Arrow dtype in DefaultArrowTypeConverter (e.g. uuid, timestamp with time zone, unknown) are inferred; 5,000 leading empty lines followed by a value still infer string across 1 KiB blocks, so NULLS FIRST results do not hit a null-type conversion error.
  • NULL in string-typed columns becomes '', the documented CSV behavior also seen in multi-column results; the test pins it.
  • The new test fails on the reverted source (both cases) and passes with the change.

@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 result; static review)

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, --sandbox read-only, session 01a10222-d559-7372-bf39-d49a4c0fc7a7. Prompt contained only the literal diff, repository context, and review instructions (no PR text, commit message, or prior findings); no edits, tests, network, or GitHub access.
Base de8cc52ac2a43cdba72bb4571c185883287741fc, head 6b924fe7161e6d0e4b61bc601beeadb78f075793, detached snapshot unchanged after the review.

Covered: sync, threaded async, and aio Arrow callers; table/fetch conversion; single/multi-column and binary results; explicit/inferred dtypes; all-NULL and header-only CSV; CRLF, quoted newlines, block_size; documented NULL behavior; the test (fails without the change: 2 vs 3 and 0 vs 1 rows).

Result: FINDINGS (2, both pre-existing; no regression in the changed line's own behavior).

  1. NULL TIME/JSON values raise when fetched. strings_can_be_null=False turns a NULL in a string-typed column into '', and _to_time('') (pyathena/converter.py:85) / _to_json('') (:121) raise. Verified: live on this head, SELECT 1 AS id, CAST(NULL AS TIME) AS t raises ValueError: time data '' does not match format '%H:%M:%S.%f' in ArrowCursor (the multi-column path is unchanged by this PR), and SELECT CAST(NULL AS TIME) AS t now raises the same instead of returning no rows. JSON: _csv_to_json in Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010 already maps '' to None. Deferred as a separate pre-existing TIME defect (it also touches _to_time, which TIME(0) and TIME(p) with p > 6 values fail to convert #1030 rewrites); not folded here.
  2. Quoted values with newlines crossing a CSV block boundary fail because newlines_in_values stays False (pyathena/arrow/result_set.py:308). Verified offline with pyarrow: 1,200-byte multi-line values with block_size=1024 raise CSV parser got out of sync with chunker; they parse with the default 1 MiB blocks unless a value straddles a boundary. Unchanged from the merge-base. Deferred as a separate issue.

@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.

Correction to finding 2: AthenaArrowResultSet.DEFAULT_BLOCK_SIZE is 128 MiB (pyathena/arrow/result_set.py:66), not 1 MiB. Multi-line values fail only when one straddles a block boundary, which needs a CSV result over 128 MiB or a smaller block_size; the 1 MiB figure in the record was the size used in the offline check.

@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.

Disposition update for finding 1: the NULL TIME ValueError is fixed in #1033 (commit "Treat an empty TIME value as NULL": _to_time() returns None for '', with an ArrowCursor regression test), which also covers #1030. It stays out of this PR to avoid a duplicate change to _to_time().

@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.

Disposition update for finding 2: filed as #1040, reproduced on Athena with execute(sql, block_size=1024).

double_quote=True,
escape_char=False,
)
Expand Down
16 changes: 16 additions & 0 deletions tests/pyathena/arrow/test_cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@ def test_binary_single_null(self, arrow_cursor):
arrow_cursor.execute("SELECT CAST(NULL AS VARBINARY) AS value")
assert arrow_cursor.fetchall() == [(None,)]

@pytest.mark.parametrize(
("query", "expected"),
[
(
"SELECT x FROM (VALUES 1, NULL, 2) AS t(x) ORDER BY x NULLS FIRST",
[(None,), (1,), (2,)],
),
# Arrow reads a NULL string from a CSV result as an empty string.
("SELECT CAST(NULL AS VARCHAR) AS v", [("",)]),
],
)
def test_single_column_null(self, arrow_cursor, query, expected):
arrow_cursor.execute(query)
assert arrow_cursor.as_arrow().num_rows == len(expected)
assert arrow_cursor.fetchall() == expected

@pytest.mark.parametrize(
"arrow_cursor",
[{"cursor_kwargs": {"unload": False}}, {"cursor_kwargs": {"unload": True}}],
Expand Down
Loading