Keep complex values that the parsers lost or mixed up - #1054
Conversation
- Native-format arrays are split as Athena joins the items, at ", ", so empty items, items of spaces, and items with a comma that is not followed by a space are kept. They used to be dropped or split. - A typed array(json) value is parsed as JSON whatever its first element is; Athena renders JSON elements as JSON text. The prefix check sent [1234567890, "a,b"] to the native splitter, which failed. - PandasCursor returns None for a NULL TIME read from a result file, as the GetQueryResults fallback and the other types do, instead of NaT. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return items | ||
|
|
||
|
|
||
| def _split_native_array_items(inner: str) -> list[str]: |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN
Base ec5323ea30e3fc2da1aca536d9cbdf8b51c7b3e2, head a3566039fd28bc6d5f5e0b90e550f4768e3726dd.
Covered: _split_native_array_items, typed _convert_typed_array (JSON-first condition and native branch), untyped _to_array/_parse_array_native, the unchanged map/struct splitting (_split_array_items), and pandas _trunc_date (non-chunked, chunked, and all-NULL columns).
- Separator: Athena joins array items with
", "(measured). A top-level,followed by a space separates items; brace/bracket depth keeps row/map items ({a=1, b=}) intact. Not stripping the interior keeps[x, ]and[ , x]. - Safety fallbacks stay as they were: nested arrays in native text return None (typed) or the original string (untyped), and items with
[]="make the untyped parser return the original string. - Typed
array(json): JSON-first only for the json element type. Other types keep the prefix check (json.loads('[1.50]')gives1.5, which would changearray(varchar)). Invalid JSON still falls back to the native branch. - pandas:
.dt.timeof an all-NULL column stays datetime64 (measured with pandas 3), soastype(object)comes beforewhere(..., None). Both a mixed and an all-NULL column give None.parse_datesis unchanged, so pyarrow engine selection is unchanged. - Regression coverage: 6 of the 8 new unit cases, both
test_fetch_complex_valuescases, andpandas[default]test_fetch_all_rowsfail with the source reverted.
| inner_preview = value[1:10] if len(value) > 10 else value[1:-1] | ||
| if '"' in inner_preview or value.startswith(("[{", "[null", "[[")): | ||
| if ( | ||
| element_type.type_name == "json" |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations): CLEAN
Base ec5323ea30e3fc2da1aca536d9cbdf8b51c7b3e2, head a3566039fd28bc6d5f5e0b90e550f4768e3726dd. Full pass over the PR body, the commit message, the docstrings, the comments, and #1041.
- The measured Athena text table (master abc99b0) is from a live run with raw converters on
Cursor(GetQueryResults) andS3FSCursor(CSV result file); the two give identical text. - "Maps and rows already kept empty strings": same run (
{=v, k=}→{'': 'v', 'k': ''},{a=, b=x}→{'a': '', 'b': 'x'}). - The ambiguity list matches the measurements:
[]forARRAY[''],[null, NULL]forARRAY['null', 'NULL'], and"[ ]"parsing as an empty JSON array in the untyped path (unit test removed for that reason). - The release notes call out the kept leading/trailing spaces and the pandas NULL TIME change, including
as_pandas(). - No AWS request changes and no public signature changes;
_split_native_array_itemsis new and private.
get_chunk() returned the reader's chunk without the date truncation that iteration applies, so a time column came back as Timestamps on the current date, with NaT for NULL. It now applies the same truncation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if isinstance(self._reader, TextFileReader): | ||
| return self._reader.get_chunk(size) | ||
| return next(self._reader) | ||
| return self._trunc_date(self._reader.get_chunk(size)) |
There was a problem hiding this comment.
Independent review (relayed): no regressions; 1 pre-existing finding → folded in
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a102c7-ffb5-7902-8830-9fe54dca52a8. Static review of ec5323ea..a3566039. Snapshot: detached worktree at a3566039fd28bc6d5f5e0b90e550f4768e3726dd. The prompt contained the literal diff and the intended behavior only. Afterwards, the snapshot and the PR worktree were clean at a3566039.
Covered (reviewer): typed/untyped arrays; scalar, row/map, and nested-array elements; NULLs, whitespace, commas, quotes, = and brackets; fallback contracts and callers; pandas whole and chunked reads, all-NULL columns, fetch methods, as_pandas(), engine selection, and test compatibility. No regressions. By source comparison, the first five new native-array cases and the long-prefix JSON case fail on the base.
Finding, pre-existing P2, pyathena/pandas/result_set.py:166: PandasDataFrameIterator.get_chunk() (documented in docs/pandas.md) returned the reader's chunk without _trunc_date. Verified live on this branch with chunksize=2: get_chunk() gave [Timestamp('2026-10-04 12:34:56'), NaT] for a TIME column, while iteration gave [time(12, 34, 56), None].
Repaired in bab2687: get_chunk() applies self._trunc_date to both reader kinds, as __next__ does. The non-chunked iterators use _no_trunc_date, so the whole-result path is unchanged. New TestPandasCursor::test_get_chunk_time passes, and fails with the previous get_chunk(). tests/pyathena/pandas and tests/pyathena/aio/pandas: 309 passed; just lint passed. Self-review of the repair (both rounds): no findings. The release note was extended to cover get_chunk().
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN for a3566039..bab2687a6a644dc4240d758aa88860905d6a6085.
Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a102d3-db57-7892-8c52-dd2169ae7b3f. Static review; afterwards, the snapshot was clean at bab2687a. Covered: TextFileReader chunks and single-DataFrame iterators with either callback (one conversion, as in iteration); the explicit-chunksize, auto-optimized, and whole-result paths (_no_trunc_date prevents double truncation); UNLOAD results; and close, exhaustion, and size handling (unchanged; conversion errors get the same cleanup as iteration). The test fails with the previous implementation.
1 participant
WHAT
Fixes the three conversion defects in #1041, plus a related pandas
get_chunk()defect found in review._to_array()and typedarray(...)hints): Athena joins array items with", ", so the new_split_native_array_items()(pyathena/parser.py) splits only at a top-level comma followed by a space and keeps empty items. The interior is no longer stripped before splitting. Empty items, items of spaces, and items with a comma that is not followed by a space are now kept; before, they were dropped or split. Maps and rows keep using_split_array_items(); they already kept empty strings.array(json):_convert_typed_array()parses the value as JSON first whenever the element type isjson, because Athena renders JSON elements as JSON text. Before, the parser decided by the first nine characters, which sent[1234567890, "a,b"]to the native splitter, and that raisedJSONDecodeError. The prefix check still applies to other element types: parsingarray(varchar)as JSON would, for example, turn1.50into1.5.PandasCursortime columns:PandasDataFrameIterator.get_chunk()applies the same date truncation as iteration (found by the independent review; it returned Timestamps on the current date andNaT).PandasCursorNULL TIME:_trunc_date()returns None for a NULLtimevalue read from a result file, as the GetQueryResults fallback and the other types do (maintainer's choice), instead ofNaT. It keepsparse_dates, so the pyarrow engine conditions are unchanged.Measured Athena native text (master abc99b0; GetQueryResults and the CSV result file give the same text):
ARRAY['x', '', 'y'][x, , y]['x', 'y']['x', '', 'y']ARRAY['x', ''][x, ]['x']['x', '']ARRAY['x', ' ', 'y'][x, , y]['x', 'y']['x', ' ', 'y']ARRAY['a,b', 'c'][a,b, c]['a', 'b', 'c']['a,b', 'c']Some values cannot be told apart in the native text, so they behave as before:
ARRAY['']andARRAY[' ']render as[]and[ ], which are also valid empty JSON arrays (untyped conversion gives[]). The text'null'and NULL both render asnull. Items that contain", "are split.Release notes (4.0.0):
ARRAY['x', '', 'y']used to come back as['x', 'y']). Behavior change: leading and trailing spaces of array items are kept.array(json)type hints parse values whose first element is not a string.PandasDataFrameIterator.get_chunk()(withchunksize) converts time columns as iteration does, instead of returning Timestamps on the current date.PandasCursorreturns None instead ofNaTfor a NULL TIME read from a result file, in the fetch methods and inas_pandas()(isna()is still true for it).WHY
Closes #1041.
The three defects were found by the independent reviews of #1033. The maintainer chose None for the pandas NULL TIME and asked for them to be fixed together in this session.
TEST
Tested commit: a356603, and bab2687 for the
get_chunk()repair (test_get_chunk_time;tests/pyathena/pandasandtests/pyathena/aio/pandas: 309 passed)just lint: passed.test_native_array_items(untyped andarray(varchar)) andtest_typed_json_array_starting_with_non_string, with the converter, parser, format converter, and S3FS reader tests: 233 passed. With the source changes reverted, 6 of the 8 new cases fail; the other 2 guard existing behavior ([x, null], rows with empty fields).TestCursor::test_fetch_complex_values(defaultandmanaged, untyped and with type hints) andtest_fetch_all_rowsforCursor,S3FSCursor,PandasCursor,PolarsCursor, andArrowCursor, whose shared query now has a NULL TIME. All 14 pass. With the source changes reverted, bothtest_fetch_complex_valuescases andpandas[default]fail.uv run --env-file .env pytest -n 4overtests/pyathena/test_cursor.py,test_async_cursor.py, and thepandas,arrow,polars,s3fs,aio/{pandas,arrow,polars,s3fs},sqlalchemy, andaio/sqlalchemydirectories with-k "json or complex or null or time or fetch or type_hint or struct or array or map or convert or timestamp": 702 passed.just test pyathenasuite and the SQLAlchemy compliance suites; CI runs the applicable ones when the PR is marked Ready.🤖 Generated with Claude Code