Skip to content

Use the C engine for Pandas DDL text results #1075

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 2 commits into from
Oct 4, 2026
Merged

Use the C engine for Pandas DDL text results #1075

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
49e857d
fix(pandas): preserve DDL text results with the PyArrow engine
laughingman7743 Oct 4, 2026
707d648
test(pandas): align DDL regression tests with existing conventions
laughingman7743 Oct 4, 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
5 changes: 3 additions & 2 deletions docs/pandas.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,9 +556,10 @@ Common performance options:
- `dtype`: Explicit column data types
- `parse_dates`: Columns to parse as dates

With `engine="pyarrow"`, PandasCursor uses the PyArrow engine only when pyarrow is installed, no chunksize is set (explicitly or by `auto_optimize_chunksize`), `quoting` is the default, the result has no columns that need a converter (`boolean`, `decimal`, `varbinary`, `json`, `time with time zone`, and `timestamp with time zone` with the default converter), and the result file is at least `AthenaPandasResultSet.PYARROW_MIN_FILE_SIZE_BYTES` bytes.
With `engine="pyarrow"`, PandasCursor uses the PyArrow engine only when the result is not a tab-separated `.txt` file, pyarrow is installed, no chunksize is set (explicitly or by `auto_optimize_chunksize`), `quoting` is the default, the result has no columns that need a converter (`boolean`, `decimal`, `varbinary`, `json`, `time with time zone`, and `timestamp with time zone` with the default converter), and the result file is at least `AthenaPandasResultSet.PYARROW_MIN_FILE_SIZE_BYTES` bytes.
Otherwise, it falls back to the C engine.
With the PyArrow engine, when `keep_default_na` and `na_values` are the defaults and the only pandas.read_csv() options passed to `execute()` are `dtype` as a mapping and `parse_dates` as a list, PyAthena reads the file with `pyarrow.csv`, allowing newlines in quoted values, and converts it to a DataFrame as `pandas.read_csv(engine="pyarrow")` does, except that columns with a string dtype are read as text, other than in the tab-separated results of DDL statements, and without `future.infer_string`, a column with dtype `str` keeps NULL as a missing value.
With `engine="pyarrow"`, tab-separated `.txt` results from DDL statements such as `SHOW TABLES`, `SHOW COLUMNS`, and `DESCRIBE` use the C engine to preserve leading zeros, exponent notation, and padding in string values.

@laughingman7743 laughingman7743 Oct 4, 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 2 — CLEAN (compatibility, operational effects, and claims).

Base: 6d268b1
Head: 49e857d

Audited the full diff and the claims in the PR body, changed comment, and documentation against their callers and the installed pandas 3.0.6 parser source. For headerless input, pandas/PyAthena read inferred Arrow types before applying the dtype mapping; the pre-fix failures confirm the reported text changes. Explicit Python engine selection precedes the new fallback, and ordinary CSV selection remains subject to its existing compatibility and size checks. All three Pandas cursor implementations construct the shared result set; no public signature, AWS request, retry, or UNLOAD change was introduced.

Checked the three in-memory DDL shapes, 100-byte boundary, NA override, both infer_string modes, literal-value assertions, and real-parser execution. The documentation now describes the .txt fallback; the private helper's generic headerless behavior remains documented accurately for its direct callers. Corrected the PR's validation status after completion of the multiversion docs build and distinguished it from the separate build of the edited working tree (195 warnings, successful).

No actionable findings or performance claims. Offline validation passed at this head; local live AWS tests and Ready CI are still pending.

@laughingman7743 laughingman7743 Oct 4, 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.

Round two repair follow-up — CLEAN.

Base: 6d268b17d984adf2b16acadfd3e8fd447a03a376. Published head: 707d648829173145f3a978e1c537d810f1df4f53. This is a separate claims, compatibility, and evidence audit of the narrow repair after the completed initial round-two review.

