Skip to content

Split SQLAlchemy type rendering into Hive DDL and Trino DML compilers - #961

Merged
laughingman7743 merged 6 commits into
masterfrom
fix/886-type-compiler-ddl-dml
Oct 3, 2026
Merged

laughingman7743 merged 6 commits into
masterfrom
fix/886-type-compiler-ddl-dml

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

WHAT

Split SQLAlchemy type rendering into a Hive DDL type compiler and a Trino DML type compiler.

  • AthenaTypeCompiler (the dialect's type_compiler_cls) renders Hive DDL types only.
    It renders CREATE TABLE column types and TypeEngine.compile().
    • STRUCT<name:type>, MAP<key, value>, ARRAY<item>, and INT in every context, including direct compilation. Before, direct compilation of a STRUCT or MAP rendered a hybrid such as ROW(name STRING).
    • The type_expression=Column check, the _athena_hive_ddl flag, and the INT special case in get_column_specification are removed.
    • JSON and an AthenaStruct without fields raise CompileError. This applies at any nesting depth (for example ARRAY<JSON>).
    • CLOB/NCLOB render STRING instead of BINARY.
    • visit_struct, visit_map, and visit_array raise CompileError for a type of an unexpected class, instead of rendering ROW(), MAP<STRING, STRING>, or ARRAY<STRING>.
  • The new AthenaDMLTypeCompiler renders the Trino types of CAST expressions: VARCHAR, INTEGER, REAL, VARBINARY, TIMESTAMP(6)/TIMESTAMP(p), JSON, ROW(name type), MAP(key, value), and ARRAY(item).
    It replaces the type branches of AthenaStatementCompiler.visit_cast and _complex_dml_type, and it keeps their require_precision and timestamp_precision options as keyword arguments of process().
    An empty AthenaStruct raises CompileError here too, instead of rendering ROW().
    After resolving variants and TypeDecorators, it matches ARRAY, MAP, ROW, String, binary, Double, Float, and DateTime types by class, as the former visit_cast did, so a subclass renders like its base whatever its visit name (for example oracle.DATE renders TIMESTAMP(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_type callers use process_element(), which resolves the type first and rejects an unknown (NullType) type, as _complex_dml_type did.
  • _dialect_type moves from the statement compiler to _ArrayTypeInspector.dialect_type(), so both compilers share it.
  • docs/sqlalchemy.md gains a "DDL and CAST types" section, and the API reference lists AthenaDMLTypeCompiler.
    The ARRAY section no longer says Athena requires INT in nested DDL; SQLAlchemy type compiler emits invalid DDL for empty STRUCT, JSON, and CLOB columns #886 measured that it accepts INTEGER.

Behavior changes for the 4.0.0 release notes:

  • TypeEngine.compile(dialect) for AthenaStruct renders STRUCT<name:type> instead of ROW(name type), and nested integers render INT (for example MAP<INT, STRING> instead of MAP<INTEGER, STRING>).
  • Integer renders INT in all DDL, including TypeEngine.compile(), a subclass of Integer, and a TypeDecorator over Integer (previously INTEGER there).
  • A column declared as Integer().with_variant(OtherType(), "awsathena") now renders the variant in CREATE TABLE (for example BIGINT). Before, the INT special case ignored the variant.
  • A JSON column (or a complex type containing JSON) in CREATE TABLE raises CompileError instead of emitting JSON, which Athena rejects. CAST(... AS JSON) is unchanged.
  • An AthenaStruct without fields raises CompileError in CREATE TABLE and in CAST instead of emitting ROW(), which Athena rejects. A table reflected from a top-level struct<...> 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/NCLOB columns render STRING instead of BINARY.

Not in this PR:

WHY

Refs #886.
Athena parses DDL statements with Hive type syntax and queries with Trino type syntax.
AthenaTypeCompiler rendered both, switching between them with a column check and a flag that visit_array set.
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 example CAST(... AS JSON) came from the DDL compiler's visit_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<> and JSON column types fail to parse in CREATE TABLE, CAST(NULL AS ROW()) fails to parse, and INTEGER/MAP<STRING, INTEGER> parse in DDL.

TEST

Tested commit: 3c61716 (the AWS SQLA compliance suites ran on 268f8d6; see below).

  • just lint: passed.
  • A script compiled CAST, 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 AthenaStruct now 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, and test_array_bind_rejects_numeric_subclass_without_precision guard 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 sqla on 268f8d6: 589 passed, 759 skipped.
  • uv run --env-file .env just test sqla-async on 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 docs on the working tree: no warnings from the compiler classes.
  • Not run locally: the rest of just test pyathena (non-SQLAlchemy suites); it runs in CI once the PR is Ready.

🤖 Generated with Claude Code

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>
@laughingman7743 laughingman7743 added this to the 4.0.0 milestone Oct 3, 2026
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):

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, 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_ddl and the get_column_specification INT case, JSON/empty STRUCT/CLOB, and CompileError for unexpected classes.
  • DML type compiler: variant and TypeDecorator resolution (_ArrayTypeInspector.dialect_type, which is the former _dialect_type), the require_precision/timestamp_precision options, NULL element rejection, and quoting with the DML preparer.
  • Callers: visit_cast, _array_slice_step, _array_json, and the three _ArrayUpdateCompiler call sites.
  • Untyped-array update: an element assignment now renders the RHS type before _rebuild validates 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:

  1. A type subclass with its own __visit_name__ regressed in CAST (see the comment on visit_unsupported_compilation).
  2. 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.

Comment thread pyathena/sqlalchemy/compiler.py Outdated
return self.process(type_, **kw)

@override
def visit_unsupported_compilation( # type: ignore[override] # base returns NoReturn

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.

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.

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.

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

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.

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>
Comment thread pyathena/sqlalchemy/compiler.py Outdated
return self.process(type_, **kw)

@override
def visit_unsupported_compilation( # type: ignore[override] # base returns NoReturn

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 (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/Numeric subclass with its own __visit_name__ now casts as INTEGER/DECIMAL, where master raised UnsupportedCompilationError. 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_compilation exists on TypeCompiler in 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_ddl have no callers left in the repository.
  • TypeEngine.compile() output changes for STRUCT/MAP/INT, and Alembic alter_column uses 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.

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.

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.

Comment thread docs/sqlalchemy.md
| `AthenaArray`, `ARRAY` | `ARRAY<item>` | `ARRAY(item)` |

Complex types apply the same syntax to their nested types.
An `AthenaStruct` without fields raises `CompileError` in both.

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.

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.

Comment thread docs/sqlalchemy.md

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

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.

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:

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 Codex result)

  • Reviewer: Codex CLI 0.160.0, model gpt-6-astra (a different model from the author). Session 01a1006f-c6dd-74e2-bdd2-622be4c0e40f.
  • Invocation: codex exec --sandbox read-only on a detached snapshot of 684eccd4fa1d5e801e94c66cceed31d4b9d84b94 (base 23ff61f6ecff6c74f5e7864641e3ea1a76f5e0a8). 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.
  1. P2, introduced: a subclass whose visit name names another handled type, e.g. type("T", (DateTime,), {"__visit_name__": "DATE"}), cast as DATE instead of TIMESTAMP(6). The visit-name fallback only covered missing visitors.
  2. P2, introduced: cast(x, T()), where T is a TypeDecorator over Integer with @compiles(T, "awsathena") returning BIGINT, rendered INTEGER instead of base's BIGINT, because the decorator was resolved before dispatch.
  3. P2, introduced: a column of type AthenaArray(NullType()).with_variant(AthenaArray(Integer), "awsathena") with t.update().values({t.c.a[1]: 1}) rendered CAST(... AS NULL) instead of raising, because the RHS call bypassed the element check.
  4. P3, test: "decorated INT" in ddl also passes for decorated 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.DATE subclasses 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 base visit_cast did. Other types dispatch the declared type, so decorator rules apply.
  • The former _complex_dml_type callers use process_element(), which rejects an unknown type at any level.
  • The round-one visit_unsupported_compilation fallback 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, and test_array_assignment_rejects_unknown_value_type. 17 fail on 684eccd4fa1d5e801e94c66cceed31d4b9d84b94; all except the nested decorator assertion pass on master's pyathena/. The 58-type comparison against base is unchanged apart from the empty-ROW errors. Offline: 480 passed. AWS tests/pyathena/sqlalchemy/ + aio: 668 passed on 857014657720868939b8132c6dd89fa62a585315.

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 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 @compiles rule returning DECIMAL, used as AthenaArray(D()) with a literal [Decimal("1.23")], rendered CAST(ARRAY[1.23] AS ARRAY(DECIMAL)) instead of raising the precision CompileError. Dispatching the declared nested decorator skipped visit_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_precision was added, and the nested assertion of test_cast_applies_compilation_rule_of_decorator now expects ARRAY(INTEGER). Both fail on 85701465.
  • 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 on 084d773b: 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_cast still uses process().
  • Claims: CLEAN. The AthenaDMLTypeCompiler and process_element() docstrings now state which path dispatches the declared type, and the PR body no longer claims the nested extension.

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.

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(

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.

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

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.

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:

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 independent-review repair (git range-diff 23ff61f6..684eccd4fa1d5e801e94c66cceed31d4b9d84b94 23ff61f6..857014657720868939b8132c6dd89fa62a585315; the merge-base is unchanged)

Round-one perspective (behavior, contracts, simplicity, tests): CLEAN.

  • process(): after dialect_type() resolution, ARRAY/MAP/ROW/String/binary/Double/Float/DateTime match by class in the same order as base visit_cast/_complex_dml_type. Other types dispatch the declared type through GenericTypeCompiler.process: the variant mapping applies, then visit_type_decorator re-enters process() with the impl. There is no recursion loop, because type_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_type caller: three in array.py and _array_slice_step. visit_cast keeps process(), so a top-level NullType renders NULL as 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 AthenaDMLTypeCompiler docstring matches process(). 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>
@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 06:59
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 07:52
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:

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.

Additional independent review (relayed Claude Fable 5.1 result, requested by the maintainer)

  • Reviewer: Claude Code 2.1.287 claude -p, model claude-fable-5-1, effort high, profile max. Auth was verified first: claude.ai, first-party, subscription max, not Enterprise, no API key override. Session 90d6c4b8-0ada-466e-a868-1b5962c02cf3.
  • Package: a fresh detached snapshot of 084d773b1f384c3aae2526aec92ecb26edab492e (base 23ff61f6ecff6c74f5e7864641e3ea1a76f5e0a8), 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 through get_column_specification and TypeEngine.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):

  1. Medium, "try_cast(col, JSON()) rendered TRY_CAST(col AS JSON) at base and now raises": rejected. On both master and 084d773b1f384c3aae2526aec92ecb26edab492e, try_cast(...) raises UnsupportedCompilationError from AthenaStatementCompiler for JSON, STRUCT, Integer, String, and Float targets. SQLAlchemy 2.0.46's SQLCompiler has no visit_try_cast, so the type clause is never reached. Supporting TRY_CAST would be a new feature, outside this PR.
  2. Low, Column("id", Integer().with_variant(BigInteger(), "awsathena")): confirmed, accepted as an improvement. It renders id INT on master (the removed type() in [Integer, ...] special case ignored the variant) and id BIGINT here. Covered in test_integer_subclass_and_decorator_columns_use_int and added to the PR body's behavior changes.
  3. Low, a Numeric subclass with its own __visit_name__ and an @compiles rule used as an ARRAY bind element: confirmed regression, repaired in 3c61716555cbfd83ca58da1b3f616e8f8b612d2a. Master raises "explicit Numeric precision"; 084d773b1f384c3aae2526aec92ecb26edab492e rendered ARRAY(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.

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 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 **kw reaches it for ARRAY elements, MAP keys/values, ROW fields, and the assignment value cast.
  • Call sites without require_precision are unchanged.
  • Removing the visit_DECIMAL override keeps the DECIMAL/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")

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.

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)

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.

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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 08:03
@laughingman7743
laughingman7743 merged commit fc80b95 into master Oct 3, 2026
14 checks passed
@laughingman7743
laughingman7743 deleted the fix/886-type-compiler-ddl-dml branch October 3, 2026 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant