Skip to content

Convert time zone values to aware times and empty TIME/JSON text to NULL - #1033

Merged
laughingman7743 merged 9 commits into
masterfrom
fix/1018-time-with-time-zone
Oct 3, 2026
Merged

laughingman7743 merged 9 commits into
masterfrom
fix/1018-time-with-time-zone

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

WHAT

TIME WITH TIME ZONE values become timezone-aware datetime.time objects and TIMESTAMP WITH TIME ZONE values become timezone-aware datetime objects (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() parses HH:MM:SS with 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:MM offset into a fixed-offset datetime.timezone.
  • _to_datetime_with_tz() turns a numeric UTC offset (+05:30, -08:00), which dateutil.tz.gettz() returns None for, into a fixed-offset datetime.timezone (the shared _parse_utc_offset(), also used by _to_time_with_tz()); zone names (UTC, America/New_York) still use gettz() (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. pandas read_csv converters 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 map time with time zone to _to_time_with_tz(); the Arrow, Polars, and pandas converters also map timestamp with time zone to _to_datetime_with_tz(). Arrow and Polars read both columns from the result file as strings.
  • PandasCursor no longer lists time with time zone or timestamp with time zone in its parse_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 .dt raised AttributeError, and a timestamp column with different zones or a zone name stayed strings.
  • Arrow and Polars time types have no time zone (measured: pa.table gives time64[us] and pl.DataFrame gives Time, 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 raised TypeError in PolarsCursor (... is not a fixed offset timezone) and dropped the zones in ArrowCursor (measured).
  • The typed complex-value parser (pyathena/parser.py): _to_json_str() encodes elements of a JSON type with json.dumps() instead of str(), 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"] as array(json) gave [{'a': 1}, 123], while the same values in map/row stayed strings), and it raised JSONDecodeError for row(... 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.
  • The type signature parser keeps with time zone after a parameterized type (time(3) with time zone, timestamp(3) with time zone), which it used to drop; other trailing text is still ignored.
  • Docs: the pandas pyarrow-engine condition lists time with time zone and timestamp with time zone among the columns that need a converter; the S3FSCursor type table lists time with time zone.

Release notes (4.0.0):

  • Behavior change: time with time zone values are timezone-aware datetime.time objects in Cursor, S3FSCursor, ArrowCursor, PolarsCursor, and PandasCursor. They used to be strings, or (PandasCursor reading a result file) naive times without the offset. In PandasCursor, a NULL in such a column is None instead of NaT, and such a column keeps the pyarrow CSV engine from being used.
  • Behavior change: timestamp with time zone values are timezone-aware datetime objects in ArrowCursor and PolarsCursor (strings before when read from a result file) and in PandasCursor for every column (strings before when the zones differed or were names).
  • Behavior change: with managed query result storage, as_arrow() / as_polars() hold time with time zone and timestamp with time zone values as text, as with a result file, instead of times without the offset or a single time zone.
  • Behavior change: with result_set_type_hints, JSON string elements of array(json) whose text looks like JSON stay strings (as in map and row) instead of being decoded a second time.
  • Fix: timestamp with time zone values 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).
  • Fix: PolarsCursor with managed query result storage no longer raises TypeError for timestamp with time zone zone names or for zones that differ by row.
  • Fix: TIME(0) and TIME(p) with p > 6 values no longer raise ValueError.
  • Fix: ArrowCursor returns None for a NULL TIME read from a result file instead of raising ValueError.
  • Fix: row(... json) type hints no longer raise JSONDecodeError for JSON-format row values, and time(p) with time zone / timestamp(p) with time zone type hints use the time zone converters.

UNLOAD supports neither time with time zone nor timestamp 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.time for every cursor (timestamp with time zone already 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 in ArrowCursor, a single JSON converter instead of a CSV-only one (decided after a benchmark), and the parameterized with time zone type hints. #854 (numeric UTC offsets in timestamp 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.
  • Measured Athena text (2026-10-03): TIME(0..12) WITH TIME ZONE returns HH:MM:SS[.f{p}]±HH:MM (for example 12:34:56+09:00, 12:34:56.7-05:30, 12:34:56.789123456789+14:00); current_time returns +00:00.
  • Measured on master 9a373fd with S3 result files: PandasCursor raised AttributeError for a column with different offsets and gave NaT for a NULL; ArrowCursor raised ValueError for a NULL TIME.
  • Measured timestamp with time zone on master 28ec68d and this branch across Cursor, PandasCursor, ArrowCursor, PolarsCursor, and S3FSCursor, with S3 results and managed storage, for +05:30, mixed +05:30/-08:00/UTC/NULL rows, and America/New_York. On master: naive datetimes for offsets in Cursor/S3FSCursor and every fallback; strings in ArrowCursor/PolarsCursor and in PandasCursor for mixed zones or names; zones dropped in ArrowCursor managed for mixed rows; and TypeError in PolarsCursor managed for mixed rows and names. On this branch: aware datetimes in every case.
  • Offline: 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_json cases), test_typed_json_elements, test_typed_time_with_tz_elements, test_text_value_converter, and test_to_datetime_with_tz_offsets_and_zone_names (tests/pyathena/test_converter.py) and test_parameterized_type_keeps_suffix (tests/pyathena/test_parser.py), with the format converter and S3FS reader tests: 225 passed.
  • test_fetch_all_rows for Cursor, S3FSCursor, PandasCursor, PolarsCursor, and ArrowCursor (default and managed) runs the shared CONVERTED_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.
  • Live, managed, with result_set_type_hints: array(time with time zone) keeps the offset in every cursor (aware times for Cursor and PandasCursor, text for ArrowCursor/PolarsCursor), and array(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 after execute() is called once per value.
  • uv run --env-file .env pytest -n 4 over tests/pyathena/test_cursor.py, test_async_cursor.py, and the pandas, arrow, polars, s3fs, aio/{pandas,arrow,polars,s3fs}, sqlalchemy, and aio/sqlalchemy directories with -k "json or complex or null or time or fetch or type_hint or struct or array or map or convert": 700 passed.
  • Not run locally: the full just test pyathena suite and the SQLAlchemy compliance suites; CI runs the applicable ones when the PR is marked Ready.

🤖 Generated with Claude Code

Comment thread pyathena/converter.py
return _parse_time(varchar_value)


def _to_time_with_tz(varchar_value: str | None) -> time | None:

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 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 and current_time).
  • pandas: read_csv passes '' for NULL to converters (verified offline), and _to_time_with_tz('') is None. A converter column disables the automatic pyarrow engine (_get_csv_engine requires not self.converters), so an explicit engine="pyarrow" cannot leave the column unconverted.
  • Arrow fetch: the S3 paths still resolve self.converters per batch (the Do not convert ArrowCursor fallback values twice #1023 contract). The fallback resolves only the time-with-tz converters, and converters.get(k) leaves other fallback columns unchanged.
  • Polars: _df_converters for the fallback holds only the time-with-tz converters. The S3 non-chunked and chunked paths are unchanged apart from the new mapping and pl.String dtype.
  • UNLOAD rejects time with time zone (measured), so the Parquet paths need nothing.
  • Regression coverage: all 10 test_fetch_all_rows cases fail with the source reverted.

"decimal": _to_decimal,
"varbinary": _to_binary,
"json": _to_json,
"time with time zone": _to_time_with_tz,

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 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.time dropped the pandas offset; different offsets stayed strings and .dt raised AttributeError; NULL was NaT": measured live on master 9a373fd with PandasCursor. VALUES (+09:00), (-05:30) raised AttributeError: Can only use .dt accessor with datetimelike values (after pandas' dateutil UserWarning), and VALUES (+09:00), (NULL) gave [(time(12, 0),), (NaT,)]. On this head, aware times and None.
  • "Arrow and Polars time types drop the offset": measured offline (pa.table gives time64[us], pl.DataFrame gives Time, 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 with fraction[:6].ljust(6, "0").
  • Docs: docs/pandas.md now lists the converter types for the pyarrow-engine condition, including time with time zone. docs/s3fs.md gains the table row. No other doc lists time conversions (grep for time with time zone and datetime.time).
  • Callers: the time and time with time zone return types change for Cursor and S3FSCursor users, as stated in the release notes. pyathena.TIME (the DB API type object) already covers both types. No AWS request changes.

Comment thread pyathena/converter.py
Returns:
The time, or None.
"""
if not varchar_value:

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.

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.

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.

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 lint passed.
  • test_to_time_any_precision gains ("", None): 127 offline converter tests passed.
  • TestArrowCursor::test_fetch_all_rows adds CAST(NULL AS TIME) AS col_time_null, expecting None. Both default and managed pass. With the converter.py change reverted, default fails and managed passes (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).

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.

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.

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.

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 lint passed.
  • New test_to_json; 137 offline converter tests passed.
  • The shared test query (renamed CONVERTED_VALUES_QUERY/CONVERTED_VALUES_ROW) adds CAST(NULL AS JSON). All 10 test_fetch_all_rows cases pass. With only this commit's converter.py change reverted, pandas[default] and arrow[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.

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.

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 lint passed.
  • New test_to_json_empty_string (only _to_json_from_file maps '' to None; _to_json('') still raises) and test_typed_json_empty_string_element (array(json) and map(varchar,json) keep ""): 139 offline converter tests passed.
  • Live: all 10 test_fetch_all_rows cases (NULL JSON in pandas and Arrow default) 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.

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.

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.

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.

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 in map(varchar,json) and row(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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 14:41
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 14:46
@laughingman7743 laughingman7743 changed the title Convert time with time zone values to aware times Convert time with time zone values to aware times and empty TIME/JSON to NULL Oct 3, 2026
@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 15:04
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 15:31
Comment thread pyathena/converter.py Outdated
# Nested values use the same mappings, so set() and remove() apply to them too.
self._typed_converter = TypedValueConverter(
converters=_DEFAULT_CONVERTERS,
converters=self.mappings,

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.

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:

  1. Regression, P2 pyathena/arrow/result_set.py:396, pyathena/polars/result_set.py:555: the fallback's set("time with time zone", _to_default) did not reach nested values, because DefaultTypeConverter built its typed converter from the module-level _DEFAULT_CONVERTERS (converter.py:714). With result_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.
  2. Pre-existing, P2 pyathena/parser.py:145: time(3) with time zone and timestamp(3) with time zone hints parsed as time/timestamp. Verified. The maintainer chose to fold it in: fixed in 7096d8d.
  3. Pre-existing, P2 pyathena/parser.py:322: the JSON preview heuristic sends array(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).
  4. Pre-existing, P2 pyathena/pandas/result_set.py:518: a pandas NULL time is NaT from a result file and None from 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.

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.

Repair: 7096d8d (findings 1 and 2)

  • DefaultTypeConverter.__init__ passes converters=self.mappings (the instance's deep copy) to TypedValueConverter, so set()/remove() also apply to nested values. Other converter instances keep the defaults (unit-tested).
  • TypeSignatureParser.parse() keeps a trailing with time zone after a parameterized type. It still ignores other trailing text, so the existing test_trailing_modifier_after_paren (decimal(10, 2) extra → decimal) passes unchanged.

Validation:

  • just lint passed. New test_parameterized_type_keeps_suffix, test_typed_time_with_tz_elements, and test_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] with result_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 for Cursor (both modes) and for PandasCursor managed, and strings for ArrowCursor/PolarsCursor managed (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.

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.

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).

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.

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:

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 lint passed. 218 offline tests passed (incl. the new test_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 zone keeps the offset (aware in Cursor/PandasCursor, text in ArrowCursor/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.

@laughingman7743
laughingman7743 force-pushed the fix/1018-time-with-time-zone branch from 7096d8d to b23cf45 Compare October 3, 2026 16:09
@laughingman7743 laughingman7743 changed the title Convert time with time zone values to aware times and empty TIME/JSON to NULL Convert time with time zone values to aware times and empty TIME/JSON text to NULL Oct 3, 2026
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)

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.

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:

  1. 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 after execute(), 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 new test_fetch_converts_each_value_once.
  2. Pre-existing, P2 pyathena/parser.py:325: the array(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.
  3. Pre-existing, P2 pyathena/parser.py:46, :351: native arrays drop empty-string elements. Verified live on master: Cursor returns ['x', 'y'] for ARRAY['x', '', 'y'] and ['x'] for ARRAY['x', ''], with or without array(varchar) hints. Out of scope; reported to the maintainer.

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.

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.

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.

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).

laughingman7743 and others added 9 commits October 4, 2026 01:30
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>
The rebase onto #1010 left this branch's earlier conversion code after
#1010's, so _fetch() converted every value twice and kept the second
result. It now has #1010's single pass with the renamed helper.

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>
@laughingman7743
laughingman7743 force-pushed the fix/1018-time-with-time-zone branch from 3cb1343 to 79fb784 Compare October 3, 2026 16:43
@laughingman7743 laughingman7743 changed the title Convert time with time zone values to aware times and empty TIME/JSON text to NULL Convert time zone values to aware times and empty TIME/JSON text to NULL Oct 3, 2026
Comment thread pyathena/converter.py
_UTC_OFFSET_PATTERN: re.Pattern[str] = re.compile(r"([+-])(\d{2}):(\d{2})")


def _parse_utc_offset(value: str) -> timezone | None:

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 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), so UTC, America/New_York, and other zone names still go to gettz(). _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 zone is read as a string (pa.string() / pl.String) and converted when fetching, as time with time zone is. The fallback keeps it as text (_TEXT_VALUE_TYPES), nested values too (_text_value_converter()).
  • pandas: removed from _PARSE_DATES and 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_QUERY adds +05:30, -08:00, America/New_York, and NULL. All 10 test_fetch_all_rows cases 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.

Comment thread pyathena/converter.py


def _parse_utc_offset(value: str) -> timezone | None:
"""Parse a ``+HH:MM`` or ``-HH:MM`` UTC offset.

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.

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):

  1. P2 pyathena/parser.py:326: the array(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.
  2. 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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 16:50
@laughingman7743
laughingman7743 merged commit ec5323e into master Oct 3, 2026
14 checks passed
@laughingman7743
laughingman7743 deleted the fix/1018-time-with-time-zone branch October 3, 2026 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant