Derive the one_row_complex expectations of the Python-object cursors - #875
laughingman7743 wants to merge 14 commits into
Conversation
test_complex for Cursor, S3FSCursor, and AsyncS3FSCursor, test_as_pandas, and the SQLAlchemy test_reflect_select spelled out the one_row_complex row, description, DB API types, and reflected types by hand. They now take them from the table definition through tests/pyathena/expected.py, which states per Athena type family what these cursors return: map and struct values as strings, JSON casts as parsed JSON, and so on. The rules never call PyAthena's converters. The queries list the table columns first and the CAST columns after them. test_as_pandas now also covers col_varchar, and test_reflect_select compares the reflected column names instead of their count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| } | ||
|
|
||
|
|
||
| def _parsed_json(value: Any, athena_type: str) -> Any: |
There was a problem hiding this comment.
Self-review round one (implementation behavior): FINDINGS (2 cleanups + 1 comment fix, fixed)
Base 2760077 (#874 head, the stacked base), head a9f4db9. Full diff reviewed (7 files).
Findings, fixed in db7a792:
_sameand_as_listhad no Google-style docstrings._json_compatiblehad one caller. It is now folded into_parsed_json.- The
PYTHONcomment listed DictCursor. DictCursor rows are dicts, and no test usesPYTHONwith it, so it is removed from the list.
The offline output is unchanged, and the five tests pass again on AWS.
Checked:
- Assertion strength: every removed literal row, description tuple, DB-API type object, and reflected-type assertion has a derived equivalent. Offline, the derived rows, types, descriptions and DB-API objects are equal to the removed literals.
test_as_pandasgainscol_varcharand a column-name check.test_reflect_selectcompares names instead of the count.
- Non-circularity:
expected.pyimports only the DB-API constants and SQLAlchemy type classes the tests already compared against. It imports no converter. - Cast placement:
Selection.rowstakes each cast's value from its source column index, and its rule is looked up by the cast's target family.timestamp with time zone,timeandjsonare all covered. - Failure mode: an unknown family raises
KeyErrorin the test, so a new type fails loudly instead of passing vacuously.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class Selection: |
There was a problem hiding this comment.
Self-review round two (claims, callers, AWS operations): FINDINGS (description only, fixed)
Claims checked:
- "Every value, Python type, description tuple, and DB API type object is still asserted": confirmed by the offline comparison, and
test_complexkeeps the per-valuetype()check. - "Test IDs unchanged": no parametrization changed.
- CI scope:
tests/pyathena/sqlalchemy/test_base.pymatches thesqlafilter in.github/workflows/test.yaml, so both compliance suites run on Ready. The description already says so. - Callers:
ONE_ROW_COMPLEXis still inTABLESat the same position, so the setup order and the DDL are unchanged. Only these five tests importexpected.py.
Findings, both corrected in the PR description:
- Overclaim: "cannot inherit a converter bug". A hand-written rule can still copy buggy output, so the sentence now only says that the rules do not call the converters.
- Overclaim: "a new rule only when the type family is new". Element and key types matter too, so it now says a rule changes when the column's type is not covered by the existing rules.
The tested commit is updated to db7a792.
Without type hints, the cursors parse Athena's text rendering of arrays, maps, and structs, so the expected nested values now come from that rendering: an array that is valid JSON is parsed, and other nested values are their text, such as true for a boolean. Type parameters are read from the outer type only, so ARRAY<DECIMAL(10,2)> has none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| result = [] | ||
| for name, athena_type, _, _ in self._items(): | ||
| code, precision, scale = _DESCRIPTION[family(athena_type)] | ||
| if parameters := _parameters(athena_type): |
There was a problem hiding this comment.
Independent review (relayed Codex result): FINDINGS (3 × P2), finding 1 of 3
Reviewer: Codex CLI 0.157.1 (codex exec -s read-only), model gpt-6-astra, session 01a0e784-02ed-7502-b61d-1a0fa9f8719a. Base 2760077, head db7a792. It ran on a detached snapshot without .env and without PR framing; afterwards the snapshot was unchanged at the head. This was a static review only.
Covered, as reported: all seven files. Removed literals vs derived rows, descriptions, Python/DB-API types, SQLAlchemy reflection checks, cast ordering, fixture generation, and converter independence. "Current fixture assertions remain equivalent; no circular converter calls or vacuous assertions found."
P2 — Nested parameters corrupt outer-column metadata. Adding
ARRAY<DECIMAL(10,2)>makes_parameters()extract the element's(10,2)and assign it to the array's description. The array requires precision/scale(0,0), so correct cursor results fail. Restrict parameter extraction to the outer type.
There was a problem hiding this comment.
Repair in 9b8cfac: verified. With the old re.search, _parameters("ARRAY<DECIMAL(10,2)>") returned (10, 2). The regex is now anchored to the outer type name, so it returns () for that type and still (10, 1) for DECIMAL(10,1) and (10,) for varchar(10), checked offline. The docstring states this.
| Returns: | ||
| The elements as a list. | ||
| """ | ||
| return list(value) |
There was a problem hiding this comment.
Independent review (relayed Codex result), finding 2 of 3
P2 — Array expectations incorrectly preserve typed elements. Adding
ARRAY<DATE>containing[date(2017, 1, 2)]produces that same expected list. Without type hints, the cursor parses Athena's rendering into["2017-01-02"]. The rewritten tests therefore fail despite correct cursor behavior. The array rule needs to account for element representation.
There was a problem hiding this comment.
Repair in 9b8cfac: verified against pyathena/converter.py. _to_array tries json.loads first and otherwise splits the rendering into strings. PYTHON["array"] is now _python_array: it parses _athena_text(value, type) as JSON, or else returns each element's text rendering. Offline: ARRAY<date> gives ["2017-01-02"], ARRAY<string> gives ["a", "b"], ARRAY<int> with a null gives [1, None], and col_array still gives [1, 2]. The rule states the expected behavior; it does not call the converter.\n\nLimit: the text rendering of nested timestamps (2017-01-01 00:00:00.000) follows Athena's CSV format for top-level timestamps. No nested timestamp column exists, so it is not verified against Athena.
| "timestamp with time zone": lambda v, t: v.replace(tzinfo=timezone.utc), | ||
| "time": lambda v, t: v.time(), | ||
| "array": _as_list, | ||
| "map": lambda v, t: {str(k): str(x) for k, x in v}, |
There was a problem hiding this comment.
Independent review (relayed Codex result), finding 3 of 3
P2 — Python stringification does not match map/struct rendering. Adding
STRUCT<flag:BOOLEAN>with{"flag": True}produces{"flag": "True"}, while the untyped cursor returns{"flag": "true"}. The map rule has the same problem; nested nulls also become"None"instead ofNone. These rules need Athena-specific rendering semantics.
There was a problem hiding this comment.
Repair in 9b8cfac: verified. _athena_text now renders a boolean as true/false and a null as null, and _python_map/_python_struct turn nested nulls into None, as the converter's null handling does (pyathena/converter.py:455). Offline: STRUCT<flag: boolean> gives {"flag": "true"}, and MAP<string, int> with a null value gives {"a": None}. The derived one_row_complex rows are still equal to the removed literals, with the same value types. The five rewritten tests pass on AWS at 9b8cfac (5 passed).
| return None if value is None else _athena_text(value, athena_type) | ||
|
|
||
|
|
||
| def _python_array(value: Any, athena_type: str) -> list[Any]: |
There was a problem hiding this comment.
Self-review of the repair (both perspectives), range db7a792..9b8cfac on the same base 2760077
Round one (behavior): CLEAN
_athena_textchecksNonebefore it dispatches on the family._python_arraycatchesValueError, which also coversJSONDecodeError._type_argumentssplits only at top-level commas, so nested<...>/(...)arguments stay whole._struct_fieldssplits each field at its first colon.assert_sqlalchemy_typenow unpacks the single array argument. A malformed array type raises instead of passing.- The derived
one_row_complexrows are unchanged: equal to the removed literals, with the same value types. The five tests pass on AWS.
Round two (claims): FINDINGS (description only, fixed)
- The PR description said map and struct values "come back as strings" without mentioning nested rendering. It now explains how nested values are derived from Athena's text rendering, and the tested commit is 9b8cfac.
- Unverified, stated in the reply above: the rendering of nested timestamps. No column uses one, so no current assertion depends on it.
When an array's rendering is not JSON, the cursors parse its map and struct elements into dicts and leave nested arrays unparsed; the expected values now follow that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return json.loads(_athena_text(value, athena_type)) | ||
| except ValueError: | ||
| (element_type,) = _type_arguments(athena_type) | ||
| return [_element_text(e, element_type) for e in value] |
There was a problem hiding this comment.
Independent follow-up review (relayed Codex result): FINDINGS (1 × P2)
- Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e789-ce64-7462-b947-6d50afe8acd2. The review ran read-only on a detached snapshot at 9b8cfac and covered the range db7a792..9b8cfac. It was a static review only.
- The reviewer reports that all three earlier findings are resolved: outer parameters are isolated, date arrays produce strings, and nested booleans and nulls in maps and structs render correctly. It also reports that the current
one_row_complexexpectations are unchanged.
P2 — Array fallback incorrectly stringifies structs. For an
ARRAY<STRUCT<a:STRING>>column containing[{"a": "x"}], the new helper expects["{a=x}"]. The untyped converter parses[{a=x}]into[{"a": "x"}](pyathena/converter.py:309–316). The previouslist(value)expectation was correct for this case; adding this column to the shared table now causes false failures in callers ofSelection.rows(PYTHON).
There was a problem hiding this comment.
Repair in fdad71a: verified against _parse_array_native (pyathena/converter.py:286). Native items are strings or None, except {...} items, which are parsed as structs. Any item containing [, ], = or " makes the converter give up and return the raw string. When an array's rendering is not JSON, _python_array now returns dicts for map and struct elements and returns the rendering itself for nested arrays. To check the rule offline, the converter was run on the rendered text, as a verification step only; the tests do not call it. The rule and the converter agree on 11 shapes: ARRAY<STRUCT<a: string>>, ARRAY<ARRAY<string>>, ARRAY<MAP<int, string>>, ARRAY<date>, ARRAY<string> with a null, ARRAY<int>, ARRAY<boolean>, two maps, and two structs. The derived one_row_complex rows are still equal to the removed literals, and the five tests pass on AWS at fdad71a.
The rules for values nested in arrays, maps, and structs cover scalars and arrays of maps or structs of scalars. Deeper nesting, which the cursors parse with further heuristics, now raises NotImplementedError instead of producing an expectation the cursors would not meet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| pass | ||
| (element_type,) = _type_arguments(athena_type) | ||
| element_family = family(element_type) | ||
| if element_family == "array": |
There was a problem hiding this comment.
Independent follow-up review 2 (relayed Codex result): FINDINGS (2 × P2)
- Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e78d-24c3-75d1-80be-0f2d4c4235dd. The review ran read-only on a detached snapshot at fdad71a, covering the range 9b8cfac..fdad71a. It was a static review only.
- Result: the flat struct case is fixed, and the current
one_row_complexexpectations are unchanged.
P2 — Nested-array fallback checks only the immediate element type. For
ARRAY<STRUCT<a: ARRAY<int>>>containing[{"a": [1]}], the helper expects[{"a": "[1]"}]. However,_to_arraychecks for[anywhere inside the rendered array (converter.py:164) and returns the entire string"[{a=[1]}]".
P2 — Nested struct/map values remain incorrectly stringified. For
ARRAY<STRUCT<a: STRUCT<b: string>>>containing[{"a": {"b": "x"}}], the helper expects[{"a": "{b=x}"}]._parse_array_native→_to_structrecursively produces[{"a": {"b": "x"}}](converter.py:414–418). The map branch has the same mismatch for nested map values.
There was a problem hiding this comment.
Repair in cea2dd1: both scenarios verified in pyathena/converter.py.
Fixing one nested shape after another would turn these rules into a copy of the converter's heuristics. Instead, the rules now state the shapes they model and refuse the rest.
- Supported shapes:
- scalars nested in arrays, maps and structs;
- arrays of maps or structs whose members are scalars.
- Refused shapes: any array, map or struct nested one level deeper raises
NotImplementedError("No expectation rule for nested ...")through_scalar_type. A new column of such a type therefore fails with an explicit request to add a rule, instead of producing an expectation the cursors would not meet. - Offline check: the supported shapes still agree with the converter's output for the rendered text in 7 cases.
ARRAY<ARRAY<string>>,ARRAY<STRUCT<a: ARRAY<int>>>,ARRAY<STRUCT<a: STRUCT<b: string>>>andSTRUCT<a: STRUCT<b: int>>raise. - Current column: the derived
one_row_complexrows are still equal to the removed literals, and the five tests pass on AWS at cea2dd1.
The nested-value rules check the whole type before looking at values, so an unsupported type raises even for a null or empty value, and a null array, map, or struct stays None. The one_row_complex note adds that string values must differ from null, which Athena renders the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if element_family == "struct": | ||
| return [None if e is None else _python_struct(e, element_type) for e in value] | ||
| try: | ||
| return json.loads(_athena_text(value, athena_type)) |
There was a problem hiding this comment.
Independent follow-up review 3 (relayed Codex result): FINDINGS (3 × P2)
Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e791-545a-76c3-943d-80ef4051dab0. The review was read-only, on a detached snapshot at cea2dd1, and covered fdad71a..cea2dd1. It was a static review only. The reviewer confirmed that the current one_row_complex expectations are unchanged.
P2 — Unsupported nesting still returns values.
ARRAY<ARRAY<int>>with[[1]]returns[[1]]through JSON parsing without checking nesting. Validation also depends on populated, non-null values:ARRAY<STRUCT<a: ARRAY<int>>>with[{"a": None}]returns[{"a": None}], and an emptyMAP<string, ARRAY<int>>returns{}. Validate the complete type before processing values.P2 — Supported scalar strings can still produce incorrect expectations.
STRUCT<a: string>with{"a": "null"}produces{"a": "null"}. Athena's{a=null}rendering becomes{"a": None}through_parse_named_structand_convert_value.P2 — Null arrays of maps or structs now fail. A supported
ARRAY<MAP<int, int>>column containing SQL NULL raisesTypeErrorwhen the comprehension iteratesNone; arrays of structs fail identically. Before this repair, JSON parsing of_athena_text(None, ...)returnedNone, matching the converter.
There was a problem hiding this comment.
Repair in 562f7b5. All three findings were verified.
_check_nestingnow checks the whole type before any value is read. It runs in_python_array,_python_mapand_python_struct, so an unsupported type raises even for a null, empty or JSON-parsable value. Offline, all of these raiseNotImplementedError:ARRAY<ARRAY<int>>with[[1]], and withNoneARRAY<STRUCT<a: ARRAY<int>>>with[{"a": None}]- an empty
MAP<string, ARRAY<int>> STRUCT<a: STRUCT<b: int>>
- A nested string
"null"renders exactly like SQL NULL, so this is a constraint on the data rather than on the rules. TheONE_ROW_COMPLEXnote intables.pynow says that string values must differ fromnull, next to the existing separator constraint. - A null array, map or struct now returns
Nonebefore dispatch, as does a null map or struct element of an array. Offline,ARRAY<MAP<int, int>>,ARRAY<STRUCT<a: int>>,MAPandSTRUCTnulls giveNone, andARRAY<STRUCT<a: int>>with[{"a": 1}, None]matches the converter's[{"a": "1"}, None].
The 7 supported shapes still match the converter's output for the rendered text. The derived one_row_complex rows still equal the removed literals, and the five tests pass on AWS at 562f7b5.
Strings nested in arrays, maps, and structs, including map keys, may contain only letters, digits, underscores, and spaces and must not be null; the rules raise ValueError for others instead of expecting a value the cursors return differently. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| # One row with a value of each column type. Tests derive their expected results | ||
| # from this definition with tests.pyathena.expected. Keep string values free of | ||
| # the separators in Athena's text rendering of arrays, maps, and structs | ||
| # (, = [ ] { }) and different from null, because the cursors parse that |
There was a problem hiding this comment.
Independent follow-up review 4 (relayed Codex result): FINDINGS (1 × P2)
Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e794-a941-7d13-8501-992c7eba5ab3. The review ran read-only on a detached snapshot at 562f7b5 and covered the range cea2dd1..562f7b5. It was a static review only.
The reviewer confirmed:
- Deeper nesting now raises
NotImplementedErrorbefore any value is inspected, including null, empty, and JSON-parsable values. - Null containers and null map/struct array elements return
None. - The current
one_row_complexexpectations are unchanged.
P2 — Documented constraints still permit values the expectation rules misrepresent. For
MAP<string, string>with[("k", "x(y)")], both strings satisfy the documented restrictions. The expectation returns{"k": "x(y)"}, but Athena's rendering{k=x(y)}makes_to_mapreturn the original string, because it contains parentheses (pyathena/converter.py:221). Model this fallback or extend the documented data restriction.
There was a problem hiding this comment.
Repair in 1928399: verified.
The restriction is no longer a list of forbidden characters that could miss one; the code now enforces it. _element_text accepts a nested string value, including a map key, only if it consists of letters, digits, underscores, and spaces and is not null. For any other string it raises ValueError("Unsupported nested string value: ..."), rather than expecting a value the cursors would return differently. The ONE_ROW_COMPLEX note in tables.py now states the same rule. Top-level strings are not affected, because the cursors do not parse them.
Checked offline:
x(y)(map value),a(b)(map key),a,b(array element), andnull(struct field) are rejected.x y_1is accepted and matches_to_map.- The derived
one_row_complexrows are unchanged.
The five tests pass on AWS at 1928399.
Nested strings must be words of letters, digits, and underscores separated by single spaces, which excludes empty strings and surrounding spaces that the cursors strip or skip. Array elements are checked before the JSON parse as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if value is None: | ||
| return None | ||
| if family(athena_type) in ("string", "varchar", "char") and ( | ||
| not re.fullmatch(r"[\w ]*", value) or value.lower() == "null" |
There was a problem hiding this comment.
Independent follow-up review 5 (relayed Codex result): FINDINGS (3 × P2)
Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e797-e031-7d62-8c4e-80f09909d2d7. Read-only, detached snapshot at 1928399, range 562f7b5..1928399. Static only. The current one_row_complex expectations are unchanged.
P2 — Accepted surrounding spaces produce incorrect expectations.
MAP<string, string>with[(" key ", " null ")]is accepted and expects{" key ": " null "}, but the converter strips both strings (converter.py:358–368) and yields{"key": None}. Struct values and native-parsed array elements have the same problem.P2 — Accepted empty array strings disappear during conversion.
ARRAY<string>with["", "x"]passes the regex. Its rendering[, x]converts to["x"], because_parse_array_nativeskips empty items (converter.py:305–307).P2 — The restriction does not cover the array JSON path.
_python_arrayreturns the parsed JSON before calling_element_text, so["null"]returns[None]instead of raising the promisedValueError, and a string with literal quotes also bypasses the check. The outputs match the converter, but the stated rejection contract is not enforced everywhere.
There was a problem hiding this comment.
Repair in 4e205a3. All three findings were verified.
- 1 and 2: A nested string, including a map key, must now match
\w+(?: \w+)*: one or more words of letters, digits, and underscores, separated by single spaces. It also must not benull. This rejects empty strings, leading or trailing spaces, and repeated spaces, which the converter skips or strips. - 3:
_python_arraynow checks every element before it tries the JSON parse. The restriction therefore holds on both paths. - Offline, rejected:
[(" key ", " null ")],["", "x"],["null"],['"x"'], and{"a": "a b"}. - Offline, accepted and equal to the converter's output for the rendered text:
x y_1(map),["a b", "c"],["123", "true"](JSON path,[123, True]on both sides),{"a": "x"}, and[{"a": "x y"}]. - The
ONE_ROW_COMPLEXnote intables.pystates the same rule. The derivedone_row_complexrows are unchanged, and the five tests pass on AWS at 4e205a3.
Athena's text rendering of nested values is now stated per type: binary as hex bytes and decimal with its declared scale, alongside booleans, integers, strings, dates, and timestamps. Other types, such as float and double, raise NotImplementedError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return str(value).lower() | ||
| if name == "timestamp": | ||
| return value.isoformat(sep=" ", timespec="milliseconds") | ||
| return str(value) |
There was a problem hiding this comment.
Independent follow-up review 6 (relayed Codex result): FINDINGS (2 × P2)
Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e79b-0825-7840-af3f-a6bc51685538.
Scope: read-only, on a detached snapshot at 4e205a3, covering 1928399..4e205a3. The review was static only.
Confirmed:
- The string restriction covers every position, including the array JSON path.
- The
one_row_complexexpectations are unchanged. - The repair itself introduces no new defect.
Findings:
P2 — Nested binary uses Python's bytes representation. For
STRUCT<a: binary>with{"a": b"123"}, the rule expects{"a": "b'123'"}. Athena renders31 32 33, and the untyped struct converter returns{"a": "31 32 33"}.
P2 — Nested decimals ignore the declared scale. For
STRUCT<a: decimal(10,1)>with{"a": Decimal("1")}, the rule expects{"a": "1"}. Athena renders1.0, and the converter returns{"a": "1.0"}.
Both mismatches predate the reviewed repair, but they concern values the rules still accept.
There was a problem hiding this comment.
Repair in e9efc09: both findings verified.
The fallback str(value) in _athena_text is removed, so no type is rendered by accident any more. Each family that can be rendered now has an explicit rule:
- boolean:
true/false - integers, string, varchar, date:
str - timestamp: millisecond ISO format with a space separator
- binary: space-separated hex bytes (
31 32 33) - decimal: formatted with the declared scale (
1.0fordecimal(10,1))
Any other type raises NotImplementedError. That includes float and double, which Athena renders with Java formatting (for example 1.0E20).
Offline checks against the converter's output for the rendered text:
STRUCT<a: binary>gives{"a": "31 32 33"}, as the converter does.STRUCT<a: decimal(10,1)>gives{"a": "1.0"}, as the converter does.ARRAY<decimal(10,1)>gives[1.0]through the JSON path, as the converter does.STRUCT<a: double>raises.ARRAY<date>andMAP<int, boolean>still match.
The derived one_row_complex rows are unchanged, and the five tests pass on AWS at e9efc09.
The nesting check now also requires every nested scalar type to have a rendering rule, so an unsupported type raises even when its values are null or empty. A nested value rendered as empty text, such as empty binary, raises ValueError because the cursors skip empty array items. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if name == "timestamp": | ||
| return value.isoformat(sep=" ", timespec="milliseconds") | ||
| if name == "binary": | ||
| return " ".join(f"{b:02x}" for b in value) |
There was a problem hiding this comment.
Independent follow-up review 7 (relayed Codex result): FINDINGS (2 × P2)
Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e79e-5331-70f3-b539-15de0304acad. The review ran read-only on a detached snapshot at e9efc09 and covered 4e205a3..e9efc09. It was a static review only. The current one_row_complex expectations are unchanged.
P2 — Empty binary array elements produce incorrect expectations. For
ARRAY<binary>containing[b"", b"\x12"], Athena's text is[, 12]. The expectation returns["", "12"], but the converter's native array parser skips the empty item and returns["12"](pyathena/converter.py:305–307).
P2 — Unsupported nested types bypass the new rejection for null or empty values. For example,
PYTHON["array"]([], "ARRAY<double>")returns[], andPYTHON["struct"]({"a": None}, "STRUCT<a: double>")returns{"a": None}without raising. The rejection is reached only when a non-null scalar is rendered; the declared member types are never validated on their own.
There was a problem hiding this comment.
Repair in 756bdd1: both findings verified.
- Member types are now checked by type.
_check_nestingrequires every nested scalar type to be in_NESTED_SCALAR_FAMILIES: boolean, integers, string, varchar, date, timestamp, binary, and decimal, the same set that_athena_textrenders. The check runs before any value is read, so an unsupported type raises even when the value is null or empty. Offline,ARRAY<double>with[]andSTRUCT<a: double>with{"a": None}both raiseNotImplementedError. - Empty renderings are rejected.
_element_textraisesValueErrorfor any nested value that renders as empty text, such asb"", because the converter skips empty array items. Offline,[b"", b"\x12"]raises. - Values that should pass still match the converter. Offline,
[b"\x12"](JSON path,[12]),STRUCT<a: binary>,MAP<int, int>andARRAY<STRUCT<a: int>>agree with the converter's output for the rendered text. - Current tests are unaffected. The derived
one_row_complexrows are unchanged, and the five tests pass on AWS at 756bdd1.
Struct values are rendered and converted field by field in declared order, with a missing field as null, as the generated Parquet data stores it; a field the type does not declare raises ValueError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if value is None: | ||
| return None | ||
| field_types = dict(_struct_fields(athena_type)) | ||
| return {k: _element_text(x, field_types[k]) for k, x in value.items()} |
There was a problem hiding this comment.
Independent follow-up review 8 (relayed Codex result): FINDINGS (1 × P2)
- Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e7a1-2906-7d10-b02a-9fb28d8d14b6.
- Scope: read-only, on a detached snapshot at 756bdd1, covering e9efc09..756bdd1. This was a static review only.
- Result: the reviewer confirmed that both previous findings are resolved, that unsupported nested types raise before any value is inspected, and that the
one_row_complexexpectations are unchanged.
P2 — Accepted struct dictionaries can omit declared fields. For
STRUCT<a: int, b: int>with{"a": 1}, the rule returns{"a": "1"}. The schema-based Arrow construction intables.py:110–113fills the missing field with null, so Athena renders{a=1, b=null}and the converter returns{"a": "1", "b": None}. This also affects arrays of such structs. Iterate over the declared fields and supply the missing nulls, or reject incomplete dictionaries.
There was a problem hiding this comment.
Repair in c0b2e3c: verified.
Change:
- A new helper,
_struct_items, walks a struct type's declared fields in order and takes a missing field as null, matching whatpa.Table.from_pylistwrites for it. - It raises
ValueErrorfor a field the type does not declare. _athena_textand_python_structnow both use it, so arrays of structs are covered as well.
Checked offline against the converter's output for the rendered text:
{"a": 1}forSTRUCT<a: int, b: int>gives{"a": "1", "b": None}, the same as the converter.- The same value in
ARRAY<STRUCT<a: int, b: int>>gives the same result. {"b": 2, "a": 1}renders in declared order.{"a": 1, "c": 3}is rejected.
The derived one_row_complex rows are unchanged, and the five tests pass on AWS at c0b2e3c.
There was a problem hiding this comment.
Independent follow-up review 9 (relayed Codex result): CLEAN
- Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e7a7-a446-7f53-a431-969698bc73cc.
- Scope: read-only review of a detached snapshot at c0b2e3c, covering 756bdd1..c0b2e3c. It was a static review only; the snapshot was unchanged afterwards.
- What the reviewer traced:
- supported scalar nesting, and arrays of scalar maps/structs, including nulls, empty containers, omitted or reordered fields, and rejection of undeclared fields;
- unsupported nested types, which still raise
NotImplementedError, including for null or empty values; - the current
one_row_complexexpectations, which are unchanged.
- Result: "No concrete accepted-value mismatch or defect introduced by the repair found."
tests/pyathena/expected.py now covers the column types of the shared tables instead of modeling every nesting the cursors can parse, and its names say what they are: ExpectedResult (was Selection), CastColumn (was Cast), base_type (was family), and PYTHON_VALUES (was PYTHON). A module docstring explains how to read it. assert_sqlalchemy_type is a test assertion rather than an expected value, so it moves to tests/pyathena/util.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A null nested in an array, map, or struct renders as null and comes back as None, and struct fields render in their declared order, not in the order of the definition's dict. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| ) | ||
|
|
||
|
|
||
| def _athena_text(value: Any, athena_type: str) -> str: |
There was a problem hiding this comment.
Independent review of the refactor (relayed Codex result): FINDINGS (2 × P2), both repaired in 36988f4
Reviewer: Codex CLI 0.157.1, model gpt-6-astra, session 01a0e88a-73e9-79d1-b521-6d185cab0805. It was a read-only static review of a detached snapshot at 7845405 (#876 head), covering c0b2e3c..0b4d138 (this PR's simplification and renaming) and 0b4d138..7845405 (#876 rebuilt). It ran without .env or PR framing.
Covered, as reported: expectation rules, renamed callers, the SQLAlchemy helper relocation, pandas/Arrow/Polars assertions, descriptions, DB-API and Python types, dtypes, schemas, and the fixture round-trip guard. The reviewer found no stale references and no circular converter use, and noted that neither finding affects the current fixture values or ARRAY<string> ["a", "b"].
P2 — Nested nulls silently produce incorrect expectations. For
ARRAY<int>[1, None],_athena_textgenerated"[1, None]"and_python_arrayexpected["1", "None"]; Athena's[1, null]parses as[1, None]. Null map values and struct fields likewise became"None".
P2 — Struct text depends on dictionary insertion order.
{"b": 2, "a": 1}forSTRUCT<a: int, b: int>rendered"{b=2, a=1}"instead of"{a=1, b=2}", and the round-trip guard accepts it because dict equality ignores order.
Repair (verified): _athena_text renders a null as null; _member_value turns nested nulls into None for array elements, map values, and struct fields; struct fields render and convert in declared order. The following were checked offline against the converter's output for the rendered text and match: [1, None], ["a", None], MAP<int, int> with a null value, {"b": 2, "a": None}, and ["a", "b"]. The derived one_row_complex rows are unchanged, and the five tests pass on AWS at 36988f4. #876–#878 were rebased onto it: 20 passed and 14/14 equal for #876.
|
Closing without merging, by the maintainer's decision. Deriving the expected results from the table definitions required tests/pyathena/expected.py to re-model how each cursor returns every type: Athena's text rendering, JSON parsing, and the pandas, Arrow, and Polars types. The module grew large enough to need tests of its own, which is more complexity than the one-place column addition is worth. The hand-written expectations stay. See #848 for the summary. The branch is kept for reference. |
WHAT
tests/pyathena/expected.pyderives the expected query results from a table defined intests/pyathena/tables.py. Its module docstring explains how to read it.ExpectedResult(table, casts=...)is the entry point.CastColumnitems such asTIMESTAMP_TZ,TIME_OF_TIMESTAMP,ARRAY_JSON, andMAP_JSON.rows(values),description(),dbapi_types(), andnames.PYTHON_VALUESholds the value rules of the cursors that return Python objects: Cursor, S3FSCursor,pandas.util.as_pandas, and SQLAlchemy rows.decimalforDECIMAL(10,1)).[1, 2]), and other nested values come back as text ({"a": "1"}).timestamp with time zoneas a UTC datetime.NotImplementedError. Such a column needs a rule first.tests/pyathena/util.py: newassert_sqlalchemy_type. It checks a reflected column type against the definition, including the varchar length, the decimal precision and scale, and the array item type.tests/pyathena/tables.py:one_row_complexis now the named constantONE_ROW_COMPLEX. Its note states the constraint on nested string values.TestCursor.test_complexTestS3FSCursor.test_complexandTestAsyncS3FSCursor.test_complexpandas/test_util.py::test_as_pandasTestSQLAlchemyAthena.test_reflect_selectAssertion changes:
test_as_pandasnow includescol_varchar, which it did not select before. It also asserts the DataFrame column names.test_reflect_selectcompares the reflected column names with the definition, instead oflen(...) == 16.WHY
Part of #848 (step 2), for #834. Stacked on #874.
Each of these tests repeated the
one_row_complexrow, description, and types by hand. Adding a column meant editing every one of them. With this PR, these five tests pick up a new column from its definition. The rules need a change only when the column's type is not covered by the existing rules. The pandas, Arrow, and Polars cursor tests follow in the next PR, and a column added only through the definition follows after that.TEST
Tested commit: 36988f4. At the maintainer's request, the expectation code was simplified and renamed in d1db7dc, after the independent reviews below. 36988f4 fixes the two findings of the independent review of that refactor.
just lintpassed.test_complexselection were compared with the removed hand-written values. The rows are equal, with the same Python type for every value. The description tuples are equal, and so are the DB API type objects.pytest -n 2on the five rewritten tests gave 5 passed on each pushed revision, a9f4db9 through 36988f4. The offline comparison with the removed literals was rerun on 36988f4 and still shows no difference.tests/pyathena/sqlalchemy/changed, both SQLAlchemy compliance suites.🤖 Generated with Claude Code