Repository navigation
Use the C engine for Pandas DDL text results - #1075
Conversation
| # 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"): | ||
| return "c" |
There was a problem hiding this comment.
Self-review round 1 — CLEAN (implementation behavior).
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.
There was a problem hiding this comment.
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.
| With `engine="pyarrow"`, PandasCursor uses the PyArrow engine only when the result file has a `.csv` extension, 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. |
There was a problem hiding this comment.
Self-review round 2 — CLEAN (compatibility, operational effects, and claims).
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.
There was a problem hiding this comment.
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.
| 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"): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
(cherry picked from commit bf6f299) Conflict resolution: the _get_csv_engine() fix in pyathena/pandas/result_set.py applies unchanged. The other files conflicted with text from PRs that are not backported to 3.x: - docs/pandas.md: master's paragraph on when engine="pyarrow" applies was added by #902 and extended by #1033 and #1057; 3.x has none of that text. #1075 added a ".txt" condition to its first sentence, which has no 3.x counterpart, and a sentence on DDL results; only that sentence is added, after the read_csv options. - tests/pyathena/pandas/test_cursor.py: only #1075's result_set._query_execution = None is added; the _metadata and _kwargs lines around it come from other PRs. - tests/pyathena/pandas/test_result_set.py: 3.x's file (from #950) has no TestAthenaPandasResultSet, so the class is created with #1075's test_get_csv_engine_result_format and the imports it needs. test_read_csv_ddl_preserves_numeric_looking_names is dropped: it stubs result_set._fs.open() and asserts that the stream is closed, but 3.x's _read_csv() passes the S3 path to pandas.read_csv(), and reading through the result set's filesystem comes from #1001 (not backported). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
WHAT
Use the C engine for tab-separated
.txtresults whenPandasCursoris configured withengine="pyarrow".This preserves leading zeros, exponent notation, and padding in numeric-looking DDL result strings.
Keep explicit Python engine selection and the existing CSV engine compatibility checks, and document the DDL fallback.
Group the result-set regression tests in the existing test class and express DDL input, column names, and expected values with parametrization.
WHY
PyArrow infers numeric types before applying string dtypes to headerless DDL results, changing values returned by statements such as
SHOW TABLES,SHOW COLUMNS, andDESCRIBE.The C engine reads these strings without that inference and already serves as the fallback for unsupported PyArrow options.
Closes #1073.
TEST
Tested commit:
707d6488(Python 3.13.1, pandas 3.0.6, pyarrow 25.0.1).just formatandjust lint: passed.uv run --env-file .env pytest --noconftest tests/pyathena/pandas/test_result_set.py tests/pyathena/pandas/test_cursor.py::TestPandasCursor::test_get_csv_engine_explicit_specification -q --tb=short: 92 passed; session hooks disabled, no AWS calls.future.infer_stringsettings. Engine selection covers.txtand.csv, the size boundary, and explicit Python/C/auto engines.just docs lint: passed.uv run sphinx-build -b html docs /private/tmp/pyathena-1073-evidence/repair-docs-current: succeeded with 195 warnings in the documentation build; this validates the edited working-tree page.just docs buildpassed at49e857d8(multiversion build, with documentation warnings).claude-opus-5-5, first-party Max profile, efforthigh; recorded inline. The snapshots and author worktree remained unchanged. These were static reviews.49e857d8,uv run --env-file .env pytest -n 1 tests/pyathena/pandas/test_cursor.py::TestPandasCursor::test_show_columns tests/pyathena/pandas/test_cursor.py::TestPandasCursor::test_empty_result_ddl tests/pyathena/pandas/test_cursor.py::TestPandasCursor::test_pyarrow_engine_string_values tests/pyathena/pandas/test_async_cursor.py::TestAsyncPandasCursor::test_show_columns tests/pyathena/pandas/test_async_cursor.py::TestAsyncPandasCursor::test_empty_result_ddl tests/pyathena/aio/pandas/test_cursor.py -q --tb=short: 40 passed against AWS, including synchronous, Future-based asynchronous, and asyncio Pandas cursors. Production code is unchanged by the repair.707d6488. The PR is Ready and mergeable after both repair self-reviews and independent review. SQLAlchemy compliance and Spark suites are excluded by the workflow's path filters for this Pandas-only change.