Checked the new test docstrings, parameter inputs and expected values, engine documentation, suite selection, and the PR's validation claims against the parser and its callers. Literal expected strings preserve the original zero/exponent/padding assertions. Moving the methods does not change fixture use or the file-based CI selection, and the repair adds no AWS requests or production behavior.

Corrected the newly added documentation condition from a required .csv extension to exclusion of tab-separated .txt results, matching _get_csv_engine(). This also remains accurate with the latest base branch's in-memory GetQueryResults CSV path when no output location exists. just docs lint passed and the direct working-tree Sphinx build succeeded with 195 documentation warnings. The 92 offline results belong to this published head; the earlier 40 local AWS results belong to 49e857d8, and current-head AWS CI remains pending while the PR is Draft. No actionable findings in this repair scope. This is an author self-review; static inspection does not establish unrun runtime results.

With the PyArrow engine, when `keep_default_na` and `na_values` are the defaults and the only pandas.read_csv() options passed to `execute()` are `dtype` as a mapping and `parse_dates` as a list, PyAthena reads the file with `pyarrow.csv`, allowing newlines in quoted values, and converts it to a DataFrame as `pandas.read_csv(engine="pyarrow")` does, except that columns with a string dtype are read as text, and without `future.infer_string`, a column with dtype `str` keeps NULL as a missing value.
Otherwise, pandas reads the file, and its PyArrow engine raises an error or returns wrong values when a quoted value containing a newline crosses one of pyarrow's read blocks.
It also changes the values of a column with a string dtype whose values all look like numbers, such as `"007"` to `"7.0"`, and without `future.infer_string`, turns NULL in a column with dtype `str` into the string `"nan"`.

