Split SQLAlchemy type rendering into Hive DDL and Trino DML compilers - #961
Conversation
Athena parses DDL with Hive type syntax and queries with Trino type syntax. AthenaTypeCompiler mixed both through the type_expression column check and the _athena_hive_ddl flag, so direct compilation produced hybrids such as ROW(name STRING) and invalid DDL for empty STRUCT, JSON, and CLOB columns. AthenaTypeCompiler now renders Hive DDL types only: INT, STRING, STRUCT<name:type>, MAP<k, v>, and ARRAY<t> in every context. JSON and a STRUCT without fields raise CompileError, CLOB/NCLOB render STRING, and an unexpected type class in visit_struct/map/array raises instead of falling back to a placeholder. The new AthenaDMLTypeCompiler renders the Trino types of CAST expressions and replaces the type branches of visit_cast and _complex_dml_type. CAST output is unchanged except that an empty ROW now raises CompileError. Refs #886 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous visit_cast matched String, LargeBinary, Float, and DateTime with isinstance, so a subclass with its own __visit_name__ still cast as VARCHAR, VARBINARY, REAL, or TIMESTAMP(6). The DML type compiler dispatches by visit name, so it now falls back to the nearest base class that it renders instead of raising UnsupportedCompilationError. Also correct the AthenaTypeCompiler docstring: String with a length renders STRING, not VARCHAR(n). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return self.visit_array(type_, **kw) | ||
|
|
||
|
|
||
| class AthenaDMLTypeCompiler(GenericTypeCompiler): |
There was a problem hiding this comment.
Self-review round one (implementation behavior, public contracts, simplicity, regression coverage)
Base 23ff61f6ecff6c74f5e7864641e3ea1a76f5e0a8, head 268f8d6b959e19f01a7666f67afab8ed087ee990. Result: FINDINGS (2, both repaired in e8812ad6900d3f9be77970ac89a83512000bbe4e).
Covered: every file in git diff 23ff61f6ecff6c74f5e7864641e3ea1a76f5e0a8..268f8d6b959e19f01a7666f67afab8ed087ee990.
- DDL type compiler: Hive rendering of STRUCT/MAP/ARRAY at every depth, removal of
_athena_hive_ddland theget_column_specificationINT case,JSON/empty STRUCT/CLOB, andCompileErrorfor unexpected classes. - DML type compiler: variant and TypeDecorator resolution (
_ArrayTypeInspector.dialect_type, which is the former_dialect_type), therequire_precision/timestamp_precisionoptions, NULL element rejection, and quoting with the DML preparer. - Callers:
visit_cast,_array_slice_step,_array_json, and the three_ArrayUpdateCompilercall sites. - Untyped-array update: an element assignment now renders the RHS type before
_rebuildvalidates the array type. The statement still raises the same "explicit element type" error. - Comparison script, 58 types: every CAST line is identical to master, apart from the intended empty-ROW error.
Findings:
- A type subclass with its own
__visit_name__regressed in CAST (see the comment onvisit_unsupported_compilation). - The class docstring was inaccurate for
String(n)(see the comment on the docstring).
Not changed: TIME still raises in CAST, as on master. Widening that is out of scope.
| return self.process(type_, **kw) | ||
|
|
||
| @override | ||
| def visit_unsupported_compilation( # type: ignore[override] # base returns NoReturn |
There was a problem hiding this comment.
Round one, finding 1 (repaired in e8812ad6900d3f9be77970ac89a83512000bbe4e).
On 268f8d6b959e19f01a7666f67afab8ed087ee990, cast(x, MyStr()) raised UnsupportedCompilationError, where class MyStr(String): __visit_name__ = "mystr". Master renders it as CAST(x AS VARCHAR), because the old visit_cast and _complex_dml_type matched with isinstance(type_, types.String), and likewise for LargeBinary, Float, Double, and DateTime.
Repair: visit_unsupported_compilation now walks the MRO and uses the nearest base __visit_name__ that this compiler handles. test_cast_renders_subclass_with_own_visit_name_as_base covers the scalar case and the ARRAY element case; the ARRAY case fails on 268f8d6b959e19f01a7666f67afab8ed087ee990. DDL behavior is unchanged, and master already raised there.
There was a problem hiding this comment.
Superseded in 8570146577. Independent review showed that the visit-name fallback added here did not cover a subclass whose visit name names another handled type (e.g. oracle.DATE). AthenaDMLTypeCompiler.process() now matches these base classes with isinstance after resolution, as base visit_cast did, and visit_unsupported_compilation has been removed. The String-subclass case from this finding is still covered, by test_cast_renders_subclass_by_base_class.
| and ``TypeEngine.compile()``. ``AthenaStatementCompiler`` renders the | ||
| types of CAST expressions with ``AthenaDMLTypeCompiler``. | ||
|
|
||
| INTEGER renders as INT, FLOAT and REAL as FLOAT, and binary types as |
There was a problem hiding this comment.
Round one, finding 2 (repaired in e8812ad6900d3f9be77970ac89a83512000bbe4e).
On 268f8d6b959e19f01a7666f67afab8ed087ee990, the docstring said that "character types ... with a length render as CHAR(n) or VARCHAR(n)". String(10) and Unicode(5) render STRING through visit_string/visit_unicode, so the statement was wrong. Reworded to name CHAR, NCHAR, VARCHAR, and NVARCHAR.
Athena's DDL parser accepts INTEGER inside nested types (#886), so INT is not required there, and the empty STRUCT sentence only needs the fact. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return self.process(type_, **kw) | ||
|
|
||
| @override | ||
| def visit_unsupported_compilation( # type: ignore[override] # base returns NoReturn |
There was a problem hiding this comment.
Self-review round two (factual claims, caller compatibility, operational effects, evidence)
Base 23ff61f6ecff6c74f5e7864641e3ea1a76f5e0a8, head e8812ad6900d3f9be77970ac89a83512000bbe4e. Result: FINDINGS (3, repaired in 684eccd4fa1d5e801e94c66cceed31d4b9d84b94 and in the PR body).
Claims checked:
- PR body, "CAST output unchanged except empty ROW". The 58-type comparison supports it, and the round-one fallback restores subclasses of String, binary, Float, and DateTime. Finding: the fallback is also more lenient than master. An
Integer/Numericsubclass with its own__visit_name__now casts asINTEGER/DECIMAL, where master raisedUnsupportedCompilationError. Measured and added to the PR body's behavior changes. - "DDL and CAST types" table rows (INT/INTEGER; STRING/VARCHAR for String, Text, CLOB; BINARY/VARBINARY; JSON; STRUCT/ROW; MAP; ARRAY): each matches the comparison output on
e8812ad6900d3f9be77970ac89a83512000bbe4e. - MAP section "CREATE TABLE renders integer MAP keys and values as INT. CAST still spells those integers as INTEGER": still true.
TIMESTAMP(6)statement at docs/sqlalchemy.md:162: still true (visit_TIMESTAMP/visit_DATETIME).- DDL and DML class docstrings: checked against the comparison output after the round-one docstring repair.
Callers:
visit_unsupported_compilationexists onTypeCompilerin SQLAlchemy 2.0, and the package requires SQLAlchemy 2.0.- The removed private helpers
_complex_dml_type,_dialect_type,_timestamp_dml_type, and_enable_hive_column_ddlhave no callers left in the repository. TypeEngine.compile()output changes for STRUCT/MAP/INT, and Alembicalter_columnuses that path; both are listed as behavior changes.- AWS operations: none changed (compile-time only).
Evidence: the targeted AWS SQLAlchemy tests were rerun on 684eccd4fa1d5e801e94c66cceed31d4b9d84b94 (651 passed). The SQLA compliance suites ran on 268f8d6b; the PR body says so, and CI runs them on Ready.
There was a problem hiding this comment.
Correction after 8570146577: the leniency noted here (an Integer/Numeric subclass with its own visit name casting instead of raising) no longer applies. The fallback was replaced by class matching that mirrors base, so those types raise UnsupportedCompilationError again, as on master. The PR body now lists a different deliberate extension: a TypeDecorator's @compiles rule also applies inside ARRAY/MAP/ROW casts.
| | `AthenaArray`, `ARRAY` | `ARRAY<item>` | `ARRAY(item)` | | ||
|
|
||
| Complex types apply the same syntax to their nested types. | ||
| An `AthenaStruct` without fields raises `CompileError` in both. |
There was a problem hiding this comment.
Round two finding (repaired in 684eccd4fa1d5e801e94c66cceed31d4b9d84b94). The sentence "...raises CompileError in both, because Athena has no empty STRUCT type" carried a rationale aside, which user docs leave out. It now states only the behavior.
|
|
||
| Compared with earlier releases, reflected ARRAY columns are no longer reported as `String`. | ||
| ARRAY DDL now renders integer elements as `INT` and row elements as `STRUCT<...>`, which Athena requires for nested DDL types. | ||
| ARRAY DDL now renders integer elements as `INT` and row elements as `STRUCT<...>`. |
There was a problem hiding this comment.
Round two finding (repaired in 684eccd4fa1d5e801e94c66cceed31d4b9d84b94). This pre-existing sentence, outside the original diff, said INT is "required for nested DDL types". #886 measured that CREATE EXTERNAL TABLE ... (a MAP<STRING, INTEGER>) parses, so the requirement was false for INT. Removed the clause; the section otherwise matches the new type syntax section.
Independent review found three CAST regressions from dispatching by visit name: - A subclass whose visit name names another handled type (for example oracle.DATE, a DateTime subclass named DATE) rendered by that name instead of TIMESTAMP(6). - A compilation rule registered for a TypeDecorator stopped applying, because the decorator was resolved before dispatch. - An ARRAY assignment whose value type resolves to NullType rendered CAST(... AS NULL) instead of raising, because only nested elements were checked. AthenaDMLTypeCompiler.process() now matches ARRAY, MAP, ROW, String, binary, Double, Float, and DateTime types by class after resolving variants and decorators, as the former visit_cast did, and dispatches the declared type otherwise. The former _complex_dml_type callers use process_element(), which rejects an unknown type at any level. This replaces the visit-name fallback added earlier, and a decorator's compilation rule now also applies inside ARRAY, MAP, and ROW casts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return _ArrayTypeInspector(self.dialect) | ||
|
|
||
| @override | ||
| def process(self, type_: TypeEngine[Any], **kw: Any) -> str: |
There was a problem hiding this comment.
Independent review (relayed Codex result)
- Reviewer: Codex CLI 0.160.0, model
gpt-6-astra(a different model from the author). Session01a1006f-c6dd-74e2-bdd2-622be4c0e40f. - Invocation:
codex exec --sandbox read-onlyon a detached snapshot of684eccd4fa1d5e801e94c66cceed31d4b9d84b94(base23ff61f6ecff6c74f5e7864641e3ea1a76f5e0a8). The prompt gave the literal diff range and Athena syntax facts, and omitted the PR number, description, commit messages, and earlier findings. No edits, builds, tests, or network access were allowed. The snapshot was verified unchanged afterwards. - Coverage, as reported: base vs head DDL/CAST rendering (decorators, variants, subclasses, nested types); compiler callers in
pyathena/(ARRAY bind, slice, update, JSON projection); tests, docs, docstrings, and simplicity. - Result: FINDINGS (4). This was a static review; the scenarios were traced from source, not executed.
- P2, introduced: a subclass whose visit name names another handled type, e.g.
type("T", (DateTime,), {"__visit_name__": "DATE"}), cast asDATEinstead ofTIMESTAMP(6). The visit-name fallback only covered missing visitors. - P2, introduced:
cast(x, T()), whereTis a TypeDecorator over Integer with@compiles(T, "awsathena")returning BIGINT, renderedINTEGERinstead of base'sBIGINT, because the decorator was resolved before dispatch. - P2, introduced: a column of type
AthenaArray(NullType()).with_variant(AthenaArray(Integer), "awsathena")witht.update().values({t.c.a[1]: 1})renderedCAST(... AS NULL)instead of raising, because the RHS call bypassed the element check. - P3, test:
"decorated INT" in ddlalso passes fordecorated INTEGER.
Verification and repair (857014657720868939b8132c6dd89fa62a585315).
- All four were reproduced (findings 1-3 by the new tests failing on
684eccd4fa1d5e801e94c66cceed31d4b9d84b94) and accepted. Finding 1 has a real-world instance:sqlalchemy.dialects.oracle.DATEsubclasses DateTime with the visit name DATE. process()now resolves variants and decorators, then matches ARRAY/MAP/ROW/String/binary/Double/Float/DateTime by class, as basevisit_castdid. Other types dispatch the declared type, so decorator rules apply.- The former
_complex_dml_typecallers useprocess_element(), which rejects an unknown type at any level. - The round-one
visit_unsupported_compilationfallback is removed. An Integer/Numeric subclass with an unknown visit name raises again, as on master. - One deliberate extension: a decorator's compilation rule now also applies inside ARRAY/MAP/ROW casts. Master applied it only to a top-level CAST, and DDL already applies it at every depth.
- Validation: new tests
test_cast_renders_subclass_by_base_class(5 bases × 4 visit names),test_cast_applies_compilation_rule_of_decorator, andtest_array_assignment_rejects_unknown_value_type. 17 fail on684eccd4fa1d5e801e94c66cceed31d4b9d84b94; all except the nested decorator assertion pass on master'spyathena/. The 58-type comparison against base is unchanged apart from the empty-ROW errors. Offline: 480 passed. AWStests/pyathena/sqlalchemy/+ aio: 668 passed on857014657720868939b8132c6dd89fa62a585315.
There was a problem hiding this comment.
Independent follow-up (relayed Codex result): Codex CLI 0.160.0, model gpt-6-astra, session 01a10081-3fdf-7c80-8ba9-bb77c34fb0b9. Run with codex exec --sandbox read-only on a detached snapshot of 85701465. It was given git diff 684eccd4..85701465 and git range-diff 23ff61f6..684eccd4 23ff61f6..85701465. Static review; the snapshot was verified unchanged.
Result: FINDINGS (1). All four earlier findings were confirmed fixed.
- P2, introduced by the repair: a Numeric TypeDecorator with an
@compilesrule returningDECIMAL, used asAthenaArray(D())with a literal[Decimal("1.23")], renderedCAST(ARRAY[1.23] AS ARRAY(DECIMAL))instead of raising the precisionCompileError. Dispatching the declared nested decorator skippedvisit_DECIMAL. The docstring's precision claim did not hold for that case either.
Repair (084d773b): accepted and reproduced. process_element() now resolves the type before dispatch, as base _complex_dml_type did, so element types never use a decorator's rule. Only a top-level CAST (process()) dispatches the declared type. This drops the nested decorator-rule extension from 85701465; the PR body is updated.
test_array_bind_rejects_decorated_numeric_without_precisionwas added, and the nested assertion oftest_cast_applies_compilation_rule_of_decoratornow expectsARRAY(INTEGER). Both fail on85701465.- With master's
pyathena/and this test file, exactly the 25 intended DDL/empty-ROW/JSON tests fail; every base-behavior guard passes. - 58-type comparison: identical to base apart from the empty-ROW errors. Offline: 481 passed. AWS
tests/pyathena/sqlalchemy/+ aio on084d773b: 669 passed.
Self-review of this repair:
- Behavior: CLEAN. Every element path goes through
process_element():visit_array/visit_map/visit_struct, the three array.py callers, and_array_slice_step. So the precision check, the NullType check, and resolution all apply to the resolved type, as in base.visit_caststill usesprocess(). - Claims: CLEAN. The
AthenaDMLTypeCompilerandprocess_element()docstrings now state which path dispatches the declared type, and the PR body no longer claims the nested extension.
There was a problem hiding this comment.
Second independent follow-up (relayed Codex result): Codex CLI 0.160.0, model gpt-6-astra, session 01a1008c-1dd4-71a0-b653-79364e49acd5. Run with codex exec --sandbox read-only on a detached snapshot of 084d773b, reviewing git diff 85701465..084d773b against base 23ff61f6 semantics. Static review; the snapshot was verified unchanged.
Result: CLEAN. The reported defect is fixed for ARRAY elements, MAP keys/values, and ROW fields. No repair-introduced difference from base was found in any former _complex_dml_type caller: partial-assignment RHS, rebuild, padding, or _array_slice_step. Both changed tests would fail on the pre-repair implementation, and the docstrings match the behavior.
| raise exc.CompileError("An ARRAY slice assignment requires a non-NULL array") | ||
| rhs = compiler.process(value, **kw) | ||
| rhs_type = compiler._complex_dml_type(expression.value_type, require_precision=True) | ||
| rhs_type = compiler._dml_type_compiler.process_element( |
There was a problem hiding this comment.
Codex finding 3 (repaired in 857014657720868939b8132c6dd89fa62a585315): this RHS type now goes through process_element(), so NullType (including one reached through with_variant) raises "Bound ARRAY values require an explicit element type" as on base. Covered by test_array_assignment_rejects_unknown_value_type.
| assert "empty ROW()" in ddl | ||
| assert "filled STRUCT<n:INT>" in ddl | ||
| assert "STRUCT<>" not in ddl | ||
| assert "decorated INT,\n" in ddl |
There was a problem hiding this comment.
Codex finding 4 (repaired in 857014657720868939b8132c6dd89fa62a585315): assertions now include the delimiter (INT,\n), and the test asserts INTEGER does not appear.
| # TypeDecorator still applies. | ||
| return super().process(type_, **kw) | ||
|
|
||
| def process_element(self, type_: TypeEngine[Any], **kw: Any) -> str: |
There was a problem hiding this comment.
Self-review of the independent-review repair (git range-diff 23ff61f6..684eccd4fa1d5e801e94c66cceed31d4b9d84b94 23ff61f6..857014657720868939b8132c6dd89fa62a585315; the merge-base is unchanged)
Round-one perspective (behavior, contracts, simplicity, tests): CLEAN.
process(): afterdialect_type()resolution, ARRAY/MAP/ROW/String/binary/Double/Float/DateTime match by class in the same order as basevisit_cast/_complex_dml_type. Other types dispatch the declared type throughGenericTypeCompiler.process: the variant mapping applies, thenvisit_type_decoratorre-entersprocess()with the impl. There is no recursion loop, becausetype_engine()returns the impl.- Probed: a decorator whose impl has a variant →
VARCHAR;Boolean().with_variant(Date())→DATE;Uuid→CHAR(32); a top-level NullType decorator →CAST(x AS NULL)(as base); a decorator inside MAP → resolved. process_element()covers every former_complex_dml_typecaller: three in array.py and_array_slice_step.visit_castkeepsprocess(), so a top-level NullType rendersNULLas on base.- Removed per-name visitors (CHAR/VARCHAR/TEXT/CLOB/BLOB/BINARY/DATETIME/...) are unreachable for their classes now that class matching runs first.
- Tests fail on
684eccd4fa1d5e801e94c66cceed31d4b9d84b94(17) and assert exact SQL.
Round-two perspective (claims): CLEAN after the PR body update.
- The PR body's WHAT/behavior/TEST sections now describe class matching,
process_element(), the nested decorator-rule extension, and the commits each result belongs to. The superseded round-one/round-two claims are corrected in their threads. - The
AthenaDMLTypeCompilerdocstring matchesprocess(). The docs table is unaffected, and the 58-type CAST comparison is unchanged.
Dispatching a nested TypeDecorator by its declared type let a registered compilation rule bypass the ARRAY decimal precision check, for example AthenaArray of a Numeric decorator whose rule returns DECIMAL. The former _complex_dml_type resolved every element before rendering it, so process_element() now does the same, and only a top-level CAST dispatches the declared type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The former _complex_dml_type rejected any Numeric without precision by class, but the check had moved into visit_DECIMAL, so a Numeric subclass with its own visit name and compilation rule skipped it. Check the resolved type in process() before dispatch, as before. Also cover a with_variant() Integer column, whose variant DDL now applies instead of the former INT special case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return _ArrayTypeInspector(self.dialect) | ||
|
|
||
| @override | ||
| def process(self, type_: TypeEngine[Any], **kw: Any) -> str: |
There was a problem hiding this comment.
Additional independent review (relayed Claude Fable 5.1 result, requested by the maintainer)
- Reviewer: Claude Code 2.1.287
claude -p, modelclaude-fable-5-1, efforthigh, profilemax. Auth was verified first: claude.ai, first-party, subscriptionmax, not Enterprise, no API key override. Session90d6c4b8-0ada-466e-a868-1b5962c02cf3. - Package: a fresh detached snapshot of
084d773b1f384c3aae2526aec92ecb26edab492e(base23ff61f6ecff6c74f5e7864641e3ea1a76f5e0a8), without.serena/,.env, or local memory. MCP servers and hooks were disabled. The prompt gave the diff range, Athena syntax facts, and the intended behavior, and omitted the PR number, description, commits, and earlier findings. - Constraint deviation: the reviewer ran
uv run --no-sync python -c ...to locate the installed SQLAlchemy, which created an empty ignored.venv/in the snapshot. No tracked file changed, and no tests or builds ran, so this remains a static review. - Coverage, as reported: the full diff vs base
visit_cast/_complex_dml_type/_timestamp_dml_type/_dialect_type; every class-matched family and the fall-through types (TypeDecorator with/without@compiles,with_variant, own-__visit_name__subclasses, nested and multi-dimensional types); DDL throughget_column_specificationandTypeEngine.compile(); callers; SQLAlchemy 2.0.46 internals; tests and docs. - Result: FINDINGS (3). The reviewer also reported that the design split, docstrings, and tests are sound, with no action needed.
Dispositions (each verified by running a script on master 23ff61f6ecff6c74f5e7864641e3ea1a76f5e0a8 and on 084d773b1f384c3aae2526aec92ecb26edab492e):
- Medium, "
try_cast(col, JSON())renderedTRY_CAST(col AS JSON)at base and now raises": rejected. On both master and084d773b1f384c3aae2526aec92ecb26edab492e,try_cast(...)raisesUnsupportedCompilationErrorfromAthenaStatementCompilerfor JSON, STRUCT, Integer, String, and Float targets. SQLAlchemy 2.0.46'sSQLCompilerhas novisit_try_cast, so the type clause is never reached. SupportingTRY_CASTwould be a new feature, outside this PR. - Low,
Column("id", Integer().with_variant(BigInteger(), "awsathena")): confirmed, accepted as an improvement. It rendersid INTon master (the removedtype() in [Integer, ...]special case ignored the variant) andid BIGINThere. Covered intest_integer_subclass_and_decorator_columns_use_intand added to the PR body's behavior changes. - Low, a
Numericsubclass with its own__visit_name__and an@compilesrule used as an ARRAY bind element: confirmed regression, repaired in3c61716555cbfd83ca58da1b3f616e8f8b612d2a. Master raises "explicit Numeric precision";084d773b1f384c3aae2526aec92ecb26edab492erenderedARRAY(DECIMAL). See the comment on the precision check.
Validation of 3c61716555cbfd83ca58da1b3f616e8f8b612d2a: lint passed; offline 482 passed; the 58-type CAST comparison is identical to base apart from the empty-ROW errors; AWS tests/pyathena/sqlalchemy/ + aio: 670 passed.
There was a problem hiding this comment.
Independent follow-up (relayed Claude Fable 5.1 result): claude -p, model claude-fable-5-1, effort high, profile max (first-party Max), session 30d2d470-6652-4e50-9617-785262ae1484. Run on a fresh detached snapshot of 3c617165, reviewing git diff 084d773b..3c617165 against base 23ff61f6 semantics. MCP servers and hooks were disabled, and uv/python/pip were additionally denied. The snapshot was verified unchanged afterwards; static review.
Result: CLEAN.
- The precision check now runs in
process()after the String/binary/Double/Float/DateTime branches, in the same order as base, and**kwreaches it for ARRAY elements, MAP keys/values, ROW fields, and the assignment value cast. - Call sites without
require_precisionare unchanged. - Removing the
visit_DECIMALoverride keeps theDECIMAL/DECIMAL(p)/DECIMAL(p, s)rendering. - The new test fails on
084d773b, and the docstrings match.
Non-blocking note: the assignment path is tested with plain Numeric() only (test_decimal_assignment_requires_precision). It shares the same process() check, so no change is made.
| if isinstance(resolved, (types.DateTime, AthenaTimestamp)): | ||
| return self.visit_TIMESTAMP(resolved, **kw) # type: ignore[arg-type] | ||
| if ( | ||
| kw.get("require_precision") |
There was a problem hiding this comment.
Fable finding 3 (repaired in 3c61716555cbfd83ca58da1b3f616e8f8b612d2a). The precision check had moved into visit_DECIMAL, so a dispatch that bypasses it skipped the check. Example: a Numeric subclass with visit name pyathena_money and @compiles(..., "awsathena") returning DECIMAL, used as AthenaArray(_Money()) with literal([Decimal("1.23")]). The check now runs in process() on the resolved type, before dispatch, at the same point as base _complex_dml_type (after the class-matched families), and the visit_DECIMAL override is removed. test_array_bind_rejects_numeric_subclass_without_precision covers it; the reproduction script shows 084d773b1f384c3aae2526aec92ecb26edab492e rendering instead of raising.
Self-review of this repair. Behavior: CLEAN. Every require_precision path goes through process(): visit_cast for complex binds, and process_element() for elements and the ARRAY assignment RHS. Float and Double are matched earlier, so they are never rejected, as on base. Claims: CLEAN. The require_precision description in the class docstring still holds, and the PR body is updated.
| else: | ||
| # type_expression marks column DDL so STRUCT and MAP use Hive syntax. | ||
| type_ = self.dialect.type_compiler_instance.process(column.type, type_expression=column) | ||
| type_ = self.dialect.type_compiler_instance.process(column.type, type_expression=column) |
There was a problem hiding this comment.
Fable finding 2 (accepted). With the INT special case gone, Integer().with_variant(BigInteger(), "awsathena") renders BIGINT here (master: INT). The variant is now respected, which is listed in the PR body and asserted in test_integer_subclass_and_decorator_columns_use_int.
WHAT
Split SQLAlchemy type rendering into a Hive DDL type compiler and a Trino DML type compiler.
AthenaTypeCompiler(the dialect'stype_compiler_cls) renders Hive DDL types only.It renders CREATE TABLE column types and
TypeEngine.compile().STRUCT<name:type>,MAP<key, value>,ARRAY<item>, andINTin every context, including direct compilation. Before, direct compilation of a STRUCT or MAP rendered a hybrid such asROW(name STRING).type_expression=Columncheck, the_athena_hive_ddlflag, and theINTspecial case inget_column_specificationare removed.JSONand anAthenaStructwithout fields raiseCompileError. This applies at any nesting depth (for exampleARRAY<JSON>).CLOB/NCLOBrenderSTRINGinstead ofBINARY.visit_struct,visit_map, andvisit_arrayraiseCompileErrorfor a type of an unexpected class, instead of renderingROW(),MAP<STRING, STRING>, orARRAY<STRING>.AthenaDMLTypeCompilerrenders the Trino types of CAST expressions:VARCHAR,INTEGER,REAL,VARBINARY,TIMESTAMP(6)/TIMESTAMP(p),JSON,ROW(name type),MAP(key, value), andARRAY(item).It replaces the type branches of
AthenaStatementCompiler.visit_castand_complex_dml_type, and it keeps theirrequire_precisionandtimestamp_precisionoptions as keyword arguments ofprocess().An empty
AthenaStructraisesCompileErrorhere too, instead of renderingROW().After resolving variants and TypeDecorators, it matches ARRAY, MAP, ROW, String, binary, Double, Float, and DateTime types by class, as the former
visit_castdid, so a subclass renders like its base whatever its visit name (for exampleoracle.DATErendersTIMESTAMP(6)). For other types, a top-level CAST dispatches the declared type, so a compilation rule registered for a TypeDecorator still applies there.ARRAY/MAP/ROW elements and the former
_complex_dml_typecallers useprocess_element(), which resolves the type first and rejects an unknown (NullType) type, as_complex_dml_typedid._dialect_typemoves from the statement compiler to_ArrayTypeInspector.dialect_type(), so both compilers share it.docs/sqlalchemy.mdgains a "DDL and CAST types" section, and the API reference listsAthenaDMLTypeCompiler.The ARRAY section no longer says Athena requires
INTin nested DDL; SQLAlchemy type compiler emits invalid DDL for empty STRUCT, JSON, and CLOB columns #886 measured that it acceptsINTEGER.Behavior changes for the 4.0.0 release notes:
TypeEngine.compile(dialect)forAthenaStructrendersSTRUCT<name:type>instead ofROW(name type), and nested integers renderINT(for exampleMAP<INT, STRING>instead ofMAP<INTEGER, STRING>).IntegerrendersINTin all DDL, includingTypeEngine.compile(), a subclass ofInteger, and aTypeDecoratoroverInteger(previouslyINTEGERthere).Integer().with_variant(OtherType(), "awsathena")now renders the variant in CREATE TABLE (for exampleBIGINT). Before, theINTspecial case ignored the variant.JSONcolumn (or a complex type containing JSON) in CREATE TABLE raisesCompileErrorinstead of emittingJSON, which Athena rejects.CAST(... AS JSON)is unchanged.AthenaStructwithout fields raisesCompileErrorin CREATE TABLE and in CAST instead of emittingROW(), which Athena rejects. A table reflected from a top-levelstruct<...>column has such a type until reflection parses its fields (follow-up PR for SQLAlchemy type compiler emits invalid DDL for empty STRUCT, JSON, and CLOB columns #886).CLOB/NCLOBcolumns renderSTRINGinstead ofBINARY.Not in this PR:
struct<...>columns and of top-levelmap<...>columns (now reflected asString). This changes reflected types, so it will be a separate PR for SQLAlchemy type compiler emits invalid DDL for empty STRUCT, JSON, and CLOB columns #886.WHY
Refs #886.
Athena parses DDL statements with Hive type syntax and queries with Trino type syntax.
AthenaTypeCompilerrendered both, switching between them with a column check and a flag thatvisit_arrayset.Direct compilation therefore produced types that are valid in neither syntax, and CAST rendering was spread across
visit_cast,_complex_dml_type, and a fallback to the DDL compiler (for exampleCAST(... AS JSON)came from the DDL compiler'svisit_JSON).With one compiler per syntax, the DDL compiler can reject types that Hive DDL does not accept without affecting CAST.
Athena results for the rejected forms are recorded in #886:
struct<>andJSONcolumn types fail to parse in CREATE TABLE,CAST(NULL AS ROW())fails to parse, andINTEGER/MAP<STRING, INTEGER>parse in DDL.TEST
Tested commit: 3c61716 (the AWS SQLA compliance suites ran on 268f8d6; see below).
just lint: passed.TypeEngine.compile(), and CREATE TABLE column DDL for 58 types, plus 11 ARRAY bind, slice, and partial-update statements, on master and on this branch.Every CAST and DML statement line is identical, except that the casts to an empty
AthenaStructnow raise.The DDL differences are exactly the behavior changes listed above.
tests/pyathena/sqlalchemy/test_compiler.py: tests updated and added for both type compilers.Run against master's
pyathena/, exactly the 25 tests for the intended DDL/empty-ROW/JSON changes fail.test_cast_renders_subclass_by_base_class,test_cast_applies_compilation_rule_of_decorator,test_array_assignment_rejects_unknown_value_type,test_array_bind_rejects_decorated_numeric_without_precision, andtest_array_bind_rejects_numeric_subclass_without_precisionguard CAST behavior that master already had; they cover regressions found in earlier commits of this branch (684eccd, 8570146, 084d773).uv run --env-file .env pytest -n 8 tests/pyathena/sqlalchemy/ tests/pyathena/aio/sqlalchemy/on 3c61716: 670 passed.uv run --env-file .env just test sqlaon 268f8d6: 589 passed, 759 skipped.uv run --env-file .env just test sqla-asyncon 268f8d6: 589 passed, 759 skipped.The later commits change how CAST types are dispatched (CAST output for the 58 compared types is unchanged) and change docs. They were not rerun locally. CI run 37104859124 passed both suites on 084d773, and CI reruns them once the PR is Ready again.
just docs lint: passed.sphinx-build docson the working tree: no warnings from the compiler classes.just test pyathena(non-SQLAlchemy suites); it runs in CI once the PR is Ready.🤖 Generated with Claude Code