Avoid Pandas JSON dtype warnings by defaulting to whole-chunk parsing - #1080
Conversation
| for converter in self._csv_converters.values() | ||
| ): | ||
| # A NULL placeholder can give internal parser blocks different dtypes. | ||
| read_csv_kwargs.setdefault("low_memory", False) |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN. Full diff: d98976731b29eed7649c3095990e9b6f4fbc6d14..c2e602283264b38b5384009838c9f812f6e8dae5. Covered all three changed files and traced converter construction, selected/renamed column handling, explicit converter and low_memory overrides, actual engine selection including PyArrow fallback, whole and chunked reads, NULL restoration in columns/indexes, and the sync/Async/aio callers. The new option is applied after final converter resolution, and setdefault preserves explicit options. Validation: required lint and docs checks passed; all 67 offline result-set tests passed using the real parser; removing only the new default in the test process causes the original DtypeWarning failure. No repairs or deferred source findings. Live AWS validation is still pending and is not implied by this source-level CLEAN result. HEAD and tracked worktree are unchanged.
| When the C engine reads columns with PyAthena's JSON converter, `low_memory` defaults to `False`. | ||
| This avoids pandas' `DtypeWarning` when JSON numbers or booleans and NULLs occur in different internal parser blocks, while keeping the decoded values exact. | ||
| Parsing each result or chunk at once can use more memory; `chunksize` still limits the rows read per chunk. | ||
| An explicit `low_memory=True` or `low_memory=False` passed to `execute()` takes precedence. |
There was a problem hiding this comment.
Self-review round two (claims, compatibility, operations): CLEAN. Full diff: d98976731b29eed7649c3095990e9b6f4fbc6d14..c2e602283264b38b5384009838c9f812f6e8dae5. Audited the PR body, changed comments/docstrings, documentation, and commit claims independently of round one's behavior checklist. Checked pandas 3.0.6 CParserWrapper: low_memory=False uses read(nrows), while True uses read_low_memory(nrows) and mixed-dtype concatenation. TextFileReader.get_chunk(size) still passes nrows=size. Real-parser tests verify both overrides, 400,005-row first chunks, exact JSON integers/booleans with NULL, and inferred int64 without NULL. Traced unchanged sync/Async/aio keyword forwarding and absence of added AWS calls/retries. The added loop inspects converter metadata only; it adds no per-value allocation or column-value scan in PyAthena. Benchmark numbers match the recorded three-run medians and show higher peak RSS for the numeric-only whole read; no general memory bound or S3 performance claim is made. Corrected the PR's validation sequence to distinguish offline CI before Ready from AWS CI after Ready. Live AWS results remain pending, separate from this source-level CLEAN result. No source repairs or deferred findings; review HEAD/worktree unchanged.
| for converter in self._csv_converters.values() | ||
| ): | ||
| # A NULL placeholder can give internal parser blocks different dtypes. | ||
| read_csv_kwargs.setdefault("low_memory", False) |
There was a problem hiding this comment.
Relayed independent review: CLEAN, static source review only. Reviewer: Claude Code claude-opus-5-5, profile max, effort high; first-party claude.ai authentication verified with subscriptionType=max, no API/provider override or fallback model. Session: d06619e5-2d05-4f07-9ab8-208985fe194f; returned modelUsage confirms claude-opus-5-5/firstParty. Full diff: d98976731b29eed7649c3095990e9b6f4fbc6d14..c2e602283264b38b5384009838c9f812f6e8dae5. Restricted to Read/Glob/Grep in an exported tracked-source snapshot; no builds, tests, code execution, edits, GitHub/commit/PR context, local memory, external tools, or delegation. Covered all changed source/tests/docs, engine and option precedence, selected/renamed columns, custom converters, S3/GetQueryResults paths, sync/Async/aio callers, chunk reads, value/dtype/NULL/index restoration, and regression failure coverage. No actionable findings. Reviewer noted the intentional memory tradeoff, the CSV-wide effect on inferred dtypes, and that unusual parsing overrides can leave an unused JSON converter in the map; these are implications of choosing low_memory=False, not suppressed warnings. The author will clarify the read-wide effect in docs when resolving the concurrent upstream rebase. All 287 snapshot files and the PR worktree remained unchanged. This review applies to the stated head; the updated upstream revision will receive a follow-up before Ready.
There was a problem hiding this comment.
Updated to head 4596cb242cea8a23214f6587c58797ac196fe06a on base 200762088e7b0bac45054f22e32a16aa0ce95dbb, preserving upstream's DDL/PyArrow fix and clarifying converter-map membership and the CSV-wide inferred-dtype effect. Both self-review perspectives and the Claude Opus 5.5 Max/high independent follow-up are CLEAN for the bounded range-diff. The code and regression tests are unchanged. Current-head validation: 115 offline tests, 17 focused AWS tests, and both auto/c runs of the issue's 400,010-row Athena query passed. The earlier 401-test full pandas/aio run remains recorded against its original head.
c2e6022 to
4596cb2
Compare
| for converter in self._csv_converters.values() | ||
| ): | ||
| # A NULL placeholder can give internal parser blocks different dtypes. | ||
| read_csv_kwargs.setdefault("low_memory", False) |
There was a problem hiding this comment.
Self-review round one, bounded rebase/documentation follow-up: CLEAN. Previous full range: d98976731b29eed7649c3095990e9b6f4fbc6d14..c2e602283264b38b5384009838c9f812f6e8dae5; current range: 200762088e7b0bac45054f22e32a16aa0ce95dbb..4596cb242cea8a23214f6587c58797ac196fe06a. Verified old objects and reviewed the literal range-diff. The implementation and 22 added regression cases are unchanged; only the docs were clarified and reconciled with upstream. Traced upstream's new .txt PyArrow-to-C fallback into final converter resolution: DDL string-only results receive no JSON-specific low_memory option. Upstream filesystem changes concern copying and annotations; the CSV open/read path is unchanged. Lint and current docs checks passed after rebase; all 115 offline result-set tests passed, including the upstream DDL cases. The full pandas/aio suite passed 401 tests on the old head; current-head focused live tests are running and are not yet reported as passed. No source findings or repairs.
|
|
||
| When the C engine's converter mapping includes PyAthena's JSON converter, `low_memory` defaults to `False`. | ||
| This avoids pandas' `DtypeWarning` when JSON numbers or booleans and NULLs occur in different internal parser blocks. | ||
| The setting applies to the entire CSV read, including other columns whose dtypes pandas infers. |
There was a problem hiding this comment.
Self-review round two, bounded rebase/documentation follow-up: CLEAN. Reviewed the same literal old/new patch-series comparison as round one; current base/head are 200762088e7b0bac45054f22e32a16aa0ce95dbb / 4596cb242cea8a23214f6587c58797ac196fe06a. Checked the revised claims against final converter-map detection and pandas' read-wide low_memory behavior. The docs now state that converter-map membership triggers the default, including unusual parsing overrides that retain an unused JSON converter, and that other inferred columns also receive whole-read dtype inference. Memory cost, preserved requested chunk sizes, and explicit-option precedence remain accurately described. The adjacent upstream DDL/PyArrow explanation is preserved. No new API, positional argument, AWS request, or retry change. Prior measured performance remains explicitly limited to the old head's unchanged local conversion/parser code; the 401-test old-head result is not represented as current-head execution. No findings or deferred claims.
| for converter in self._csv_converters.values() | ||
| ): | ||
| # A NULL placeholder can give internal parser blocks different dtypes. | ||
| read_csv_kwargs.setdefault("low_memory", False) |
There was a problem hiding this comment.
Relayed independent rebase/documentation follow-up: CLEAN, static source review only. Reviewer: Claude Code claude-opus-5-5, verified first-party max subscription/profile, effort high, no provider/API override or fallback. Session: 2f27307c-3e04-4ecf-bddb-96ddd1369ca5; returned modelUsage confirms claude-opus-5-5/firstParty. Compared literal git range-diff d98976731b29eed7649c3095990e9b6f4fbc6d14..c2e602283264b38b5384009838c9f812f6e8dae5 200762088e7b0bac45054f22e32a16aa0ce95dbb..4596cb242cea8a23214f6587c58797ac196fe06a, the current diff, and directly affected upstream pandas changes. Confirmed unchanged code/tests, accurate converter-map/CSV-wide documentation, preserved chunk limits and explicit overrides, and compatibility with upstream's .txt-to-C engine fallback and its tests. No actionable findings. Read/Glob/Grep only in an exported source snapshot, with no executed validation, writes, PR/commit discussion, memory, or external tools. All 289 snapshot files and the PR worktree remained unchanged. Separate author-run current-head validation: 115 offline result-set tests and 17 focused real-AWS tests passed; the issue's 400,010-row Athena reproduction passed on auto and c with ten None values and DtypeWarning treated as an error. This runtime evidence is the author's validation, not the reviewer's.
1 participant
WHAT
Default CSV reads using the C engine and PyAthena's JSON converter to
low_memory=False.This avoids
DtypeWarningwhen numeric or boolean JSON values and NULLs occur in different internal parser blocks, preserving the NULL placeholder conversion.Explicit
low_memoryoptions take precedence; selected columns, custom converter overrides, and the actual CSV engine determine whether the default applies.Document the memory tradeoff and add real-parser regression tests for whole reads, large chunks, indexes, column selection, engine fallback, and overrides.
WHY
Closes #1078.
The NULL placeholder introduced by #1044 preserves JSON integer precision, but pandas can infer different dtypes in its internal blocks and warn when joining them.
Parsing each result or requested chunk at once avoids this warning without adding per-value allocations or column scans to PyAthena's conversion code.
TEST
Current tested commit:
4596cb242cea8a23214f6587c58797ac196fe06a; base200762088e7b0bac45054f22e32a16aa0ce95dbb; Python 3.13.1, pandas 3.0.6.just format,just lint, andjust docs lint: passed after rebase.uv run --env-file .env pytest --noconftest -n 0 tests/pyathena/pandas/test_result_set.py -q: 115 passed, including the upstream DDL regressions; AWS session hooks disabled.uv run --env-file .env pytest -n 1 tests/pyathena/pandas/test_cursor.py::TestPandasCursor::test_integer_and_json_with_null tests/pyathena/pandas/test_cursor.py::TestPandasCursor::test_show_columns tests/pyathena/pandas/test_async_cursor.py::TestAsyncPandasCursor::test_show_columns tests/pyathena/aio/pandas/test_cursor.py::TestAioPandasCursor::test_as_pandas -q: 17 passed against real AWS.engine=autoandengine=c, treating DtypeWarning as an error: each returned 400,010 rows, 400,000 exact JSON integers, ten None values, and no DtypeWarning. Query IDs:cee695e3-8ac5-4430-a85a-d054e7af45a0,73188282-db4e-486d-a6d5-0d0b26e83280.just docs buildsucceeded before rebase.uv run sphinx-build -b html docs docs/_build/currentsucceeded after rebase; the incremental build reported 93 existing generated-API/cross-reference warnings. Draft Docs Lint/build checks also passed.claude-opus-5-5, verified first-party Max authentication, and effort high, with no fallback model. Initial and bounded rebase/documentation follow-up reviews were CLEAN; both were static only.Before rebase, commit
c2e602283264b38b5384009838c9f812f6e8dae5passed the full pandas/aio suite:uv run --env-file .env pytest -n 1 tests/pyathena/pandas/ tests/pyathena/aio/pandas/ -q(401 passed). Removing only the new default in the test process made the new numeric C-engine regression fail with the original DtypeWarning. The code and regression tests were unchanged by the rebase; the docs now explicitly describe converter-map membership and the CSV-wide inference effect.The following local CSV comparison was measured on that earlier head: 400,010 rows, two columns, three interleaved runs per setting in fresh processes; median parse/restore time and process peak RSS. It excludes S3 download and does not establish memory bounds for other result shapes.
low_memory=Truelow_memory=FalseCurrent-head CI completed successfully after Ready: Test run 37190479496 on
4596cb242cea8a23214f6587c58797ac196fe06a, Python 3.14.7: 2327 passed, 1 skipped, 13 warnings, no reruns. All applicable checks (Test lint/AWS suite, Docs Lint/build, License Headers, and Benchmark tooling offline) passed. The SQLAlchemy compliance jobs are intentionally skipped by the path filter because no dialect, shared fixture, or dependency changed. The PR is Ready and mergeable; all required reviews and validation are complete.