Expand Down
4 changes: 4 additions & 0 deletions pyathena/pandas/result_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,10 @@ def _get_csv_engine(
# Use PyArrow only when explicitly requested and all compatibility
# checks pass; otherwise fall through to the C engine default.
if self._engine == "pyarrow":
# Header-less DDL results lose numeric-looking strings and their padding
# when PyArrow infers types before applying the string dtypes.
if self.output_location and self.output_location.endswith(".txt"):

@laughingman7743 laughingman7743 Oct 4, 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.

Relayed independent review — CLEAN (static source review).

Reviewer: Claude Code, requested and actual model claude-opus-5-5; first-party Max profile (subscriptionType=max verified before launch), effort high. Session: df008dcd-c865-459a-9366-746bc506a313.
Base: 6d268b1
Head: 49e857d

The reviewer covered the full four-file diff, the CSV engine selection/read pipeline, dtype/converter and metadata contracts, exact-value regression assertions, file-size boundaries, stream closure, existing DDL/CSV tests, documentation claims, and repository conventions. It returned no actionable defects.

The review used an exported tracked-file snapshot and literal diff, with Read/Grep/Glob tools only. Builds, tests, edits, Git/GitHub operations, PR discussion, commit messages, credentials, and agent memory were excluded. The CLI completed successfully with no permission denials; its modelUsage reports canonicalModel=claude-opus-5-5 and provider=firstParty. All 287 snapshot files remained unchanged, and the author's worktree remained clean at the reviewed head.

Non-blocking note relayed from the reviewer: headerless branches in the private _read_csv_with_pyarrow helper are now unreachable through the result-set reader, but remain accurate and directly tested for callers of that helper. They cause no failure; simplification is deferred to a separate change to keep this fix bounded.

This result establishes a completed static review, not executed AWS validation.

@laughingman7743 laughingman7743 Oct 4, 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.

Relayed independent repair review — CLEAN.

Reviewer: Claude Code, requested and actual model claude-opus-5-5, effort high, verified first-party max authentication; no Enterprise, API-key, or alternate-provider route. Session: 2a90b454-5aa0-4ec2-a997-88e6e2caa280.

Base: 6d268b17d984adf2b16acadfd3e8fd447a03a376. Published head: 707d648829173145f3a978e1c537d810f1df4f53. The reviewer received a tracked-source snapshot of that head, the literal repair diff from 49e857d8c45acd8da7efef8dab8f95bacf020877, and the bounded patch-series mapping with commit subjects removed. The unchanged original commit was not reopened. No PR description, discussion, prior findings, author conclusions, credentials, or memory were supplied. Invocation allowed only Read/Grep/Glob, with no writes, shell, builds, tests, GitHub operations, hooks/plugins, or external MCP tools. The snapshot fingerprints and author worktree/head were unchanged after completion.

Covered pytest discovery and class conventions, the unchanged 36 engine-selection and 12 DDL parsing cases, exact 20-character padding and empty DESCRIBE fields, independent expected strings, mock/stream lifetimes, the directly affected engine/read-option paths, and the documentation condition at docs/pandas.md:559. The reviewer found no actionable defects in the bounded repair.

The reviewer noted that the unchanged documentation omits the existing column-name-resolution fallback to C. This omission predates the repair and is deferred outside its test-organization and .txt condition scope; it is not an introduced defect.

This is a static review. The reviewer did not execute pytest, lint, documentation builds, or dependency behavior checks, and compared the old test bodies only through the supplied repair hunks. The separate author validation results remain runtime evidence, not reviewer-executed checks.

return "c"

@laughingman7743 laughingman7743 Oct 4, 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 1 — CLEAN (implementation behavior).

Base: 6d268b1
Head: 49e857d

Covered the full four-file diff: engine selection, parser regression coverage, existing test setup, and Pandas documentation. Traced _read_csv(), converter/dtype options, stream closure, and the shared result set used by synchronous, Future-based asynchronous, and asyncio Pandas cursors. The .txt fallback occurs only for a requested PyArrow engine; explicit Python selection, CSV compatibility checks, chunking, and UNLOAD parsing retain their existing paths.

The tests call the real CSV parsers with in-memory result files above the size threshold and assert literal names, frame parity with the C engine, and stream closure. Before the fix, 15 new cases failed on the reported numeric inference; after it, the result-set tests and existing engine compatibility test passed (92 tests).

No actionable findings. This is a source review supported by offline tests, not proof of live AWS execution; live validation is pending.

@laughingman7743 laughingman7743 Oct 4, 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.

Round one repair follow-up — CLEAN.

Base: 6d268b17d984adf2b16acadfd3e8fd447a03a376. Published head: 707d648829173145f3a978e1c537d810f1df4f53. Compared the previously reviewed series ending at 49e857d8c45acd8da7efef8dab8f95bacf020877 with the repaired series using range-diff after verifying both old commit objects.

Covered the moved result-set methods, pytest parametrization, mock and stream lifetimes, engine selection, and DDL parsing assertions. The two methods now belong to the existing TestAthenaPandasResultSet; SHOW TABLES, SHOW COLUMNS, and DESCRIBE are explicit data/name/expected-value cases. All three serialized inputs and their 30-row expected strings exactly match the original cases (120/630/1290 bytes), so leading zeros, exponent notation, padding, NULL parity, and the PyArrow size threshold remain covered. The 36 engine-selection and 12 DDL parsing cases are retained. Production code is unchanged by this repair.

The test-convention concern is resolved. just format, just lint, and the affected offline suite passed (92 tests, no AWS calls). No actionable defects found in the repair scope; this is an author self-review, not an independent review.

effective_chunksize = chunksize if chunksize is not None else self._chunksize
is_compatible = (
effective_chunksize is None
Expand Down
1 change: 1 addition & 0 deletions tests/pyathena/pandas/test_cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,7 @@ def test_get_csv_engine_explicit_specification(self):
result_set._chunksize = None # Default values
result_set._quoting = 1
result_set._metadata = None
result_set._query_execution = None
result_set._kwargs = {}

# Test C engine specification
Expand Down
99 changes: 99 additions & 0 deletions tests/pyathena/pandas/test_result_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,105 @@ def test_as_pandas_single_dataframe(self):


class TestAthenaPandasResultSet:
@pytest.mark.parametrize("engine", ["auto", "c", "python", "pyarrow"])
@pytest.mark.parametrize(
("output_location", "file_size_bytes", "pyarrow_engine"),
[
("s3://bucket/result.txt", None, "c"),
("s3://bucket/result.txt", 99, "c"),
("s3://bucket/result.txt", 100, "c"),
("s3://bucket/result.txt", 101, "c"),
("s3://bucket/result.csv", None, "pyarrow"),
("s3://bucket/result.csv", 99, "c"),
("s3://bucket/result.csv", 100, "pyarrow"),
("s3://bucket/result.csv", 101, "pyarrow"),
(None, None, "pyarrow"),
],
)
def test_get_csv_engine_result_format(
self, engine, output_location, file_size_bytes, pyarrow_engine
):
"""PyArrow falls back for DDL text while C and Python choices are preserved."""
result_set = AthenaPandasResultSet.__new__(AthenaPandasResultSet)
result_set._query_execution = MagicMock(output_location=output_location)
result_set._metadata = None
result_set._converter = DefaultPandasTypeConverter()
result_set._engine = engine
result_set._chunksize = None
result_set._quoting = csv.QUOTE_ALL
result_set._kwargs = {}

expected = {"auto": "c", "c": "c", "python": "python", "pyarrow": pyarrow_engine}[engine]
assert result_set._get_csv_engine(file_size_bytes) == expected

@pytest.mark.parametrize("infer_string", [True, False])
@pytest.mark.parametrize("read_csv_kwargs", [{}, {"keep_default_na": True}])
@pytest.mark.parametrize(
("data", "names", "expected_names"),
[
("001\n007\n1e3\n" * 10, ["tab_name"], ["001", "007", "1e3"] * 10),
(
"001 \n007 \n1e3 \n" * 10,
["field"],
["001 ", "007 ", "1e3 "] * 10,
),
(
(
"001 \tint \t\n"
"007 \tint \t\n"
"1e3 \tint \t\n"
)
* 10,
["col_name", "data_type", "comment"],
["001 ", "007 ", "1e3 "] * 10,
),
],
ids=["show_tables", "show_columns", "describe"],
)
def test_read_csv_ddl_preserves_numeric_looking_names(
self, data, names, expected_names, read_csv_kwargs, infer_string
):
"""Reading DDL results preserves numeric-looking names and their padding."""
data = data.encode()
assert len(data) >= AthenaPandasResultSet.PYARROW_MIN_FILE_SIZE_BYTES

result_set = AthenaPandasResultSet.__new__(AthenaPandasResultSet)
result_set._query_execution = MagicMock(
output_location="s3://bucket/result.txt", substatement_type=None
)
result_set._converter = DefaultPandasTypeConverter()
result_set._engine = "pyarrow"
result_set._chunksize = None
result_set._auto_optimize_chunksize = False
result_set._quoting = csv.QUOTE_ALL
result_set._keep_default_na = False
result_set._na_values = ("",)
result_set._kwargs = read_csv_kwargs
result_set._fs = MagicMock()
result_set._fs.open.return_value = stream = io.BytesIO(data)
description = [(name, "string", None, None, 0, 0, "UNKNOWN") for name in names]

with (
pd.option_context("future.infer_string", infer_string),
patch.object(
AthenaPandasResultSet,
"description",
new_callable=PropertyMock,
return_value=description,
),
patch.object(result_set, "_get_content_length", return_value=len(data)),
patch("pandas.read_csv", wraps=pd.read_csv) as read_csv,
):
expected = pd.read_csv(io.BytesIO(data), **result_set._get_csv_read_options("c", None))
read_csv.reset_mock()
actual = result_set._read_csv()

assert_frame_equal(actual, expected, check_exact=True)
assert actual.iloc[:, 0].tolist() == expected_names
read_csv.assert_called_once()
assert read_csv.call_args.kwargs["engine"] == "c"
assert stream.closed

@pytest.mark.parametrize(
("execute_kwargs", "path", "filesystem_kwargs"),
[
Expand Down
Loading