Convert time zone values to aware times and empty TIME/JSON text to NULL - #1033
Conversation
| return _parse_time(varchar_value) | ||
|
|
||
|
|
||
| def _to_time_with_tz(varchar_value: str | None) -> time | None: |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN
Base 9a373fd3eb4b4486ea5a1eece70b7886740b3683, head f0626671c39648936cb0e408e9daf05ada2cda1d.
Covered: _parse_time, _to_time, and _to_time_with_tz; the mappings in the default, S3FS (deepcopy of _DEFAULT_CONVERTERS), Arrow, Polars, and pandas converters, and the Arrow/Polars CSV column types; AthenaPandasResultSet (_PARSE_DATES, _time_columns, _trunc_date, the converters passed to read_csv, pyarrow engine selection, chunked reads, and the fallback); AthenaArrowResultSet (_fetch and _as_arrow_from_api); AthenaPolarsResultSet (_df_converters, the chunked CSV converters, and _as_polars_from_api); the sync/async/aio cursors (shared result sets and converters).
Checks:
_to_time_with_tz: the time part never contains+or-, so the last sign marks the offset. Athena always appends±HH:MM(measured for p = 0..12 andcurrent_time).- pandas:
read_csvpasses''for NULL to converters (verified offline), and_to_time_with_tz('') is None. A converter column disables the automatic pyarrow engine (_get_csv_enginerequiresnot self.converters), so an explicitengine="pyarrow"cannot leave the column unconverted. - Arrow fetch: the S3 paths still resolve
self.convertersper batch (the Do not convert ArrowCursor fallback values twice #1023 contract). The fallback resolves only the time-with-tz converters, andconverters.get(k)leaves other fallback columns unchanged. - Polars:
_df_convertersfor the fallback holds only the time-with-tz converters. The S3 non-chunked and chunked paths are unchanged apart from the new mapping andpl.Stringdtype. - UNLOAD rejects
time with time zone(measured), so the Parquet paths need nothing. - Regression coverage: all 10
test_fetch_all_rowscases fail with the source reverted.
| "decimal": _to_decimal, | ||
| "varbinary": _to_binary, | ||
| "json": _to_json, | ||
| "time with time zone": _to_time_with_tz, |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations): CLEAN
Base 9a373fd3eb4b4486ea5a1eece70b7886740b3683, head f0626671c39648936cb0e408e9daf05ada2cda1d. Full pass over the PR body, the commit message, the docstrings, the comments, the docs, #1018, and #1030.
- "
.dt.timedropped the pandas offset; different offsets stayed strings and.dtraisedAttributeError; NULL wasNaT": measured live on master 9a373fd withPandasCursor.VALUES (+09:00), (-05:30)raisedAttributeError: Can only use .dt accessor with datetimelike values(after pandas' dateutilUserWarning), andVALUES (+09:00), (NULL)gave[(time(12, 0),), (NaT,)]. On this head, aware times andNone. - "Arrow and Polars time types drop the offset": measured offline (
pa.tablegivestime64[us],pl.DataFramegivesTime, both naive). - "UNLOAD does not support time with time zone": measured (
NOT_SUPPORTED: Unsupported Hive type: time(3) with time zone). - "as
_parse_datetime()does (Support SQLAlchemy datetime literals and microsecond timestamps #818)": both truncate digits beyond six withfraction[:6].ljust(6, "0"). - Docs:
docs/pandas.mdnow lists the converter types for the pyarrow-engine condition, includingtime with time zone.docs/s3fs.mdgains the table row. No other doc lists time conversions (grep fortime with time zoneanddatetime.time). - Callers: the
timeandtime with time zonereturn types change forCursorandS3FSCursorusers, as stated in the release notes.pyathena.TIME(the DB API type object) already covers both types. No AWS request changes.
| Returns: | ||
| The time, or None. | ||
| """ | ||
| if not varchar_value: |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS (no regressions, 1 pre-existing)
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1022b-eae4-7663-8219-5ec79f0666a4. Static review only. Snapshot: detached worktree at head f0626671c39648936cb0e408e9daf05ada2cda1d, base 9a373fd3eb4b4486ea5a1eece70b7886740b3683. The prompt contained the literal diff and the intended behavior only. Afterwards, the snapshot and the PR worktree were clean at f0626671.
Covered (reviewer): parser signs, offsets, precision, NULL and empty input; all five cursor families and the async/aio construction paths; S3 CSV/TXT, chunking, the API fallback, and failed/empty states; pandas engine selection; Arrow converter resolution (still per fetched batch); Polars row conversion; SQLAlchemy integration; docs and regression tests. No regressions found.
Finding, pre-existing P2, pyathena/converter.py:111 with pyathena/arrow/result_set.py:337: the Arrow CSV reader keeps non-binary string NULLs as "" (documented in docs/null_handling.md), so _to_time("") raises for a NULL TIME in ArrowCursor S3 results.
Author verification (live, 2026-10-03, ArrowCursor S3 results): SELECT 1 AS id, CAST(NULL AS TIME) AS t raises ValueError on master 9a373fd and on f0626671. In the same measurement, a NULL TIME WITH TIME ZONE, DECIMAL, or DATE gives (1, None), and a NULL JSON also raises (JSONDecodeError from _to_json("")). Action: fixed for TIME in this PR, because _to_time is rewritten here. The JSON case is outside this PR and is reported to the maintainer separately.
There was a problem hiding this comment.
Repair: 44b7f30 (TIME part of the finding)
_to_time() now returns None for an empty string, as _to_decimal(), _to_boolean(), and _to_time_with_tz() do. Athena never returns an empty TIME text, and DefaultTypeConverter gets None for NULL from GetQueryResults, so only the Arrow CSV path changes.
Validation:
just lintpassed.test_to_time_any_precisiongains("", None): 127 offline converter tests passed.TestArrowCursor::test_fetch_all_rowsaddsCAST(NULL AS TIME) AS col_time_null, expecting None. Bothdefaultandmanagedpass. With theconverter.pychange reverted,defaultfails andmanagedpasses (the fallback was already None).
Self-review of the repair: round one (behavior) and round two (claims) found nothing. The docstrings of _to_time and _to_time_with_tz name both empty-string sources (pandas read_csv converters, measured offline; the Arrow CSV reader, documented in docs/null_handling.md and measured live).
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN for f0626671..44b7f30feba01c7752163d483f23ab81356499cc.
Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a10233-16e5-7a00-813d-d86f54e62579. Static review; afterwards, the snapshot was clean at 44b7f30f. Covered: DefaultTypeConverter (typed complex values, tuple/dict results, sync, threaded async, and aio), S3FS (both CSV readers and the fallback), Arrow CSV with and without binary columns plus the fallback and UNLOAD, Polars eager and chunked CSV plus the fallback and UNLOAD, pandas' separate TIME handling, the docstrings, and the tests. The guard fixes the "" case. None and every nonempty TIME text behave as before. The Arrow test reaches the failing path. No regressions.
There was a problem hiding this comment.
Repair: 4d51ead (the JSON case reported above; folded in by the maintainer's choice)
Measured on master 9a373fd with S3 result files and SELECT 1 AS id, CAST(NULL AS JSON) AS j: PandasCursor.execute() raised OperationalError (from _to_json('') in the read_csv converter), ArrowCursor fetches raised JSONDecodeError, and Cursor, PolarsCursor, and S3FSCursor returned None.
_to_json() now returns None for '' and gained a docstring. Athena never returns empty JSON text: an empty JSON string is "", which still decodes to '' (unit-tested).
Validation:
just lintpassed.- New
test_to_json; 137 offline converter tests passed. - The shared test query (renamed
CONVERTED_VALUES_QUERY/CONVERTED_VALUES_ROW) addsCAST(NULL AS JSON). All 10test_fetch_all_rowscases pass. With only this commit'sconverter.pychange reverted,pandas[default]andarrow[default]fail. -k "json or complex or null or time or fetch_all"across the cursor, converter, pandas, Arrow, Polars, S3FS, aio, and SQLAlchemy test directories: 340 passed.
Self-review of the repair: round one (behavior) found nothing; every _to_json mapping (default, Arrow, Polars, pandas) gets None for NULL. Round two (claims) found nothing; the PR title, body, and release notes now include the JSON fix.
There was a problem hiding this comment.
Independent follow-up (relayed): FINDINGS, 1 regression → repaired in 213c6e0
Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a10241-af07-78a3-859e-3dbe0d3c0895. Static review of 44b7f30f..4d51eadf; afterwards, the snapshot was clean at 4d51eadf. Covered: the default, Arrow, Polars, pandas, and S3FS converters; untyped and typed array/map/struct helpers; SQLAlchemy JSON handling; the docstring, tests, and constant references (no stale TIME_VALUES_*).
Finding, regression P2, pyathena/converter.py:179: DefaultTypeConverter().convert("array", '[""]', type_hint="array(json)") returned [""] before and [None] after 4d51ead. The typed parser passes the decoded element through _to_json_str to _to_json (pyathena/parser.py:285, :327) and relied on JSONDecodeError to fall back to the native format. Verified locally: 44b7f30 gives [''], and 4d51ead gives [None] and {'k': None} for map(varchar,json).
Repair: _to_json() keeps its previous behavior (None only), so the default, S3FS, Polars, and typed complex paths are unchanged. The pandas and Arrow converters, the two that read CSV result files where '' means NULL, map json to the new _to_json_from_file(), which treats '' as None.
Validation:
just lintpassed.- New
test_to_json_empty_string(only_to_json_from_filemaps''to None;_to_json('')still raises) andtest_typed_json_empty_string_element(array(json)andmap(varchar,json)keep""): 139 offline converter tests passed. - Live: all 10
test_fetch_all_rowscases (NULL JSON in pandas and Arrowdefault) pass.-k "json or complex or null or time or fetch_all"across the cursor, pandas, Arrow, Polars, S3FS, aio, and SQLAlchemy test directories: 280 passed.
Self-review of the repair: round one (behavior) found nothing. _to_json_from_file is used only by converters whose input is CSV text from a result file. Polars reads NULL as null, not '' (its NULL JSON case passes with _to_json). Round two (claims): the PR body is updated to name _to_json_from_file.
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN for 4d51eadf..213c6e0d4a5d55448c0b56de3ce0b1f75eaddf41.
Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a10248-23be-72c1-adff-ec5a8f215feb. Static review; afterwards, the snapshot was clean at 213c6e0d. Covered: the typed array/map fallback paths, pandas and Arrow CSV conversion, the default/S3FS/Polars mappings, the docstrings, and the regression tests. Typed empty JSON strings are preserved, pandas and Arrow still turn empty CSV JSON fields into None, and the default, S3FS, and Polars converters keep the strict _to_json.
There was a problem hiding this comment.
Repair: ce31538 (replaces _to_json_from_file; maintainer chose this design after a benchmark)
The separate _to_json_from_file() existed only because the typed complex-value parser relied on _to_json("") raising. The parser now encodes JSON-typed elements with json.dumps() (_to_json_str(value, type_node)), so _to_json() treats '' as NULL like the other converters, and the pandas and Arrow mappings use _to_json again.
Behavior differences against master's parser (local, DefaultTypeConverter().convert(..., type_hint=...)):
array(json)'["{\"a\": 1}", "123"]':[{'a': 1}, 123]→['{"a": 1}', '123']. Measured with Athena:ARRAY[CAST('{"a": 1}' AS JSON), CAST('123' AS JSON)]renders["{\"a\": 1}", "123"](JSON string scalars). The same values inmap(varchar,json)androw(a json, b json)(native format) already gave strings on master.row(a json, b json)with JSON-format text ({"a": "x", "b": {"c": 1}},{"a": "", ...},{"a": true, ...}):JSONDecodeError→{'a': 'x', 'b': {'c': 1}}and so on.- Unchanged:
array(json)of objects, numbers, booleans, null,"", and mixed values;map(varchar,json);array(varchar);map(varchar,varchar).
Performance (100-element values, 3 runs): string elements about 164 → 113 µs (no exception fallback), object elements about 196 → 203 µs, numbers and booleans unchanged, and _to_json about 0.67 µs either way.
Validation: just lint passed. The new test_typed_json_elements and test_to_json cases plus test_parser.py and the converter tests: 174 passed offline; 4 new cases fail with the commit's source reverted. Live: 558 related tests passed (json, complex, null, time, fetch_all, type_hint, struct, array, map across the cursor, aio, and SQLAlchemy directories).
Self-review of the repair: round one (behavior) found nothing. _to_json_str has no other callers, and only JSON-typed elements change. Round two (claims) found nothing. The PR body and release notes now describe the array(json) behavior change and the row(json) fix. Because the parser contract changed, the next independent review covers the whole PR.
| # Nested values use the same mappings, so set() and remove() apply to them too. | ||
| self._typed_converter = TypedValueConverter( | ||
| converters=_DEFAULT_CONVERTERS, | ||
| converters=self.mappings, |
There was a problem hiding this comment.
Independent review of the whole PR (relayed): FINDINGS (1 regression, 3 pre-existing)
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a10269-fae5-72b1-a402-4a4273816a32. Static review of 9a373fd3..ce31538e (full scope after the parser contract changed). Snapshot: detached worktree at ce31538ecd3c45d4c9860365a766c2dc9b3e65d1. Afterwards, the snapshot and the PR worktree were clean at ce31538e.
Covered (reviewer): converter registration and construction paths; sync, async, and aio cursors; S3 and API fallback; typed/untyped array, map, and row parsing, nesting, keys, NULLs, and empty strings; pandas engines and chunking; Arrow converter mutation; SQLAlchemy callers; performance; docs and tests.
Findings and author verification:
- Regression, P2
pyathena/arrow/result_set.py:396,pyathena/polars/result_set.py:555: the fallback'sset("time with time zone", _to_default)did not reach nested values, becauseDefaultTypeConverterbuilt its typed converter from the module-level_DEFAULT_CONVERTERS(converter.py:714). Withresult_set_type_hints={"v": "array(time with time zone)"}, nested values were converted to aware times and lost their offsets in the Arrow/Polars tables. Verified by code reading. Live, master returns strings there. Fixed in 7096d8d. - Pre-existing, P2
pyathena/parser.py:145:time(3) with time zoneandtimestamp(3) with time zonehints parsed astime/timestamp. Verified. The maintainer chose to fold it in: fixed in 7096d8d. - Pre-existing, P2
pyathena/parser.py:322: the JSON preview heuristic sendsarray(json)values like[123456789, "a,b"]to the native splitter. Verified live: it fails on master and on this branch. The string-first order fails on master but works on this branch. Filed as Complex-value parsing loses values (typed array(json), native arrays) and PandasCursor NULL TIME differs by result storage #1041 (the maintainer chose one issue for 3 and 4). - Pre-existing, P2
pyathena/pandas/result_set.py:518: a pandas NULLtimeisNaTfrom a result file andNonefrom the fallback. Verified live on master and this branch. Filed as Complex-value parsing loses values (typed array(json), native arrays) and PandasCursor NULL TIME differs by result storage #1041.
There was a problem hiding this comment.
Repair: 7096d8d (findings 1 and 2)
DefaultTypeConverter.__init__passesconverters=self.mappings(the instance's deep copy) toTypedValueConverter, soset()/remove()also apply to nested values. Other converter instances keep the defaults (unit-tested).TypeSignatureParser.parse()keeps a trailingwith time zoneafter a parameterized type. It still ignores other trailing text, so the existingtest_trailing_modifier_after_paren(decimal(10, 2) extra→decimal) passes unchanged.
Validation:
just lintpassed. Newtest_parameterized_type_keeps_suffix,test_typed_time_with_tz_elements, andtest_set_applies_to_nested_values: 215 offline tests passed (converter, parser, the format converters, and the S3FS reader). All 4 new cases fail with the commit's source reverted.- Live,
SELECT ARRAY[CAST('12:34:56.789 +09:00' AS TIME WITH TIME ZONE), NULL]withresult_set_type_hints={"v": "array(time with time zone)"}. Master returns strings in every cursor (and raw text for the S3 format cursors). This branch returns aware times forCursor(both modes) and forPandasCursormanaged, and strings forArrowCursor/PolarsCursormanaged (no lost offset). The S3 format cursors still return raw text, as on master. - 558 related live tests passed.
Self-review of the repair: round one (behavior) found nothing. TypedValueConverter stores the dict by reference, and Converter.set/remove mutate self.mappings in place. Round two (claims) found nothing. The PR body now lists the set() scope change and the parser fix.
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN for ce31538e..7096d8d44c0023084443bbd7be0b7f1bf04796b8.
Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a10279-9762-7571-a5ca-c560e5252c53. Static review; afterwards, the snapshot was clean at 7096d8d4. Covered: Cursor, dict, and async result paths; S3FS's private DefaultTypeConverter; the Arrow, Polars, pandas, and S3FS fallbacks; custom subclass mappings via set(), remove(), and update(); cached hints; shared converter instances and isolation between instances; parameterized time/timestamp suffixes, case and whitespace, unrelated trailing text, and recursive array/map/row/struct parsing. Both findings are fixed, and there are no regressions. The added tests do not directly cover table construction, remove(), or shared cursors; the reviewer checked those statically, and the author checked table construction live (see the repair reply).
There was a problem hiding this comment.
Rebase and repair: b23cf45 (new merge-base a73c6db457110ea13eae8f7d1e7670b9095605dd, which includes #1010 and #1031)
Master gained #1010 while this PR was in review. #1010 added _csv_to_json() (empty string as NULL for CSV results) and a GetQueryResults fallback that keeps json values as text in the Arrow/Polars tables (_json_text_converter()/_json_converters()). The rebase resolved each commit's conflicts. The maintainer chose one JSON converter:
_to_json('')is None, and_csv_to_json()is removed; the pandas and Arrow mappings use_to_json. Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010'stest_csv_to_jsoncases moved intotest_to_json.- Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010's fallback helpers now cover
time with time zonetoo:_text_value_converter()/_text_value_converters()over_TEXT_VALUE_TYPES = ("json", "time with time zone"). They replace this PR's separate time-zone code in the Arrow and Polars result sets. - Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010's
test_fetch_all_rowsassertions are kept, and this PR's shared-query assertions run after them.
New finding during the rebase review (author, live): with 7096d8d's converters=self.mappings, #1010's set("json", _to_default) also reached nested values, so array(json)/map(varchar,json) hints in the Arrow/Polars fallback returned JSON text instead of decoded values. Master returns [{'a': 1}, {'a': 2}] and {'k': [1, 2]}. Repaired in b23cf45: DefaultTypeConverter uses the default mappings for nested values again (no set() scope change). Only _text_value_converter() gets its own typed converter, which keeps nested time with time zone values as text and still decodes nested JSON.
Validation at b23cf45:
just lintpassed. 218 offline tests passed (incl. the newtest_text_value_converter).- Live, managed: nested JSON returns the same as master in every cursor, including master's existing Arrow/Polars error for an array that mixes objects and strings. Nested
time with time zonekeeps the offset (aware inCursor/PandasCursor, text inArrowCursor/PolarsCursor). - 585 related live tests passed.
Self-review of the repair and the rebase: round one (behavior) checked every conflict resolution against master's #1010 and #1031 code; no findings. Round two (claims): the PR title and body were rewritten for the final state. The set() scope release note was removed, and the NULL JSON fix is attributed to #1010; no findings. The next independent review covers the whole PR at b23cf45.
7096d8d to
b23cf45
Compare
| self.converters if self._convert_rows else self._json_converters(self.converters) | ||
| self.converters | ||
| if self._convert_rows | ||
| else self._text_value_converters(self.converters) |
There was a problem hiding this comment.
Independent review of the whole PR at b23cf45 (relayed): FINDINGS (1 regression, 2 pre-existing)
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a10288-3226-7ff1-9b1e-407de4a0f67a. Static review of a73c6db4..b23cf45e (the merge-base after the rebase over #1010 and #1031). Snapshot: detached worktree at b23cf45e95507735eb21c1b88271ce6c990b9b39. Afterwards, the snapshot and the PR worktree were clean at b23cf45e.
Covered (reviewer): typed/untyped array, map, and row parsing; JSON/native formats, nesting, NULLs, empty strings, and keys; converter construction and customization; sync/async/aio cursor paths; S3 and API fallbacks; pandas engine selection and chunking; Arrow/Polars text preservation; S3FS; SQLAlchemy JSON handling; docs and regression assertions.
Findings and author verification:
- Regression, P2
pyathena/arrow/result_set.py:279:_fetch()converted every value twice. The rebase left this branch's earlier conversion block after Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010's, and the second pass overwrote the first. Verified: with a counting converter set afterexecute(), master and the fix call it['a', 'b'], and b23cf45 calls it['a', 'b', 'a', 'b']. Fixed in 3cb1343 (Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010's single pass with the renamed helper) with the newtest_fetch_converts_each_value_once. - Pre-existing, P2
pyathena/parser.py:325: thearray(json)preview heuristic,[1234567890, "a,b"]. Already Complex-value parsing loses values (typed array(json), native arrays) and PandasCursor NULL TIME differs by result storage #1041. - Pre-existing, P2
pyathena/parser.py:46,:351: native arrays drop empty-string elements. Verified live on master:Cursorreturns['x', 'y']forARRAY['x', '', 'y']and['x']forARRAY['x', ''], with or withoutarray(varchar)hints. Out of scope; reported to the maintainer.
There was a problem hiding this comment.
Repair: 3cb1343 (finding 1)
_fetch() is #1010's single pass again; the only difference from master is the renamed _text_value_converters. The diff of pyathena/{arrow,polars,pandas}/result_set.py and pyathena/result_set.py against master now has only the helper renames, the _TEXT_VALUE_TYPES selection, comments and docstrings, and the pandas parse_dates change.
Validation: just lint passed. The new TestArrowCursor::test_fetch_converts_each_value_once (a counting varchar converter set after execute()) passes, and fails with b23cf45's _fetch() (['a', 'b', 'a', 'b']). 700 related live tests passed (-k "json or complex or null or time or fetch or type_hint or struct or array or map or convert" across the cursor, aio, and SQLAlchemy directories).
Self-review of the repair: round one checked the other result sets for leftover pre-rebase blocks (none); round two found no claims needing changes.
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN for b23cf45e..3cb13439ff8dc21d9ba2a83ef997fc2d616eef69.
Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a10295-5979-74b2-83bd-17df3a3436b6. Static review; afterwards, the snapshot was clean at 3cb13439. Covered: the commit, the whole pyathena diff against a73c6db4, the conversion paths of the Arrow, Polars, pandas, and base result sets, converter propagation through execute(), and the new test. Arrow _fetch() calls each converter once per value on the S3 path, and the fallback converts only json and time with time zone. Converter mappings are resolved at fetch time, and no leftover conversion block remains. The test detects both double conversion (4 calls) and stale mappings (0 calls).
The cursors returned TIME WITH TIME ZONE values as text, except PandasCursor reading a result file, which parsed them as dates and dropped the offset (and raised AttributeError when the offsets differed). TIME values with no fraction or more than six fraction digits also raised ValueError. - _parse_time() parses TIME values of any precision, truncating digits beyond microseconds as _parse_datetime() does, and _to_time_with_tz() returns a time with a fixed-offset tzinfo. The default, Arrow, Polars, and pandas converters map time with time zone to it. - PandasCursor no longer parses time with time zone columns as dates. - Arrow and Polars time types have no time zone, so with the GetQueryResults fallback their tables keep these values as text, as with a result file, and the fetch methods convert them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Arrow CSV reader returns an empty string for a NULL in a column that it reads as text, so ArrowCursor raised ValueError for a NULL TIME. An empty string is now None, as for DECIMAL and TIME WITH TIME ZONE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pandas passes an empty string to its read_csv converters for a NULL, and the Arrow CSV reader returns one, so a NULL JSON value made PandasCursor fail in execute() and ArrowCursor fail in the fetch methods. Athena never returns empty JSON text, so an empty string is now None. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Treating an empty string as NULL in _to_json() also turned an empty JSON string inside a typed complex value (for example array(json) '[""]') into None, because the complex-type parser passes the decoded element to the converter and relied on the decode error to fall back to the native format. _to_json() keeps its previous behavior, and the pandas and Arrow converters, which read CSV result files, use the new _to_json_from_file() that treats an empty string as NULL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The typed complex-value parser turned a decoded JSON element back into text with str(), so a JSON string element lost its quotes. It relied on the JSON decode error to fall back to the native parser, decoded string scalars that looked like JSON a second time, and raised for row(json) fields. JSON-typed elements are now encoded with json.dumps(), which lets _to_json() treat an empty string as NULL like the other converters, so the separate _to_json_from_file() is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DefaultTypeConverter built its typed complex-value converter from the module-level mappings, so set() did not reach nested values. The Arrow and Polars GetQueryResults fallbacks keep TIME WITH TIME ZONE values as text with set(), but nested ones were still converted and lost their offsets in the Arrow and Polars tables. The typed converter now uses the converter's own mappings. The type signature parser also dropped " with time zone" after a precision, so time(3) with time zone and timestamp(3) with time zone hints used the TIME and TIMESTAMP converters. It now keeps that suffix and still ignores other trailing text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Making the typed converter use the converter's own mappings also kept nested JSON values as text in the Arrow and Polars GetQueryResults fallbacks, which decoded them before. DefaultTypeConverter's typed converter uses the default mappings again, and only the fallback converter, _text_value_converter(), gets a typed converter that keeps nested TIME WITH TIME ZONE values as text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_to_datetime_with_tz() passed the trailing zone to dateutil's gettz(), which returns None for a numeric UTC offset such as +05:30, so Cursor, S3FSCursor, and every GetQueryResults fallback returned a naive datetime. A numeric offset now becomes a fixed-offset time zone; zone names still use gettz(). The other cursors now return aware datetimes for these values as well: the Arrow, Polars, and pandas converters map timestamp with time zone to _to_datetime_with_tz(), PandasCursor no longer parses it as a date (a column with different zones stayed strings), and the Arrow and Polars fallback tables keep it as text, as their result-file tables do, which also avoids a Polars error and a lost Arrow time zone for mixed zones. Reported by @aminghadersohi in #854. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3cb1343 to
79fb784
Compare
| _UTC_OFFSET_PATTERN: re.Pattern[str] = re.compile(r"([+-])(\d{2}):(\d{2})") | ||
|
|
||
|
|
||
| def _parse_utc_offset(value: str) -> timezone | None: |
There was a problem hiding this comment.
Self-review of the #854 fold-in (79fb784, rebased on master 28ec68d): rounds one and two CLEAN
The maintainer asked for #854 to be folded in (relayed by another maintainer session). The pyathena commit before the fold-in is the rebased d1dee801.
Round one (behavior):
_parse_utc_offset()matches only[+-]HH:MM(a full match), soUTC,America/New_York, and other zone names still go togettz()._to_time_with_tz()reuses it, so the time and timestamp offset parsing are the same code._to_datetime_with_tz('')is None. Athena never returns empty TIMESTAMP text, and pandas/Arrow return''for NULL.- Arrow/Polars:
timestamp with time zoneis read as a string (pa.string()/pl.String) and converted when fetching, astime with time zoneis. The fallback keeps it as text (_TEXT_VALUE_TYPES), nested values too (_text_value_converter()). - pandas: removed from
_PARSE_DATESand mapped in_DEFAULT_PANDAS_CONVERTERS, so a converter column also disables the automatic pyarrow engine (docs updated). - UNLOAD rejects
timestamp with time zone(measured), so the Parquet converters need nothing. - Regression coverage: the shared
CONVERTED_VALUES_QUERYadds+05:30,-08:00,America/New_York, and NULL. All 10test_fetch_all_rowscases fail with the source changes reverted, and so do 4 cases of the new unit test.
Round two (claims): the PR title, body, release notes, and commit message state the cursor-wide change. The commit message credits @aminghadersohi, and the body has Closes #854. Measured claims: master's naive offsets, strings, Arrow zone loss, and Polars TypeError (tzfile(...) is not a fixed offset timezone) come from the live matrix run on master 28ec68d, and this branch gives aware datetimes in all 30 cursor × mode × query cases. No findings.
|
|
||
|
|
||
| def _parse_utc_offset(value: str) -> timezone | None: | ||
| """Parse a ``+HH:MM`` or ``-HH:MM`` UTC offset. |
There was a problem hiding this comment.
Independent review of the whole PR at 79fb784 (relayed): no regressions; 2 pre-existing findings
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a102a7-0d36-79a3-bafa-4fc8366fb3aa. Static review of 28ec68d9..79fb7848 (full scope after the #854 fold-in and the rebase). Snapshot: detached worktree at 79fb784848701c674bb537c96624fba9f54614c6. Afterwards, the snapshot and the PR worktree were clean at 79fb7848.
Covered (reviewer): typed/untyped array, map, and row parsing; JSON/native formats, nesting, NULLs, empty strings, and keys; converter construction across sync/async/aio cursors; S3 and API fallbacks; pandas engines and chunking; Arrow converter timing; SQLAlchemy JSON handling; docs and test assertions.
Findings (both pre-existing, outside this PR):
- P2
pyathena/parser.py:326: thearray(json)preview heuristic sends[1234567890, "a,b"]to the native splitter. Already Complex-value parsing loses values (typed array(json), native arrays) and PandasCursor NULL TIME differs by result storage #1041. - P2
pyathena/parser.py:351,:46: native arrays drop empty-string elements. Verified live on master earlier (ARRAY['x', '', 'y']→['x', 'y']); reported to the maintainer.
Reviewer notes on coverage: the precision, numeric-offset, suffix, and JSON-string assertions are meaningful against the base. The Arrow convert-once test guards behavior that the base already had (it caught the rebase leftover at b23cf45). Chunked reads, explicit pandas engines, async/aio variants, and nested fallback table construction are not exercised by the new integration cases. The nested fallback was checked live, as recorded in the earlier replies.
WHAT
TIME WITH TIME ZONE values become timezone-aware
datetime.timeobjects and TIMESTAMP WITH TIME ZONE values become timezone-awaredatetimeobjects (including numeric UTC offsets) in every cursor. TIME values of any precision convert, and an empty TIME, TIMESTAMP WITH TIME ZONE, or JSON text is NULL.pyathena/converter.py:_parse_time()parsesHH:MM:SSwith an optional fraction of up to 12 digits, truncating digits beyond microseconds as_parse_datetime()does (Support SQLAlchemy datetime literals and microsecond timestamps #818)._to_time()uses it. The new_to_time_with_tz()parses the trailing+HH:MM/-HH:MMoffset into a fixed-offsetdatetime.timezone._to_datetime_with_tz()turns a numeric UTC offset (+05:30,-08:00), whichdateutil.tz.gettz()returns None for, into a fixed-offsetdatetime.timezone(the shared_parse_utc_offset(), also used by_to_time_with_tz()); zone names (UTC,America/New_York) still usegettz()(TIMESTAMP WITH TIME ZONE values with a numeric UTC offset come back naive #854). Before,Cursor,S3FSCursor, and every GetQueryResults fallback returned a naive datetime for an offset._to_time(),_to_time_with_tz(),_to_datetime_with_tz(), and_to_json()treat''as NULL, as_to_decimal()and_to_boolean()do. pandasread_csvconverters and the Arrow CSV reader return''for a NULL, and Athena never returns empty TIME or JSON text. This replaces_csv_to_json()from Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010, so_to_json()is again the only JSON converter.DefaultTypeConverter(Cursor,S3FSCursor, and the GetQueryResults fallback of the format cursors) and the Arrow, Polars, and pandas converters maptime with time zoneto_to_time_with_tz(); the Arrow, Polars, and pandas converters also maptimestamp with time zoneto_to_datetime_with_tz(). Arrow and Polars read both columns from the result file as strings.PandasCursorno longer liststime with time zoneortimestamp with time zonein itsparse_dates/ time truncation; the converters read them. The pandas time offset was dropped by.dt.time, a time column with different offsets stayed strings so.dtraisedAttributeError, and a timestamp column with different zones or a zone name stayed strings.pa.tablegivestime64[us]andpl.DataFramegivesTime, both dropping the offset). Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010's GetQueryResults fallback mechanism, which keeps json values as text in the Arrow/Polars tables and converts them when fetching, now covers the time zone types as well:_json_text_converter()/_json_converters()become_text_value_converter()/_text_value_converters()over_TEXT_VALUE_TYPES = ("json", "time with time zone", "timestamp with time zone"). For values nested in typed complex values (result_set_type_hints), the fallback converter keeps only the time zone types as text, so nested JSON values are decoded as before. On master, a GetQueryResults fallback with zones that differ by row raisedTypeErrorinPolarsCursor(... is not a fixed offset timezone) and dropped the zones inArrowCursor(measured).pyathena/parser.py):_to_json_str()encodes elements of a JSON type withjson.dumps()instead ofstr(), so_to_json()decodes their original JSON text. Before, a JSON string element lost its quotes, and the parser relied on the decode error to fall back to the native format. That decoded string scalars whose text looked like JSON a second time (["{\"a\": 1}", "123"]asarray(json)gave[{'a': 1}, 123], while the same values inmap/rowstayed strings), and it raisedJSONDecodeErrorforrow(... json)fields. Measured with Athena:ARRAY[json_parse(...), ...]renders[{"a":1}, "x", 1, true, null, ""]. In a microbenchmark of 100-element values, string elements ran about 30% faster (no exception fallback), object elements stayed within about 5%, and numbers and booleans were unchanged.with time zoneafter a parameterized type (time(3) with time zone,timestamp(3) with time zone), which it used to drop; other trailing text is still ignored.time with time zoneandtimestamp with time zoneamong the columns that need a converter; the S3FSCursor type table liststime with time zone.Release notes (4.0.0):
time with time zonevalues are timezone-awaredatetime.timeobjects inCursor,S3FSCursor,ArrowCursor,PolarsCursor, andPandasCursor. They used to be strings, or (PandasCursorreading a result file) naive times without the offset. InPandasCursor, a NULL in such a column isNoneinstead ofNaT, and such a column keeps the pyarrow CSV engine from being used.timestamp with time zonevalues are timezone-awaredatetimeobjects inArrowCursorandPolarsCursor(strings before when read from a result file) and inPandasCursorfor every column (strings before when the zones differed or were names).as_arrow()/as_polars()holdtime with time zoneandtimestamp with time zonevalues as text, as with a result file, instead of times without the offset or a single time zone.result_set_type_hints, JSON string elements ofarray(json)whose text looks like JSON stay strings (as inmapandrow) instead of being decoded a second time.timestamp with time zonevalues with a numeric UTC offset keep the offset instead of being returned naive (TIMESTAMP WITH TIME ZONE values with a numeric UTC offset come back naive #854, reported by @aminghadersohi).PolarsCursorwith managed query result storage no longer raisesTypeErrorfortimestamp with time zonezone names or for zones that differ by row.TIME(0)andTIME(p)withp > 6values no longer raiseValueError.ArrowCursorreturns None for a NULLTIMEread from a result file instead of raisingValueError.row(... json)type hints no longer raiseJSONDecodeErrorfor JSON-format row values, andtime(p) with time zone/timestamp(p) with time zonetype hints use the time zone converters.UNLOAD supports neither
time with time zonenortimestamp with time zone(measured:NOT_SUPPORTED: Unsupported Hive type: time(3) with time zone/timestamp(3) with time zone), so the Parquet paths are unaffected.WHY
Closes #1018. Closes #1030. Closes #854.
The maintainer chose timezone-aware
datetime.timefor every cursor (timestamp with time zonealready returns aware datetimes) and folded #1030 in, since both need the same TIME parser. Review findings folded in by the maintainer's choice: the empty-string NULL TIME inArrowCursor, a single JSON converter instead of a CSV-only one (decided after a benchmark), and the parameterizedwith time zonetype hints. #854 (numeric UTC offsets intimestamp with time zone, reported by @aminghadersohi, who offered a PR; the maintainers picked it up after a week as the issue said they might) was folded in at the maintainer's request, because this PR rewrites the neighbouring converter code; the cursor-wide handling follows the maintainer's choice of aware datetimes everywhere. The remaining pre-existing findings are #1041.TEST
Tested commit: 79fb784 (rebased on master 28ec68d)
just lint: passed.TIME(0..12) WITH TIME ZONEreturnsHH:MM:SS[.f{p}]±HH:MM(for example12:34:56+09:00,12:34:56.7-05:30,12:34:56.789123456789+14:00);current_timereturns+00:00.PandasCursorraisedAttributeErrorfor a column with different offsets and gaveNaTfor a NULL;ArrowCursorraisedValueErrorfor a NULL TIME.timestamp with time zoneon master 28ec68d and this branch acrossCursor,PandasCursor,ArrowCursor,PolarsCursor, andS3FSCursor, with S3 results and managed storage, for+05:30, mixed+05:30/-08:00/UTC/NULL rows, andAmerica/New_York. On master: naive datetimes for offsets inCursor/S3FSCursorand every fallback; strings inArrowCursor/PolarsCursorand inPandasCursorfor mixed zones or names; zones dropped inArrowCursormanaged for mixed rows; andTypeErrorinPolarsCursormanaged for mixed rows and names. On this branch: aware datetimes in every case.test_to_time_any_precision,test_to_time_with_tz,test_to_json(incl. Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010's_csv_to_jsoncases),test_typed_json_elements,test_typed_time_with_tz_elements,test_text_value_converter, andtest_to_datetime_with_tz_offsets_and_zone_names(tests/pyathena/test_converter.py) andtest_parameterized_type_keeps_suffix(tests/pyathena/test_parser.py), with the format converter and S3FS reader tests: 225 passed.test_fetch_all_rowsforCursor,S3FSCursor,PandasCursor,PolarsCursor, andArrowCursor(defaultandmanaged) runs the sharedCONVERTED_VALUES_QUERY/CONVERTED_VALUES_ROW(tests/pyathena/util.py): TIME(0), TIME(9), TIME WITH TIME ZONE with+09:00, TIME(0) WITH TIME ZONE with-05:30, a NULL TIME WITH TIME ZONE, a NULL JSON, and TIMESTAMP WITH TIME ZONE with+05:30,-08:00,America/New_York, and NULL. It also keeps Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010's JSON assertions. Arrow and Polars check that the table keeps the text, and Arrow adds a NULL TIME. With the TIMESTAMP WITH TIME ZONE values with a numeric UTC offset come back naive #854 commit's source changes reverted, all 10 cases fail.result_set_type_hints:array(time with time zone)keeps the offset in every cursor (aware times forCursorandPandasCursor, text forArrowCursor/PolarsCursor), andarray(json)/map(varchar,json)return the same values as master in every cursor (including master's existing Arrow/Polars failure for an array mixing objects and strings).TestArrowCursor::test_fetch_converts_each_value_once: a converter set afterexecute()is called once per value.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": 700 passed.just test pyathenasuite and the SQLAlchemy compliance suites; CI runs the applicable ones when the PR is marked Ready.🤖 Generated with Claude Code